-
Notifications
You must be signed in to change notification settings - Fork 952
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
Test malleability attacks #3925
base: main
Are you sure you want to change the base?
Conversation
d4e741b
to
39972d5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3925 +/- ##
==========================================
+ Coverage 73.88% 74.04% +0.16%
==========================================
Files 341 341
Lines 106444 106510 +66
==========================================
+ Hits 78647 78870 +223
+ Misses 27797 27640 -157 ☔ View full report in Codecov by Sentry. |
crates/sdk/src/lib.rs
Outdated
prop_compose! { | ||
/// Generate an arbitrary signed inner tx that has been tampered with. | ||
pub fn arb_tampered_inner_tx(signer: common::SecretKey) | ||
(tx1 in arb_valid_signed_inner_tx(signer.clone()))( | ||
tx2 in arb_valid_signed_inner_tx(signer.clone()), | ||
mut tx in Just(tx1), | ||
) -> Tx { | ||
// TODO: tamper with the sections too once signature is updated | ||
// Tamper with the header only for now | ||
tx.header = tx2.header; | ||
tx | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be interesting if we tampered with a tx section, particularly one that had meaningful tx data, such as a bond tx. the raw header hash would have changed, while the authorization stored in a section would have been for the previous hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact is that changing a section does not change its commitment (unless the attacker also does that): if this happens, than the signature will still be valid and the tx will fail later during execution because it's missing the correct section that the commitment is pointing to. I believe this scenario should have a different test than this one
39972d5
to
da127a4
Compare
Describe your changes
test_wrapper_unknown_address
Checklist before merging
breaking::
labelsnamada-docs
reponamada-indexer
ornamada-masp-indexer
, a corresponding PR is opened in that repo