use eip 191 standard for presigned transactions #15
Conversation
# Conflicts: # contracts/MainChain.sol # contracts/SideChain.sol # test/MainChainUnitTest/addBlackListUnitTest.js # test/MainChainUnitTest/removeBlackListUnitTest.js # test/MainChainUnitTest/submitTransactionUnitTest.js # test/SideChainUnitTest/submitSignatureMCUnit.js # test/utils/utils.js
contracts/SideChain.sol
Outdated
| /// @param destination destination provided in deposit tx in main chain | ||
| /// @param value msg.value of deposit tx in main chain | ||
| /// @param data data of deposit tx in main chain | ||
| function submitTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data) |
There was a problem hiding this comment.
why we remove msgHash from mainchian.sol but use it here?
There was a problem hiding this comment.
because we are not passing in v,r,s in this function, this standard is only for preSigned Transaction without msgHash here, we can't validate the parameters
contracts/SideChain.sol
Outdated
| if (sideChainTx[txHash].destination == address(0)) { | ||
| addTransactionSC(txHash, destination, value, data); | ||
| } else { |
There was a problem hiding this comment.
Why we want to remove these lines? Another user could sign the tx with different parameters than ones have been used in sideChainTx.
There was a problem hiding this comment.
they are not necessary because if sideChainTx[txHash].destination != destination then hashedTxParams would be different then msgHash, then line 96 will fail
There was a problem hiding this comment.
Could this a valid test case? There are two owners. Owner1 submitTransaction with value 1000, then the txHash has been added to sideChainTx. Then owner2 tried to submitTransaction with value 10000, it generates a new msgHash, and it will pass the check require(hashedTxParams == msgHash); We need the require(sideChainTx[txHash].value == value); to revert?
There was a problem hiding this comment.
yeah, i am thinking of a way to solve this, just having require(sideChainTx[txHash].value == value) is not enough because we can't do the same for data
I am thinking about either using msgHash as the mapping instead of txhash
or just storing msgHash as a parameter
There was a problem hiding this comment.
lets just store msgHash, since this is on the sidechain side, gas shouldn't be an issue
There was a problem hiding this comment.
hmm.... i think there is a DOS situation with only having submitTransactionSC, imagine a scenario where an hacker got one of the owner's account, they can submit bogus transaction with proper txHash, in that case then no of the other owners can do anything to fix that situation
For presigned transactions, ethereum/EIPs#191 has been implemented