input-output-hk / nami

Nami Wallet is a browser based wallet extension to interact with the Cardano blockchain. Support requests: https://iohk.zendesk.com/hc/en-us/requests/new
https://namiwallet.io
Apache License 2.0
372 stars 167 forks source link

Nami wallet signing transactions with the wrong keys #771

Open Quantumplation opened 1 year ago

Quantumplation commented 1 year ago

Hello!

We recently launched our ISO portal, and we're having a number of users reporting that the Nami wallet (often with a Ledger attached) is signing the transaction with the wrong private keys.

Here's a sample transaction:

84a600838258200316be61ac5f8cc3afbd1c9b7bf354fa7ee523bfb291aef70538ca24a7a478b001825820b5ebbdf4d432bb0975938619b1ab953db626e3fed22fbefb0bab919a14d215bb01825820f7dc00bd344e9dfc53f866edac97ae13baf0d4cfce14d0409fafabcc522fdb69010183a20058390151a2a3b41fc1a80af9e1f4e902d26ac264fc2028a13ee629af3c6d9beef8120e0298c457bd0c4b0b65648d269cbb80616f8ca04d2aa8e38b01821a001e8480a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451ae062ff90a200583901fde279f027aff73ea3fed34959236d0bbcbc96a0350b0a329a4556ded7244b4a8777b7dc6909f4640cf02ea4757a557a99fb483b05f87dfe01821a002dc6c0a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451b00000001f8d3e33ea20058390151a2a3b41fc1a80af9e1f4e902d26ac264fc2028a13ee629af3c6d9beef8120e0298c457bd0c4b0b65648d269cbb80616f8ca04d2aa8e38b011b0000005cfb02f017021a00032af9031a04bbf72a0758208d86230c246148a5a0a76434377780136011b31de0070cd6afee893de1c8a39f0e81581ceef8120e0298c457bd0c4b0b65648d269cbb80616f8ca04d2aa8e38ba100818258201d2c6f36ea927f2efe65932ce4af8da467211f6f837ee17612577c414e4dca845840d05f24a476e483f623ede64769bcc5096e0d6e57defedd1e0daaef6f03b4ed589bfd0660e1b2f36003f36db85c674b5a482fcbe4a55210a7f82d15297e5e8404f5d90103a100a11a000df3f9a178386565663831323065303239386334353762643063346230623635363438643236396362623830363136663863613034643261613865333862a1645249534fa2676e756d44617973016b746f74616c416d6f756e741ae062ff90

Essentially, we take a UTXO from the users wallet, a UTXO from our wallet, and transfer an appropriate amount of SUNDAE in the process.

We include the users staking key in the requiredSigners, to prove they have ownership of the staking address which earned the ISO rewards. I suspect this might be the root cause of the issue.

The user receives the following error:

transaction submit error ShelleyTxValidatorError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (FromAlonzoUtxowFail (WrappedShelleyEraFailure (InvalidWitnessUTXOW [VKey (VerKeyEd25519DSIGN \"79acaa9baf732d775bb19120899399da9db0e12a38bbf93db3c6940c3ec4bf84\")]))),UtxowFailure (FromAlonzoUtxowFail (MissingRequiredSigners (fromList [KeyHash \"eef8120e0298c457bd0c4b0b65648d269cbb80616f8ca04d2aa8e38b\"])))])"

(I may have transcribed that from the screenshot wrong, but you get the idea; Screenshot below)

Other wallets have been able to sign these transactions, so I suspect it's some crossed wires around the API call to the ledger device.

Let me know if there's any other details I can provide to be helpful!

Regards, Pi Lanningham

image

Quantumplation commented 1 year ago

@Monseceaser given that Eternl is able to sign these transactions just fine, I don't believe it's a ledger issue. I believe Nami is invoking the ledger API incorrectly.

GGAlanSmithee commented 1 year ago

@Quantumplation - don't. @Monseceaser is a scammer trying to phish your crypto.

alessandrokonrad commented 1 year ago

Hey @Quantumplation, what signatures are required for these transactions? Payment key and stake key right? It's a bit odd, because these transactions do not look any special and I think that the issue would have popped up much earlier already.

Are you sure you are not including any UTxOs or other things into the tx that can only be signed by wallets that have multiple addresses?

Quantumplation commented 1 year ago

@alessandrokonrad as far as I know, no;

We get a list of utxos from the CIP-30 method, so unless that could return utxos from another address.

Then, we send that to the backend; the backend selects our own utxo, and builds the atomic swap transaction, with the users stake address in the required signers. We sign the transaction, and put the witness in the witness set.

The server then returns the cbor hex for that tx, and passes it to the wallet via CIP-30.

alessandrokonrad commented 1 year ago

@alessandrokonrad as far as I know, no;

We get a list of utxos from the CIP-30 method, so unless that could return utxos from another address.

Then, we send that to the backend; the backend selects our own utxo, and builds the atomic swap transaction, with the users stake address in the required signers. We sign the transaction, and put the witness in the witness set.

The server then returns the cbor hex for that tx, and passes it to the wallet via CIP-30.

Can you confirm that it doesn't work with normal Nami soft wallets? You said it often happens on Ledger.

Quantumplation commented 1 year ago

I don't have any direct confirmations of it happening on soft wallets; I've had a dozen or so reports, and maybe half of them have confirmed they're using a ledger, with no information on whether or not it's a ledger from the others (from before we thought to start asking).

So, it may be an exclusively Nami<>Ledger issue. I'll try to reproduce with my ledger and Nami this afternoon.

Quantumplation commented 1 year ago

@alessandrokonrad I've verified with the users that have reported this that it is always with Ledger hardware wallets. I haven't had any reports of this issue from Trezor wallets (don't have one to test with myself), and none from software wallets.

Quantumplation commented 1 year ago

@alessandrokonrad Any update on this?

alessandrokonrad commented 1 year ago

@alessandrokonrad Any update on this?

I will look into this today

alessandrokonrad commented 1 year ago

Okay this should be fixed now with 3.4.4

Quantumplation commented 1 year ago

@alessandrokonrad I got a report today from someone on Nami 3.4.4 and is still receiving this error.

"transaction submit error ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (FromAlonzoUtxowFail (WrappedShelleyEraFailure (InvalidWitnessesUTXOW [VKey (VerKeyEd25519DSIGN \"13ca17a21fa5d19d9f83d333afeb18767c2d3b89bf479558318f7027afd039a2\"),VKey (VerKeyEd25519DSIGN \"4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5\")])))])"

Here's the CBOR Hex for the transaction we send to Nami:

84a600828258206e3f964d860c875fc543eb62cd428ccee9f4dc11c6c4ccdfc9315663f08c669501825820bd892dddbf5c3c00c8da431f41feaca92f5440a213450425fa5f6a084462ff66184c0183a2005839014306719d301c555a059222c83eb3d6d1a5436246a8803d17640affefb96186f2d8974fd49051478441bf677b3d50275646c241064140b01801821a001e8480a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451a09cec2d1a200583901fde279f027aff73ea3fed34959236d0bbcbc96a0350b0a329a4556ded7244b4a8777b7dc6909f4640cf02ea4757a557a99fb483b05f87dfe01821a0016e360a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451b000000037443132fa2005839014306719d301c555a059222c83eb3d6d1a5436246a8803d17640affefb96186f2d8974fd49051478441bf677b3d50275646c241064140b018011a00f62052021a000322b9031a04fa1ab807582092f0ff0b7998705936014bd4296c4eec3d940783d63582eb8b00bfd1c48fba480e81581cb96186f2d8974fd49051478441bf677b3d50275646c241064140b018a100818258201d2c6f36ea927f2efe65932ce4af8da467211f6f837ee17612577c414e4dca84584031eae0b858cd83f8bb982335dd4b1bbcf86cacfe0a45903770ce0c4782af6748c3cab2e26e928a62570d86ca053417b41abd139779f7cc56d72071175a74750ff5d90103a100a11a000df3f9a178386239363138366632643839373466643439303531343738343431626636373762336435303237353634366332343130363431343062303138a26349534fa2676e756d44617973016b746f74616c416d6f756e741a051e0127645249534fa2676e756d44617973016b746f74616c416d6f756e741a04b0c1aa

User is using a Ledger Nano X and confirmed that it is also updated to the latest version.

13ca17a21fa5d19d9f83d333afeb18767c2d3b89bf479558318f7027afd039a2 for the error message hashes to 4306719d301c555a059222c83eb3d6d1a5436246a8803d17640affef, which is the payment key for both of the input UTXOs, and 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5 hashes to 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5, which is the staking key.

alessandrokonrad commented 1 year ago

@alessandrokonrad I got a report today from someone on Nami 3.4.4 and is still receiving this error.

"transaction submit error ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (FromAlonzoUtxowFail (WrappedShelleyEraFailure (InvalidWitnessesUTXOW [VKey (VerKeyEd25519DSIGN \"13ca17a21fa5d19d9f83d333afeb18767c2d3b89bf479558318f7027afd039a2\"),VKey (VerKeyEd25519DSIGN \"4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5\")])))])"

Here's the CBOR Hex for the transaction we send to Nami:

84a600828258206e3f964d860c875fc543eb62cd428ccee9f4dc11c6c4ccdfc9315663f08c669501825820bd892dddbf5c3c00c8da431f41feaca92f5440a213450425fa5f6a084462ff66184c0183a2005839014306719d301c555a059222c83eb3d6d1a5436246a8803d17640affefb96186f2d8974fd49051478441bf677b3d50275646c241064140b01801821a001e8480a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451a09cec2d1a200583901fde279f027aff73ea3fed34959236d0bbcbc96a0350b0a329a4556ded7244b4a8777b7dc6909f4640cf02ea4757a557a99fb483b05f87dfe01821a0016e360a1581c9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77a14653554e4441451b000000037443132fa2005839014306719d301c555a059222c83eb3d6d1a5436246a8803d17640affefb96186f2d8974fd49051478441bf677b3d50275646c241064140b018011a00f62052021a000322b9031a04fa1ab807582092f0ff0b7998705936014bd4296c4eec3d940783d63582eb8b00bfd1c48fba480e81581cb96186f2d8974fd49051478441bf677b3d50275646c241064140b018a100818258201d2c6f36ea927f2efe65932ce4af8da467211f6f837ee17612577c414e4dca84584031eae0b858cd83f8bb982335dd4b1bbcf86cacfe0a45903770ce0c4782af6748c3cab2e26e928a62570d86ca053417b41abd139779f7cc56d72071175a74750ff5d90103a100a11a000df3f9a178386239363138366632643839373466643439303531343738343431626636373762336435303237353634366332343130363431343062303138a26349534fa2676e756d44617973016b746f74616c416d6f756e741a051e0127645249534fa2676e756d44617973016b746f74616c416d6f756e741a04b0c1aa

User is using a Ledger Nano X and confirmed that it is also updated to the latest version.

13ca17a21fa5d19d9f83d333afeb18767c2d3b89bf479558318f7027afd039a2 for the error message hashes to 4306719d301c555a059222c83eb3d6d1a5436246a8803d17640affef, which is the payment key for both of the input UTXOs, and 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5 hashes to 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5, which is the staking key.

I don't see where the staking key 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5 is used in the transaction you posted above. Or is the tx not related to it? In general where do you add the staking key, is it just part of the required signers?

Quantumplation commented 1 year ago

@alessandrokonrad sorry, I didn't get a notification for your response, probably because the GH issue is closed;

There's a typo in my original message; 4b8cd2675f10442457897f78d9a9d1cb3cebc83d39b81c0cee70ed608fa04fb5 hashes to b96186f2d8974fd49051478441bf677b3d50275646c241064140b018, which is the users staking key hash. You can see that b96... is in the required signers field of the CBOR I pasted.

So essentially this error message is saying "the signature provided for these two keys is invalid";

In my experience with other wallets, this is most often because either:

Claiming with a Ledger definitely works from other wallets, so I'm fairly confident that we're producing the transactions in the canonical format, but I'm happy to fix it if you can point to an issue on our end.

Also, if you want, I can set you up with some mock rewards on the preview environment so you can try to reproduce yourself, if that would be easier.

Quantumplation commented 1 year ago

@alessandrokonrad Any update on this? Can we reopen the ticket since it's still happening?

alessandrokonrad commented 1 year ago

@Quantumplation I think I found the root cause. The transaction above used the post alonzo format for outputs even though no inline datums or script refs were used. That's at least how I checked it for HWs. I make a more precise check now directly on the cbor encoded output to figure out which format is used. Hopefully this fixes the issue. I published a new Nami version already. The one with the fix is 3.4.8.

Quantumplation commented 1 year ago

@alessandrokonrad Ooo, thank you! Maybe that's what's tripping up Flint and Typhon too.

Sorry this was a thorn in your side, and thanks for fixing it!

Quantumplation commented 1 year ago

@alessandrokonrad I'm not sure if it's the same issue, or a new one introduced by this fix, but we're still having failed transactions with hardware wallets.

Here's the whole flow:

This has a transaction ID of e810cb1268043ff2897b96453fc77befdca0cbc6da9fb40522e569f17af7373c, and the signature from our wallet in the witness set is:

{ 
  "publicKey": "78b0eff557a5468f74ca5cc03a55ad3f9310568a037f9b295360b9e9316c953d",
  "signature": "d876f49889fa242542354cf7b9120e9ee3ee30427ab04c74ea0bd5dfe279d2bf6c66a564912f8e75c42c163ba7414ca603ea90de217a726f1b6120d7603b030a"
}

When I validate this signature manually, it reports that it's a valid signature.

We then pass this to Nami for a signature, and get the following witness set back:

a100818258209c666c7700259c0ff8c329de522b5f0223529e39e3aec419790c3a7ae3ae47be584014f132cafca9e5162e6de84ebc4cf08c9e4f168f5135fc9be5678e36901338055f7385d1586f0550d74dc71ca3f9c76e490e89eef6cd4bab7174ba02e0afdb05

Which is equivalent to

{
  "publicKey": "9c666c7700259c0ff8c329de522b5f0223529e39e3aec419790c3a7ae3ae47be",
  "signature": "14f132cafca9e5162e6de84ebc4cf08c9e4f168f5135fc9be5678e36901338055f7385d1586f0550d74dc71ca3f9c76e490e89eef6cd4bab7174ba02e0afdb05"
}

This signature is not a valid ed25519 signature for the txHash above.

We insert this into the witness set into the transaction, as so:

84a5008282582078118007ebbe60f444db457dff794dfc52c69c7ebab61e4c230eaf688dc2c8f60182582099ab8df99c1015d0d6d25bce4e7f1a373a43e59e8234a269c716f71b96e00788000183a200583901e88f632267e4cfa839451d0b7ef937a25a9376b515cedf31375c1b545413db12f0dbc98b3449a306fc4096da2a496dd0c04c33ff4ff3ac0d01821a001e8480a1581cda8c30857834c6ae7203935b89278c532b3995245295456f993e1d24a1424c511a0de003b3a20058390171f487196b6e4cc826b0df2ffc2fac03f69830e6119a4dd6d0646a8bd7244b4a8777b7dc6909f4640cf02ea4757a557a99fb483b05f87dfe01821a0044aa20a1581cda8c30857834c6ae7203935b89278c532b3995245295456f993e1d24a1424c511a0df59d23a200583901e88f632267e4cfa839451d0b7ef937a25a9376b515cedf31375c1b545413db12f0dbc98b3449a306fc4096da2a496dd0c04c33ff4ff3ac0d011a00887a7d021a00031b55031a057106930e81581ce88f632267e4cfa839451d0b7ef937a25a9376b515cedf31375c1b54a1008282582078b0eff557a5468f74ca5cc03a55ad3f9310568a037f9b295360b9e9316c953d5840d876f49889fa242542354cf7b9120e9ee3ee30427ab04c74ea0bd5dfe279d2bf6c66a564912f8e75c42c163ba7414ca603ea90de217a726f1b6120d7603b030a8258209c666c7700259c0ff8c329de522b5f0223529e39e3aec419790c3a7ae3ae47be584014f132cafca9e5162e6de84ebc4cf08c9e4f168f5135fc9be5678e36901338055f7385d1586f0550d74dc71ca3f9c76e490e89eef6cd4bab7174ba02e0afdb05f5f6

And get the following error when we submit:

{"code":2,"info":"Wallet could not send the tx.","message":"\"transaction submit error ShelleyTxValidationError ShelleyBasedEraBabbage (ApplyTxError [UtxowFailure (FromAlonzoUtxowFail (WrappedShelleyEraFailure (InvalidWitnessesUTXOW [VKey (VerKeyEd25519DSIGN \\\"9c666c7700259c0ff8c329de522b5f0223529e39e3aec419790c3a7ae3ae47be\\\")])))])\""}

This is complaining about our signature. The most likely/common reason for this is:

Can we please re-open this ticket until we've confirmed with a few end-users that the issue is resolved?

alessandrokonrad commented 1 year ago

@Quantumplation I'm pretty sure it's not related to Nami (at least for normal wallets). It doesn't transform or change the bytes. The signature is made on the raw bytes it receives initially. It's only when passing it to the hardware wallets, which require a different transaction format. So the conversion here may be wrong, however I look at those transactions you construct and they are not complicated. They include a required signer and some metadata. I replicated that and signed such a tx with my hw through Nami and it works flawlessly. Is there anything special you include in those transactions I might be missing?

Quantumplation commented 1 year ago

@alessandrokonrad Nothing that I can think of; they're generated via cardano-cli, then normalized according to RFC 7049 Section 3.9 Canonical CBOR. This puts map keys in order, omits fields without a value (nulls and empty maps), encodes Ints in a canonical way, etc.

One thing that might be causing it: if there is no collateral field set, are you omitting it, or setting it to an empty array?

alessandrokonrad commented 1 year ago

Oh I just realized the recent change I made didn't have any effect. When I convert the outputs to bytes then it does exactly the same checks. Serialize to alonzo format if only address, assets and datum hash, otherwise to post alonzo format.

I wanna keep the support for both formats for now, but at one point I can get rid of the legacy format. To actually detect the format of the output right now I would need to make quite a few changes. The easiest solution would be if you serialize the outputs in the legacy format if they only have what I described above.

Quantumplation commented 1 year ago

@alessandrokonrad Unfortunately that's not possible for us right now, unless you know a way to tell cardano-cli to serialize with the legacy format. (I know, I know, I probably shouldn't be using cardano-cli to build the transactions :sweat_smile: but that's probably an even bigger change for this particular backend)

Quantumplation commented 1 year ago

@alessandrokonrad if you want to drop a link to the relevant parts of the code, I can take a stab at submitting a pull request!

alessandrokonrad commented 1 year ago

@Quantumplation check version 3.4.9. I'll publish it soon, but this should do fix it finally.