Message and Intents are now hashed as per EIP712 #566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Message, StakeIntent, and RedeemIntent now have a new structure and different fields that are part of the hash. Stake and redeem intent now consist of amount, beneficiary, and (originating) gateway address. The Message has as first parameter a hash which is not specific to staking or redeeming. All three pre-images now include a type hash as per EIP712. This means that I was able to remove a lot of stake/redeem specific code for message hashing. A message's hash is now generated regardless of the kind of intent it holds. I added a method `storeMessage` which creates a message hash and stores the message at the hash in the mapping. The reason was to circumvent a "stack too deep" error which prevented me from creating the hash directly in confirm stake/redeem intent. I restructured the tests in an effort to make them more easily understandable. I spent quite some time wrapping my head around the construct. I did not add the domain separator yet, as the hashes are never signed or the signature proven/verified. :warning: Please check that the new logic of how and what is hashed still guarantees correctness and prevents fraud when relaying the messages :warning: Fixes OpenST#415
| intentHash, | ||
| _stakerNonce, | ||
| _gasPrice, | ||
| _gasLimit | ||
| _gasLimit, | ||
| _staker, |
There was a problem hiding this comment.
a message now has a sender address?
correction: it already did. I'll review tomorrow as well, but don't wait for me as I'll be traveling.
deepesh-kn
reviewed
Dec 21, 2018
contracts/gateway/GatewayBase.sol
Outdated
| /** Maps messageHash to the Message object. */ | ||
| mapping(bytes32 => MessageBus.Message) messages; | ||
| mapping(bytes32 => MessageBus.Message) internal messages; |
There was a problem hiding this comment.
Can this be public? Any issue if it will be public?
In one of the other open PR, its changed to public
schemar
commented
Dec 21, 2018
0xsarvesh
approved these changes
Dec 21, 2018
contracts/lib/MockMessageBus.sol
Outdated
| @@ -670,7 +641,7 @@ library MockMessageBus { | |||
| { | |||
| messageHash_ = keccak256( | |||
| abi.encode( | |||
| _messageTypeHash, | |||
| // TODO: add type hash | |||
There was a problem hiding this comment.
😱 Fixed
However, I had to add a header to the MessageBus library. Any better ideas?
/cc @deepesh-kn
Co-Authored-By: schemar <martinschenck@fastmail.com>
…into eip712_hashing
schemar
commented
Dec 21, 2018
There was a problem hiding this comment.
Thanks I made some changes and added some comments.
- The
MockMessageBuswas missing the typehash as part of the message hash (/cc @deepesh-kn) - I am not certain about the
MessageBusmethodsconfirm...andprogress.... There are offsets and indexes that may be wrong (inbox/outbox confusion).
contracts/lib/MockMessageBus.sol
Outdated
| @@ -670,7 +641,7 @@ library MockMessageBus { | |||
| { | |||
| messageHash_ = keccak256( | |||
| abi.encode( | |||
| _messageTypeHash, | |||
| // TODO: add type hash | |||
There was a problem hiding this comment.
😱 Fixed
However, I had to add a header to the MessageBus library. Any better ideas?
/cc @deepesh-kn
…into eip712_hashing
* Updated MockMessageBus from MessageBus
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Message, StakeIntent, and RedeemIntent now have a new structure and
different fields that are part of the hash. Stake and redeem intent now
consist of amount, beneficiary, and (originating) gateway address. The
Message has as first parameter a hash which is not specific to staking
or redeeming.
All three pre-images now include a type hash as per EIP712.
This means that I was able to remove a lot of stake/redeem specific code
for message hashing. A message's hash is now generated regardless of the
kind of intent it holds.
I added a method
storeMessagewhich creates a message hash and storesthe message at the hash in the mapping. The reason was to circumvent a
"stack too deep" error which prevented me from creating the hash
directly in confirm stake/redeem intent.
I restructured the tests in an effort to make them more easily
understandable. I spent quite some time wrapping my head around the
construct.
I did not add the domain separator yet, as the hashes are never signed
or the signature proven/verified.
still guarantees correctness and prevents fraud when relaying the
messages
Fixes #415