unchained-capital/caravan

JS error when using Enter as text with incorrect access type

periodic1236 opened this issue · 6 comments

periodic1236 commented

I'm submitting a ...

  • Regression (a behavior that used to work and stopped working in a new release)
  • Bug report
  • Feature request
  • Documentation issue or request
  • Support request

Expected Behavior

Caravan allows for manual entry of public keys when creating a single multisig address. However, there is inadequate validation to ensure the provided keys are compatible with the selected Address Type.

Public keys used in a version 0 witness program must be passed in the compressed format (BIP143). Transactions that try to use uncompressed keys (65 bytes) are not relayed or mined by default.

P2SH-P2WSH and P2WSH addresses should not be derivable from redeem scripts containing uncompressed public keys, or funds sent to them may be lost forever.

Current Behavior

No warning is displayed on the page when uncompressed public keys are entered as text and either "P2SH-P2WSH" or "P2WSH" is the chosen address type. The corresponding address details are dutifully displayed to the user by the application, suggesting that everything is fine despite the danger lurking just around the corner.

This very pitfall caught one redditor by surprise this month.

Possible Solution

If either P2SH-P2WSH or P2WSH is selected, do one or more of the following:

  • do not show an address unless all provided keys are valid compressed keys (33 bytes)
  • display a prominent warning that funds being sent to the created address may be unspendable
  • convert uncompressed public keys to the corresponding compressed format AND display a warning that this has been done, since not all wallet software may be able to handle the distinction especially if user is working with an "uncompressed" private key

Steps to Reproduce

Open Caravan, click on "Address", choose "P2WSH" under the Address Type heading of the sidebar, then under any Public Key # section, select "Enter as text" from the dropdown and add an uncompressed public key (beginning with "04"). Fill in the other public keys, then look for the new P2WSH address that has been generated.

bruce1971 commented

When following the "Steps to Reproduce", I do not manage to generate any address.
Instead, I run into the "Something went wrong." error page:

x.mov
bruce1971 commented

This error page is originated by the validatePublicKey function from "unchained-bitcoin" package, that in turn uses the "bitcoinjs-lib" package, that throws the following error:

"TypeError: redeem.input or redeem.output contains uncompressed pubkey"

I will work on a PR, that improves PK validation when using "Enter as text". Hence users won't be able to add uncompressed PK... that causes the error.

periodic1236 commented

Good to see the upstream library no longer allows this. Thanks for taking a look!

waldenraines commented

I will work on a PR, that improves PK validation when using "Enter as text". Hence users won't be able to add uncompressed PK... that causes the error.

I changed the title to reflect the above.

Now what we want to fix here is to show a friendlier error message for this:

"TypeError: redeem.input or redeem.output contains uncompressed pubkey"

bruce1971 commented

Hi guys, just submitted my PR with a proposed fix: #205
Thanks for taking a look if you have the time.
Let me know if you have any questions!

bruce1971 commented

Hi guys, after @bucko13 feedback, I amended my fix:

  1. I extended the validatePublicKey() function in unchained-bitcoin to accept addressType as a parameter. See PR
  2. Using this extended the validatePublicKey(), I added code to caravan to validate that uncompressed public keys are not accepted for P2WSH and P2SH-P2WSH. See PR

Thank you for taking a look! Let me know if you have any questions/feedback :)