From 2c182119c867499be9f860293640b6cd32459676 Mon Sep 17 00:00:00 2001 From: Shine Lee Date: Fri, 18 May 2018 14:37:05 -0700 Subject: [PATCH] use eip 191 standard for presigned transactions (#15) * implement standard from EIP 191 and fix tests * remove unused code * fix changes caused by bad rebase * merge conflicts and fix eslint and solium errors * remove settings not needed * store msgHash in the Transaction for parameter validation * fix eslint --- contracts/MainChain.sol | 33 +++---- contracts/SideChain.sol | 91 ++++++++++--------- contracts/libs/Freezable.sol | 8 +- contracts/libs/MultiSigOwnable.sol | 13 ++- .../MainChainUnitTest/addBlackListUnitTest.js | 10 +- .../removeBlackListUnitTest.js | 10 +- .../submitTransactionUnitTest.js | 64 ++++--------- .../submitSignatureMCUnit.js | 21 +---- test/utils/utils.js | 23 ++++- 9 files changed, 125 insertions(+), 148 deletions(-) diff --git a/contracts/MainChain.sol b/contracts/MainChain.sol index 96d716a..46a5d51 100644 --- a/contracts/MainChain.sol +++ b/contracts/MainChain.sol @@ -101,25 +101,23 @@ contract MainChain is Freezable { emit Deposit(msg.sender, to, msg.value); } - /// @dev submit a transaction to be processed - /// @param msgHash sha3 hash of txHash destination value data and VERSION - /// @param txHash transaction hash of the deposit tx in side chain - /// @param destination destination provided in deposit tx in side chain - /// @param value msg.value of deposit tx in side chain - /// @param data data of deposit tx in side chain - /// @param v list of v part of sig - /// @param r list of r part of sig - /// @param s list of s part of sig - function submitTransaction(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s) - notNull(destination) - transactionDoesNotExists(txHash) - txNotBlackListed(txHash) - onlyWhenNotFrozen - public { + /// @dev submit a transaction to be processed + /// @param txHash transaction hash of the deposit tx in side chain + /// @param destination destination provided in deposit tx in side chain + /// @param value msg.value of deposit tx in side chain + /// @param data data of deposit tx in side chain + /// @param v list of v part of sig + /// @param r list of r part of sig + /// @param s list of s part of sig + function submitTransaction(bytes32 txHash, address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s) + notNull(destination) + transactionDoesNotExists(txHash) + txNotBlackListed(txHash) + onlyWhenNotFrozen + public { require(v.length >= required); - bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION); - require(hashedTxParams == msgHash); + bytes32 msgHash = keccak256(byte(0x19), VERSION, address(this),txHash, destination, value, data); // execute the transaction after all checking the signatures require (hasEnoughRequiredSignatures(msgHash, v, r, s)); @@ -160,7 +158,6 @@ contract MainChain is Freezable { /// @param destination target destination /// @param value ether value /// @param data data payload - // @TODO still need to write tests for emergency withdrawal function emergencyWithdrawal(address destination, uint256 value, bytes data) onlyWhenFrozen ownerExists(msg.sender) diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index 24d60f7..3f19fe9 100644 --- a/contracts/SideChain.sol +++ b/contracts/SideChain.sol @@ -31,11 +31,11 @@ contract SideChain is Freezable { //////////////////////// // Storage Variables //////////////////////// - mapping (bytes32 => Transaction) public sideChainTx; + mapping (bytes32 => SideChainTransaction) public sideChainTx; mapping (bytes32 => mapping(address => bool)) public isSignedSC; uint256 sideChainTxCount; - mapping (bytes32 => Transaction) mainChainTxs; + mapping (bytes32 => MainChainTransaction) mainChainTxs; mapping (bytes32 => Signatures) mainChainSigs; mapping (bytes32 => mapping(address => bool)) public isSignedMC; uint256 mainChainTxCount; @@ -43,13 +43,21 @@ contract SideChain is Freezable { //////////////////////// // Structs //////////////////////// - struct Transaction { + struct SideChainTransaction { + bytes32 msgHash; address destination; uint256 value; bytes data; bool executed; } + + struct MainChainTransaction { + address destination; + uint256 value; + bytes data; + } + struct Signatures { uint8[] v; bytes32[] r; @@ -76,30 +84,26 @@ contract SideChain is Freezable { emit Deposit(msg.sender, to, msg.value); } - /// @dev submit transaction to be processed pending the approvals - /// @param msgHash sha3 hash of txHash destination value data and VERSION - /// @param txHash transaction hash of the deposit tx in main chain - /// @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 - /// @param v part of sig - /// @param r part of sig - /// @param s part of sig - function submitTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s) + /// @dev submit transaction to be processed pending the approvals + /// @param txHash transaction hash of the deposit tx in main chain + /// @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 txHash, address destination, uint256 value, bytes data) ownerExists(msg.sender) notNull(destination) public { require(!isSignedSC[txHash][msg.sender]); - address signer = ecrecover(msgHash, v, r, s); - require(msg.sender == signer); - bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION); - require(hashedTxParams == msgHash); + + bytes32 msgHash = keccak256(txHash, destination, value, data, VERSION); + if (sideChainTx[txHash].destination == address(0)) { - addTransactionSC(txHash, destination, value, data); - } else { - require(sideChainTx[txHash].destination == destination); - require(sideChainTx[txHash].value == value); + addTransactionSC(msgHash, txHash, destination, value, data); } + + // this validates that all the parameters are the same as the previous submitter + require(sideChainTx[txHash].msgHash == msgHash); + isSignedSC[txHash][msg.sender] = true; emit Confirmation(msg.sender, txHash); executeTransaction(txHash); @@ -124,7 +128,7 @@ contract SideChain is Freezable { require(isSignedSC[txHash][msg.sender]); require(!sideChainTx[txHash].executed); if (isConfirmed(txHash)) { - Transaction storage txn = sideChainTx[txHash]; + SideChainTransaction storage txn = sideChainTx[txHash]; txn.executed = true; if (external_call(txn.destination, txn.value, txn.data.length, txn.data)) emit Execution(txHash); @@ -142,7 +146,7 @@ contract SideChain is Freezable { public returns(bool) { uint count = 0; - for (uint i=0; i