-
Notifications
You must be signed in to change notification settings - Fork 566
fix: Some fields in transaction are not authenticated by signature #689
Conversation
Codecov Report
@@ Coverage Diff @@
## main #689 +/- ##
==========================================
+ Coverage 57.36% 57.41% +0.04%
==========================================
Files 63 63
Lines 5505 5570 +65
==========================================
+ Hits 3158 3198 +40
- Misses 2180 2199 +19
- Partials 167 173 +6
|
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.
maybe good to add some tests for the failure cases?
Good idea! |
Add more tests for failure cases where the tx.Tx gets signed, or has Memo, TimeoutHeight, or has invalid fee amount and gaslimit (not compatible with MsgEthereumTx). |
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.
Thanks for the PR, I'd like to see a separate test that compares the signatures from:
- MsgEthereumTx (v, r, s)
- sdk.Tx using the tx Builder
- ethereum Transaction
Ok, I will write a separate test comparing these signatures |
Closes: #426
Description
Some fields in transaction are not authenticated by signature, so there may be a cosmos transaction malleability vulnerability.
Since cosmos transaction is not signed, we can verify most of the fields of tx.Tx as null and verify fields like Fee.Amount and Fee.GasLimit based on MsgEtherumTx
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)