Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Wrong witnesses for signing a token tx with one Ada only change output #125

Closed
MarcelKlammer opened this issue Aug 15, 2021 · 9 comments

Comments

@MarcelKlammer
Copy link

We found an edge case in ccwallet, that is reproducable:

Sending a token, that produces an Ada only change output fails with "InvalidWitnessesUTXOW" (cardano-cli)

The cardano app uses wrong witnesses to sign the transaction which results in a tx hash mismatch (cardano-serialization-lib vs ledger cardano-app). Submitting that signed tx via cardano-cli fails with InvalidWitnessesUTXOW

Setup:

Produce a utxo with 4 (or more) Ada + 1 (or more) asset

Send that 1 asset to any address using the minimum amount of Ada necessary (1.444443), which will produce a change output, that consists of only an Ada amount.

The ledger will sign the tx, but cardano-cli fails with InvalidWitnessesUTXOW. Also the produced tx hash is not matching the tx hash calculated by cardano-serialization-lib.

Tried with 3.2.1, 3.1.0, 3.0.0. Same behavior. Also tested Testnet and Mainnet. Same behavior.

Sanity check:

Produce a utxo with 4 (or more) Ada + 2 assets

Send 1 asset to any address using the minimum amount of Ada necessary (1.444443), which will produce a change output, that consists of an Ada amount and the second token. The cardano-app signs that tx correctly. All fine.

@Scitz0
Copy link

Scitz0 commented Aug 15, 2021

Confirmed on my end as well.

@refi93
Copy link
Collaborator

refi93 commented Aug 16, 2021

@MarcelKlammer could you please share an unsigned transaction body produced by your wallet that doesn't match with the transaction hash obtained from Ledger?

@Scitz0
Copy link

Scitz0 commented Aug 16, 2021

@MarcelKlammer could you please share an unsigned transaction body produced by your wallet that doesn't match with the transaction hash obtained from Ledger?

Not sure if this is the proper way to serialize the txBody object generated by cardano-serialization-lib.
Buffer.from(txBody.to_bytes()).toString('hex')
generates the following data:
a40082825820aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a295300825820aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a2953010182825839003362ba78427d72c57cee3a6940d912ce68daff3823b394f6e14a1d56ac55cbd289d79e492ec999bb8da678f7d280526da1013e2d30978a6b821a00160a5ba1581c349911218a23c92792490225bc827689527cf9bc1eb14e31151a06b4a14741484c636f696e05825839002e9d61ade84a006946361c0c6e7a6b7878df07e49c872474df80026dac55cbd289d79e492ec999bb8da678f7d280526da1013e2d30978a6b821b00000014beff5a7ba0021a0002b11d031a0212257e

JSON generated and sent to ledgerjs lib for signing:
{"signingMode":"ordinary_transaction","tx":{"network":{"protocolMagic":1097911063,"networkId":0},"inputs":[{"txHashHex":"aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a2953","outputIndex":0,"path":[2147485500,2147485463,2147483648,0,0]},{"txHashHex":"aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a2953","outputIndex":1,"path":[2147485500,2147485463,2147483648,1,10]}],"outputs":[{"destination":{"type":"device_owned","params":{"type":0,"params":{"spendingPath":[2147485500,2147485463,2147483648,0,0],"stakingPath":[2147485500,2147485463,2147483648,2,0]}}},"amount":"1444443","tokenBundle":[{"policyIdHex":"349911218a23c92792490225bc827689527cf9bc1eb14e31151a06b4","tokens":[{"assetNameHex":"41484c636f696e","amount":"5"}]}]},{"destination":{"type":"device_owned","params":{"type":0,"params":{"spendingPath":[2147485500,2147485463,2147483648,1,11],"stakingPath":[2147485500,2147485463,2147483648,2,0]}}},"amount":"89103751803"}],"fee":"176413","ttl":"34743678"}}

Tx Hash generated by cardano-serialization-lib:
b3ee0843b367cea5fa8ebd5ce37e814490e0c594197d48757557cc8cf70de920
Tx Hash I got back from ledgerjs lib after successfully signing it with ledger device:
0e58b6049cadb44ece023abcefc9844ea02b36ddb09c13e1667fa0e7cea223a3

@refi93
Copy link
Collaborator

refi93 commented Aug 16, 2021

Thanks @Scitz0 ! did you also try to create the transaction directly with the cardano-cli, or did you just manually paste the raw tx body bytes into the file to be passed to cardano-cli to send the transaction? If the former, could you forward the command used for assembling the transaction?

@Scitz0
Copy link

Scitz0 commented Aug 16, 2021

Thanks @Scitz0 ! did you also try to create the transaction directly with the cardano-cli, or did you just manually paste the raw tx body bytes into the file to be passed to cardano-cli to send the transaction? If the former, could you forward the command used for assembling the transaction?

cardano-serialization-lib by emurgo (referred to as csl from now on) is used to construct the tx body using the TransactionBuilder. cardano-cli is not involved in this process. It's done entirely in browser/JS. From the csl TransactionBuilder a csl TransactionBody object is generated once all inputs/outputs etc are added to the builder. This object is what I refer to in the previous post as txBody. We then construct the object needed by ledgerjs lib based on the built tx body ("JSON generated" pasted in the previous post) and pass this along to Ledger.Ada.signTransaction() for signing. Witnesses we get back is then used to assemble the full csl Transaction object, this is then serialized and sent to the backend to be posted to chain(through graphql/cardano-cli currently)

FYI, 7.1.0 of cardano-serialization-lib used. So the issue might be related to csl and ledgerjs libs handling empty token bundles differently?

@refi93
Copy link
Collaborator

refi93 commented Aug 16, 2021

So the issue might be related to csl and ledgerjs libs handling empty token bundles differently?

Yes, seems so, the tx body

a40082825820aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a295300825820aa863c6fd227a42c9a8b0d55621f0df0bd9c178e140f500d51bd3d51b17a2953010182825839003362ba78427d72c57cee3a6940d912ce68daff3823b394f6e14a1d56ac55cbd289d79e492ec999bb8da678f7d280526da1013e2d30978a6b821a00160a5ba1581c349911218a23c92792490225bc827689527cf9bc1eb14e31151a06b4a14741484c636f696e05825839002e9d61ade84a006946361c0c6e7a6b7878df07e49c872474df80026dac55cbd289d79e492ec999bb8da678f7d280526da1013e2d30978a6b1b00000014beff5a7b021a0002b11d031a0212257e

Has the blake2b256 hash matching the one from ledgerjs, i.e. 0e58b6049cadb44ece023abcefc9844ea02b36ddb09c13e1667fa0e7cea223a3 - notice I removed the empty token bundle and encapsulating array to encode the standalone coin amount (89103751803)

I raised a related issue in the cardano-serialization-lib repo: Emurgo/cardano-serialization-lib#189

@refi93
Copy link
Collaborator

refi93 commented Aug 16, 2021

I made a patched version of the library which should omit an empty token bundle when serializing the output amount, I pre-released it here, could you try if it works for you?

https://github.com/vacuumlabs/cardano-serialization-lib/releases/tag/7.1.0-patch-token-bundle-serialization

please note that this is just a quick & dirty fix to the serialization itself by me and given I'm otherwise not familiar with the library codebase, this fix may not properly play with other parts of the logic like fee estimation or min ada amount per output computation (though I'd expect the resulting fee / change output ADA to be potentially higher than needed given the serialized output is smaller in the end in case of an empty token bundle, so the tx itself should be valid, just possibly suboptimal in terms of fee/required change ADA amount)

@MarcelKlammer
Copy link
Author

I'm currently on vacation and cannot test it.

We modified the CSL package.json and added the target web (instead of browser), and may have modified the exported wrapper code as well. Can't tell without my dev environment.

So unfortunately I can't test it until 01st Sept. Or you export CSL as target web for Ola to test it.

@Scitz0
Copy link

Scitz0 commented Aug 17, 2021

@refi93 Thanks for the help debugging this issue. With your help we have now identified that CSL wrongly adds the empty token map and I think we can close this issue. I have further identified what method in CSL that is the troublemaker but I will continue that discussion in the issue you opened in CSL repo.

@refi93 refi93 closed this as completed Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants