-
Notifications
You must be signed in to change notification settings - Fork 321
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
CIP-0021 Transformation Breaks Script Integrity #871
Comments
The problem is and will always be that cli does not produce transactions with data in canonical order. HW Wallets have a memory limitation and are therefore not capable of loading whole sets into the memory for checks. The data must be processed in a stream. So f.e. to find duplicated entries, canonical order for HW Wallets is mandatory. Cli refused to switch to canonical order years ago. So i wonder if this can be somehow taken into account for the redeemer issue you pointed out. |
No, it can't. The order of the data for datums and redeemers has internal meaning to smart contracts. There is no such thing as "canonical order" for datums and redeemers. If you change the order of these elements, you will change the behavior of the smart contracts. AFAIK this is what the I think the simple fix is to just not transform the datums and redeemers. The rest of the transaction can still be standardized. I don't see how this would negatively impact hardware wallet verification. |
@fallen-icarus @gitmachtl I think it will help to get some wallet dev visibility on this & have added (as we do with issues sometimes) to the next CIP meeting agenda (https://hackmd.io/@cip-editors/94). It seems pretty well documented here so far & I'm glad to see it progressing but I think in addition to raising the red flag at the meeting we also want to alert the affiliated Wallets Working Group about this (cc @Ryun1 @Crypto2099). |
This seems like a related issue. Perhaps the script data hash just needs to be recalculated after the transformation. I don't know enough typescript to really know what is happening in |
ok, but only the order of datums and redeemers, not the order of inputs and outputs? @refi93 @davidmisiak @janmazak can you check if hw-cli is reordering the redeemer/datum while running the transform command? |
Correct. The internal ordering of the datums and redeemers should be left alone as well. Again, though, I'm not sure what the transformation is actually doing. If it is actually a benign transformation (ie, it has no impact on what the smart contract actually does), it may just be the hash needs to be recalculated. EDIT: After looking back at the cddl, I think only the internal structure of datums and redeemers is important. The redeemer list can still be sorted, but the redeemers themselves cannot be touched. |
As was mentioned, the canonical encoding of some tx elements is essential for HW wallets security. The reason why CIP-21 requires canonical encoding of the entire tx is mostly practical I believe - usually, CBOR libraries expose only a boolean flag whether the entire structure should be encoded canonically or not. This is also the case of the The specific attributes that determine canonicality are listed here. I'm not experienced with smart contracts - can someone please reliably confirm or deny that a script can access this info? E.g.:
The possible outcomes are:
|
I think it is better to have smart contract compiler devs answer these questions. They'll know a lot more than me. I am personally using aiken so I will tag them. @rvcas @MicroProofs @KtorZ |
So cbor maps from datums and redeemers come in to smart contracts as a list of pairs based on user order. They are not reordered. So if you reordered a datum's map canonically that would make a tx that would fail in phase 1 since the hash of the cbor of the datum would not match the onchain cbor datum for a given input if that input was using a datum hash. There are likely other issues that would be present like invalid budget calculations after reordering maps. |
Also currently the onchain would allow the datum to have maps that have multiples of the same item. So best to leave the serialization of datums and redeemers alone. |
@davidmisiak What about a non-typescript executable/dependecy? Haskell has the cborg library which I believe does have a more granular API:
The serialise library is built upon the cborg library. I have not used either library, but perhaps they can be used to build a more granular transformation tool. |
@mpizenberg and I were investigating this issue some more and found the redeemer is changed from an indefinite-list to a definite-list as part of the transformation (in compliance with this CIP). This is the redeemer before:
and this is it after:
Plutus Data uses indefinite lists which is in violation of this CIP. I found this note discussing why indefinite-lists are sometimes used for plutus Data. I don't know if this is a situation where the script hash just needs to be re-calculated (and possibly budgets updated). |
The main issue seems to be that CIP21 specifies that lists should have definite lengths. But as far as I understand from my discussions with @KtorZ, the inside of plutus Data should be offlimit for CIP21 because there are specific strict rules about how plutus Data should be encoded. In particular lists in Data are encoded as definite length if 0 length, but indefinite length otherwise. |
The Script Data hash would need to be recalculated. |
Hum, it’s weird, I thought Plutus Data had very strict encoding rules, but I just tried to do the same lock-unlock 2ada example, first with indefinite length encoding of the redeemer Here is the second one: https://preview.cardanoscan.io/transaction/90706202b45cf9cfb45d402804c5c4015b1660745162e9c7e00f5651ef7748fc?tab=contracts So on that case, it seems recomputing the script data hash was sufficient, and it worked for both definite/indefinite lengths encoding of the redeemer Data for my hardware wallet ledger nano x. Is the LNX capable of signing non-CIP21 transactions? |
So for the sake of completeness, I just did another test, where I encoded the list of inputs with indefinite length. And this time I got an error from eternl saying |
I’m no Haskell expert but according to the plutus Data decoders here https://github.com/IntersectMBO/plutus/blob/b086464584f2684658633b375283994dbe04bef8/plutus-core/plutus-core/src/PlutusCore/Data.hs#L190 So at least the specific issue you were having @fallen-icarus are solvable by recomputing the script data hash. Now regarding other transformations like deduplication or re-ordering of elements accessible from a plutus contract. As Micro said, it will cause some issues. So if leaving the redeemers and datums unchanged is a possibility for hardware wallets, then that would be ideal. And in fact, the introduction of the CIP21 spec only mentions restrictions about the transaction body, because that’s the part which is signed by hardware wallets.
So my guess is the hardware wallets do not inspect the WitnessSet part of the transaction. And consequently, there is no good reason to transform that part of the transaction with CIP21 post-processing tools like cardano-hw-cli. |
Surely, the |
CIP-21 describes how hardware wallets require transactions to be formatted in order to be signed by them.
cardano-cli
does not produce transactions that satisfy this CIP so the tx.body file must be transformed after being created (docs). However, thecardano-hw-interop-lib
linked in this CIP actually breaks transactions using smart contracts with more complicated redeemers. I opened the corresponding github issue here. The flow is effectively:cardano-cli
.cardano-hw-cli transaction transform
which internally uses thecardano-hw-interop-lib
.If there are any smart contracts using complicated redeemers in the transaction, step 4 will always fail with a
ScriptIntegrity
error. It seems to be the transformation step that is breaking the transaction. You can even do the transformation on transactions being signed by CLI keys and you will still get theScriptIntegrity
submission error. This bug makes it impossible to usecardano-hw-cli
to sign these transactions since it requires all transactions to be transformed to satisfy this CIP. I'm not aware of other libraries that enable users to transform and then sign transactions with hardware wallets.I can't find any mention of redeemers in this CIP so I am now wondering if there is a piece missing from this CIP's Specification. Redeemers use CBOR as well, but the CBOR in the redeemer specifies which action is being taken with that script. Redeemers should NOT be forced to comply with this CIP. Altering the redeemers to "make the integers as small as possible" will actually change what that script will do! I think the
ScriptIntegrity
error is highlighting that the transformation is changing the redeemers since the integrity hash is a hash of the script + datum + redeemer used for a specific script execution (cddl spec). I think this CIP needs to be updated to explain what should happen with redeemers.(If the CIP editors do not think this is an appropriate topic for CIP Issues, feel free to close this issue.)
The text was updated successfully, but these errors were encountered: