From a84dea6bbf0e145bf4ad28db4ec25dbd4987af5e Mon Sep 17 00:00:00 2001 From: shine2lay Date: Thu, 17 May 2018 15:01:11 -0700 Subject: [PATCH 1/7] implement standard from EIP 191 and fix tests --- contracts/MainChain.sol | 7 +- contracts/SideChain.sol | 51 +++++++------- .../MainChainUnitTest/addBlackListUnitTest.js | 18 ++--- .../removeBlackListUnitTest.js | 18 ++--- .../submitTransactionUnitTest.js | 69 +++++++++---------- .../submitSignatureMCUnit.js | 36 +++++----- test/utils/utils.js | 15 ++-- 7 files changed, 111 insertions(+), 103 deletions(-) diff --git a/contracts/MainChain.sol b/contracts/MainChain.sol index 24e62da..e91cb74 100644 --- a/contracts/MainChain.sol +++ b/contracts/MainChain.sol @@ -18,6 +18,7 @@ contract MainChain is Freezable { event BlackListed(bytes32 indexed txHash); event UnBlackListed(bytes32 indexed txhash); event EmergencyWithdrawal(bytes32 hashedParam, address indexed destination, uint256 value, bytes data); + event test(bytes32 msgHash); ////////////////////// // Modifiers @@ -102,7 +103,6 @@ contract MainChain is Freezable { } /// @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 @@ -110,7 +110,7 @@ contract MainChain is Freezable { /// @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) + function submitTransaction(bytes32 txHash, address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s) notNull(destination) transactionDoesNotExists(txHash) txNotBlackListed(txHash) @@ -118,8 +118,7 @@ contract MainChain is Freezable { 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)); diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index a2688f1..cb68520 100644 --- a/contracts/SideChain.sol +++ b/contracts/SideChain.sol @@ -31,8 +31,8 @@ contract SideChain is Freezable { //////////////////////// // Storage Variables //////////////////////// - mapping (bytes32 => Transaction) public sideChainTx; - mapping (bytes32 => mapping(address => bool)) public isSignedSC; + mapping (bytes32 => Transaction) sideChainTx; + mapping (bytes32 => mapping(address => bool)) isSignedSC; uint256 sideChainTxCount; mapping (bytes32 => Transaction) mainChainTxs; @@ -77,32 +77,21 @@ contract SideChain is Freezable { } /// @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) - ownerExists(msg.sender) - notNull(destination) - public { + function submitTransactionSC(bytes32 txHash, address destination, uint256 value, bytes data) 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); - if (sideChainTx[txHash].destination == address(0)) { - addTransactionSC(txHash, destination, value, data); - } else { + + if(sideChainTx[txHash].destination != address(0)){ require(sideChainTx[txHash].destination == destination); require(sideChainTx[txHash].value == value); + } else { + addTransactionSC(txHash, destination, value, data); } + isSignedSC[txHash][msg.sender] = true; - emit Confirmation(msg.sender, txHash); - executeTransaction(txHash); } /// @dev Allows an owner to revoke a confirmation for a transaction. @@ -120,6 +109,7 @@ contract SideChain is Freezable { /// @dev Allows anyone to execute a confirmed transaction. /// @param txHash to be executed function executeTransaction(bytes32 txHash) + ownerExists(msg.sender) public { require(isSignedSC[txHash][msg.sender]); require(!sideChainTx[txHash].executed); @@ -202,6 +192,18 @@ contract SideChain is Freezable { sideChainTxCount += 1; } + /// @dev Allows an owner to confirm a transaction. + /// @param txHash to be confirmed + function confirmTransaction(bytes32 txHash) + ownerExists(msg.sender) + private { + require(!isSignedSC[txHash][msg.sender]); + require(sideChainTx[txHash].destination != 0); + isSignedSC[txHash][msg.sender] = true; + emit Confirmation(msg.sender, txHash); + executeTransaction(txHash); + } + // call has been separated into its own function in order to take advantage // of the Solidity's code generator to produce a loop that copies tx.data into memory. function external_call(address destination, uint value, uint dataLength, bytes data) private returns(bool) { @@ -229,7 +231,6 @@ contract SideChain is Freezable { ////////////////////////////////////////////// /// @dev store signature to be submitted in mainchain - /// @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 @@ -237,18 +238,17 @@ contract SideChain is Freezable { /// @param v part of sig /// @param r part of sig /// @param s part of sig - function submitSignatureMC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s) + function submitSignatureMC(bytes32 txHash, address destination, uint256 value, bytes data, uint8 v, bytes32 r, bytes32 s) ownerExists(msg.sender) notNull(destination) public { require(!isSignedMC[txHash][msg.sender]); + bytes32 msgHash = keccak256(byte(0x19), VERSION, address(this),txHash, destination, value, data); + address signer = ecrecover(msgHash, v, r, s); require(msg.sender == signer); - bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION); - require(hashedTxParams == msgHash); - if(mainChainTxs[txHash].destination == address(0)){ addTransactionMC(txHash, destination, value, data); } @@ -273,7 +273,8 @@ contract SideChain is Freezable { /// @param data Transaction data payload. function addTransactionMC(bytes32 txHash, address destination, uint256 value, bytes data) notNull(destination) - private { + private + { mainChainTxs[txHash] = Transaction({ destination: destination, value: value, diff --git a/test/MainChainUnitTest/addBlackListUnitTest.js b/test/MainChainUnitTest/addBlackListUnitTest.js index 38cea87..026c6c5 100644 --- a/test/MainChainUnitTest/addBlackListUnitTest.js +++ b/test/MainChainUnitTest/addBlackListUnitTest.js @@ -20,6 +20,7 @@ let toAddress; let value; let data; let version; +let contractAddress; contract('MainChain: addBlackList Unit Test', function(accounts) { beforeEach(async function () { @@ -30,6 +31,7 @@ contract('MainChain: addBlackList Unit Test', function(accounts) { value = 0; data = ''; version = await mainchainInstance.VERSION.call(); + contractAddress = mainchainInstance.address; }); it("checks that addBlackList work as intended if all the condition are valid", async function () { @@ -43,8 +45,8 @@ contract('MainChain: addBlackList Unit Test', function(accounts) { // check oulyByWallet const addBlackListData = mainchainInstance.contract.addBlackList.getData(txHash); - const sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, addBlackListData, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); + const sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, addBlackListData, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); // check txHash is added to blacklist successfully txBlackListed = await mainchainInstance.isBlackListed.call(txHash); @@ -69,16 +71,16 @@ contract('MainChain: addBlackList Unit Test', function(accounts) { assert.notOk(isFrozen); let addBlackListData = mainchainInstance.contract.addBlackList.getData(txHash); - let sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, addBlackListData, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); + let sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, addBlackListData, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); // check txHash is already blacklisted. let txBlackListed = await mainchainInstance.isBlackListed.call(txHash); assert.ok(txBlackListed); // const data = mainchainInstance.contract.addBlackList.getData(txHash); - sigs = utils.multipleSignedTransaction([0, 1], txHashes[1], toAddress, value, addBlackListData, version); - const res = await mainchainInstance.submitTransaction(sigs.msgHash, txHashes[1], toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHashes[1], toAddress, value, addBlackListData, version); + const res = await mainchainInstance.submitTransaction(txHashes[1], toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); // only 'Execucion' event is excuted, 'addBlackList' is reverted. assert.equal(res.logs.length, 1); @@ -86,8 +88,8 @@ contract('MainChain: addBlackList Unit Test', function(accounts) { it("checks that BlackListed event is emitted properly", async function () { const addBlackListData = mainchainInstance.contract.addBlackList.getData(txHash); - const sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, addBlackListData, version); - const res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); + const sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, addBlackListData, version); + const res = await mainchainInstance.submitTransaction(txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); assert.equal(res.logs[0].event, 'BlackListed'); }); diff --git a/test/MainChainUnitTest/removeBlackListUnitTest.js b/test/MainChainUnitTest/removeBlackListUnitTest.js index cb55dcb..3acea1c 100644 --- a/test/MainChainUnitTest/removeBlackListUnitTest.js +++ b/test/MainChainUnitTest/removeBlackListUnitTest.js @@ -20,6 +20,7 @@ let toAddress; let value; let data; let version; +let contractAddress; contract('MainChain: removeBlackList Unit Test', function(accounts) { beforeEach(async function () { @@ -30,9 +31,10 @@ contract('MainChain: removeBlackList Unit Test', function(accounts) { value = 0; data = ''; version = await mainchainInstance.VERSION.call(); + contractAddress = mainchainInstance.address; const addBlackListData = mainchainInstance.contract.addBlackList.getData(txHash); - const sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, addBlackListData, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); + const sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, addBlackListData, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, addBlackListData, sigs.v, sigs.r, sigs.s); }); it("checks that removeBlackList work as intended if all the condition are valid", async function () { @@ -46,8 +48,8 @@ contract('MainChain: removeBlackList Unit Test', function(accounts) { // check oulyByWallet const removeBlackListData = mainchainInstance.contract.removeBlackList.getData(txHash); - const removeBlackListSigs = utils.multipleSignedTransaction([0, 1], txHashes[1], toAddress, value, removeBlackListData, version); - await mainchainInstance.submitTransaction(removeBlackListSigs.msgHash, txHashes[1], toAddress, value, removeBlackListData, removeBlackListSigs.v, removeBlackListSigs.r, removeBlackListSigs.s, {from: accounts[0]}); + const removeBlackListSigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHashes[1], toAddress, value, removeBlackListData, version); + await mainchainInstance.submitTransaction(txHashes[1], toAddress, value, removeBlackListData, removeBlackListSigs.v, removeBlackListSigs.r, removeBlackListSigs.s, {from: accounts[0]}); // check txHash is removed from blacklist successfully txBlackListed = await mainchainInstance.isBlackListed.call(txHash); @@ -77,8 +79,8 @@ contract('MainChain: removeBlackList Unit Test', function(accounts) { assert.notOk(txBlackListed); const removeBlackListData = mainchainInstance.contract.removeBlackList.getData(txHashes[1]); - const removeBlackListSigs = utils.multipleSignedTransaction([0, 1], txHashes[1], toAddress, value, removeBlackListData, version); - const res = await mainchainInstance.submitTransaction(removeBlackListSigs.msgHash, txHashes[1], toAddress, value, removeBlackListData, removeBlackListSigs.v, removeBlackListSigs.r, removeBlackListSigs.s); + const removeBlackListSigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHashes[1], toAddress, value, removeBlackListData, version); + const res = await mainchainInstance.submitTransaction(txHashes[1], toAddress, value, removeBlackListData, removeBlackListSigs.v, removeBlackListSigs.r, removeBlackListSigs.s); // only 'Execucion' event is excuted, 'removeBlackList' is reverted. assert.equal(res.logs.length, 1); @@ -86,8 +88,8 @@ contract('MainChain: removeBlackList Unit Test', function(accounts) { it("checks that UnBlackListed event is emitted properly", async function () { const removeBlackListData = mainchainInstance.contract.removeBlackList.getData(txHash); - const sigs1 = utils.multipleSignedTransaction([0, 1], txHashes[1], toAddress, value, removeBlackListData, version); - const res = await mainchainInstance.submitTransaction(sigs1.msgHash, txHashes[1], toAddress, value, removeBlackListData, sigs1.v, sigs1.r, sigs1.s); + const sigs1 = utils.multipleSignedTransaction([0, 1], contractAddress, txHashes[1], toAddress, value, removeBlackListData, version); + const res = await mainchainInstance.submitTransaction(txHashes[1], toAddress, value, removeBlackListData, sigs1.v, sigs1.r, sigs1.s); assert.equal(res.logs[0].event, 'UnBlackListed'); }); diff --git a/test/MainChainUnitTest/submitTransactionUnitTest.js b/test/MainChainUnitTest/submitTransactionUnitTest.js index 46bf5b0..3e3d5d1 100644 --- a/test/MainChainUnitTest/submitTransactionUnitTest.js +++ b/test/MainChainUnitTest/submitTransactionUnitTest.js @@ -23,6 +23,7 @@ let value; let data; let sigs; let version; +let contractAddress; contract('MainChain: submitTransaction Unit Test', function(accounts) { @@ -37,6 +38,7 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { value = 1e6; data = ''; version = await mainchainInstance.VERSION.call(); + contractAddress = mainchainInstance.address; }); it("checks that ETH withdrawal works as intended if all the condition are valid", async function () { @@ -45,8 +47,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { const contractBalanceBefore = web3.eth.getBalance(mainchainInstance.address); toAddress = user; - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - const res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s, {from: accounts[0], gas: 1e6}); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + const res = await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s, {from: accounts[0], gas: 1e6}); const userBalanceAfter = web3.eth.getBalance(user); const contractBalanceAfter = web3.eth.getBalance(mainchainInstance.address); @@ -71,8 +73,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { const userBalanceBefore = await exampleTokenInstance.balanceOf.call(user); const contractBalanceBefore = await exampleTokenInstance.balanceOf.call(mainchainInstance.address); - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); const userBalanceAfter = await exampleTokenInstance.balanceOf.call(user); const contractBalanceAfter = await exampleTokenInstance.balanceOf.call(mainchainInstance.address); @@ -90,8 +92,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { data = mainchainInstance.contract.addOwner.getData(accounts[9]); toAddress = mainchainInstance.address; value = 0; - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - let res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + let res = await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); let log = res.logs[0]; assert.equal(log.event, 'OwnerAddition'); @@ -103,8 +105,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { // Try to remove the Added Owner data = mainchainInstance.contract.removeOwner.getData(accounts[9]); txHash = txHashes[1]; // we need to use a different txHash for this to succeed - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + res = await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); log = res.logs[0]; assert.equal(log.event, 'OwnerRemoval'); @@ -124,8 +126,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { assert.equal(transaction[2], '0x'); - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); transaction = await mainchainInstance.transactions.call(txHash); @@ -136,17 +138,17 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { }); it("revert if transaction Hash already exits", async function () { - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); - await utils.assertRevert(mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); + await utils.assertRevert(mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); }); it("revert if transaction Hash is blackListed", async function () { // add txHash to blackList. const txHashToBlackList = txHashes[1]; const addBlackListData = mainchainInstance.contract.addBlackList.getData(txHash); - const addBlackListSigs = utils.multipleSignedTransaction([0, 1], txHashToBlackList, mainchainInstance.contract.address, 0, addBlackListData, version); - await mainchainInstance.submitTransaction(addBlackListSigs.msgHash, txHashToBlackList, mainchainInstance.contract.address, 0, addBlackListData, addBlackListSigs.v, addBlackListSigs.r, addBlackListSigs.s); + const addBlackListSigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHashToBlackList, mainchainInstance.contract.address, 0, addBlackListData, version); + await mainchainInstance.submitTransaction(txHashToBlackList, mainchainInstance.contract.address, 0, addBlackListData, addBlackListSigs.v, addBlackListSigs.r, addBlackListSigs.s); let istxBlackListed = await mainchainInstance.isBlackListed.call(txHash); assert.ok(istxBlackListed); @@ -155,10 +157,10 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { const userBalanceBefore = web3.eth.getBalance(user); const contractBalanceBefore = web3.eth.getBalance(mainchainInstance.address); toAddress = user; - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); // check transaction is reverted. - utils.assertRevert(mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s, {from: accounts[0], gas: 1e6})); + utils.assertRevert(mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s, {from: accounts[0], gas: 1e6})); const userBalanceAfter = web3.eth.getBalance(user); const contractBalanceAfter = web3.eth.getBalance(mainchainInstance.address); @@ -175,39 +177,32 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { isFrozen = await mainchainInstance.checkIfFrozen.call(); assert.ok(isFrozen); - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await utils.assertRevert(mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + await utils.assertRevert(mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); }); it("revert if number of signatures are less than required", async function () { // one less signature than required - sigs = utils.multipleSignedTransaction([0], txHash, toAddress, value, data, version); - await utils.assertRevert(mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); + sigs = utils.multipleSignedTransaction([0], contractAddress, txHash, toAddress, value, data, version); + await utils.assertRevert(mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); }); - it("revert if the hash of the parameter doesn't equal to msgHash signed", async function () { - // use a different txHash than the one signed with - const msgHash = utils.createMsgHash(txHashes[1], toAddress, value, data, version); - - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await utils.assertRevert(mainchainInstance.submitTransaction(msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); - }); it("revert if there aren't enough valid signatures", async function () { // two of the same signature - sigs = utils.multipleSignedTransaction([0, 0], txHash, toAddress, value, data, version); - await utils.assertRevert(mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); + sigs = utils.multipleSignedTransaction([0, 0], contractAddress, txHash, toAddress, value, data, version); + await utils.assertRevert(mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s)); - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); }); it("checks that Execution event is emitted properly", async function () { - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - const res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + const res = await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); const log = res.logs[0]; assert.equal(log.event, 'Execution'); @@ -216,8 +211,8 @@ contract('MainChain: submitTransaction Unit Test', function(accounts) { it("checks that ExecutionFailure event is emitted properly", async function () { value = 1e8; // try to withdraw more than the contract holds - sigs = utils.multipleSignedTransaction([0, 1], txHash, toAddress, value, data, version); - const res = await mainchainInstance.submitTransaction(sigs.msgHash, txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); + sigs = utils.multipleSignedTransaction([0, 1], contractAddress, txHash, toAddress, value, data, version); + const res = await mainchainInstance.submitTransaction(txHash, toAddress, value, data, sigs.v, sigs.r, sigs.s); const log = res.logs[0]; assert.equal(log.event, 'ExecutionFailure'); diff --git a/test/SideChainUnitTest/submitSignatureMCUnit.js b/test/SideChainUnitTest/submitSignatureMCUnit.js index 7d4a3c2..092877a 100644 --- a/test/SideChainUnitTest/submitSignatureMCUnit.js +++ b/test/SideChainUnitTest/submitSignatureMCUnit.js @@ -23,6 +23,7 @@ let value; let data; let sigs; let version; +let contractAddress; contract('SideChain: submitSignature Unit Test', function(accounts) { @@ -35,7 +36,8 @@ contract('SideChain: submitSignature Unit Test', function(accounts) { value = 1e6; data = ''; version = await sidechainInstance.VERSION.call(); - sigs = utils.multipleSignedTransaction([0,1,2,3,4,5,6], txHash, toAddress, value, data, version); + contractAddress = sidechainInstance.address; + sigs = utils.multipleSignedTransaction([0,1,2,3,4,5,6], contractAddress, txHash, toAddress, value, data, version); }); it("checks that a new transaction object is added for first signature submission", async function () { @@ -46,7 +48,7 @@ contract('SideChain: submitSignature Unit Test', function(accounts) { assert.equal(destination, consts.ZERO_ADDRESS); // we only need to submit one signature - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); tx = await sidechainInstance.getTransactionMC.call(txHash); destination = tx[0]; // first index is the destination address; @@ -65,14 +67,14 @@ contract('SideChain: submitSignature Unit Test', function(accounts) { }); it("checks that signature is added properly to existing transaction object for non-first signature submission", async function () { - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); let tx = await sidechainInstance.getTransactionMC.call(txHash); let v = tx[3]; // there should be one signature in the list assert.equal(v.length, 1); - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[1], sigs.r[1], sigs.s[1], {from: accounts[1]}); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[1], sigs.r[1], sigs.s[1], {from: accounts[1]}); tx = await sidechainInstance.getTransactionMC.call(txHash); // we don't check v here because the value of v is always either 27 or 28 @@ -87,34 +89,34 @@ contract('SideChain: submitSignature Unit Test', function(accounts) { // should revert because it is calling from non owner account const nonSigner = accounts[1]; const signer = accounts[0]; - await utils.assertRevert(sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: nonSigner})); + await utils.assertRevert(sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: nonSigner})); - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: signer}); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: signer}); }); it("revert if called from a non owner account", async function () { // should revert because it is calling from non owner account const nonOwner = accounts[9]; - await utils.assertRevert(sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: nonOwner})); + await utils.assertRevert(sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: nonOwner})); const owner = accounts[0]; - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: owner}) + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0], {from: owner}) }); it("revert if destination address is null", async function () { toAddress = consts.ZERO_ADDRESS; - sigs = utils.multipleSignedTransaction([0], txHash, toAddress, value, data, version); + sigs = utils.multipleSignedTransaction([0], contractAddress, txHash, toAddress, value, data, version); - await utils.assertRevert(sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); + await utils.assertRevert(sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); // test with valid conditions to see if it passes toAddress = accounts[1]; - sigs = utils.multipleSignedTransaction([0], txHash, toAddress, value, data, version); - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + sigs = utils.multipleSignedTransaction([0], contractAddress, txHash, toAddress, value, data, version); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); }); it("revert if owner already submitted a signature", async function () { - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); //checks that signature submission works const tx = await sidechainInstance.getTransactionMC.call(txHash); @@ -127,19 +129,19 @@ contract('SideChain: submitSignature Unit Test', function(accounts) { assert.equal(s, sigs.s[0]); // try to submit the same signature again - await utils.assertRevert(sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); + await utils.assertRevert(sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); }); it("revert if hash of Params is different from msgHash", async function () { toAddress = accounts[2]; // a different account than one used to create msgHash - await utils.assertRevert(sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); + await utils.assertRevert(sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0])); toAddress = accounts[1]; // try to make sure it works with proper value - await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); }); it("checks that signatureAdded event is emitted properly", async function () { - const res = await sidechainInstance.submitSignatureMC(sigs.msgHash, txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); + const res = await sidechainInstance.submitSignatureMC(txHash, toAddress, value, data, sigs.v[0], sigs.r[0], sigs.s[0]); const log = res.logs[0]; assert.equal(log.event, 'SignatureAdded'); diff --git a/test/utils/utils.js b/test/utils/utils.js index 8c6cb1c..2d9ecb2 100644 --- a/test/utils/utils.js +++ b/test/utils/utils.js @@ -66,11 +66,18 @@ module.exports = { }); }, - createMsgHash: function(txHash, toAddress, value, data, version) { + createMsgHash: function(contractAddress, txHash, toAddress, value, data, version) { const hexEncodedData = this.checkAndEncodeHexPrefix(data); - return web3Uitl.soliditySha3({ t: 'bytes32', v: txHash}, {t: 'address', v: toAddress}, value, { t: 'bytes', v: hexEncodedData}, { t:'uint8', v: version }).substring(2); + return web3Uitl.soliditySha3( + { t: 'bytes', v: '0x19'}, + { t: 'uint8', v: version}, + {t: 'address', v: contractAddress}, + { t: 'bytes32', v: txHash}, + {t: 'address', v: toAddress}, + value, + { t: 'bytes', v: hexEncodedData}).substring(2); }, checkAndEncodeHexPrefix: function(toHex) { @@ -93,12 +100,12 @@ module.exports = { return { msgHash: '0x' + msgHash, v: sig.v, r :'0x' + sig.r.toString('hex'), s: '0x' + sig.s.toString('hex')}; }, - multipleSignedTransaction: function(arryOfUserIndexes, txHash, toAddress, value, data, version) { + multipleSignedTransaction: function(arryOfUserIndexes, contractAddress, txHash, toAddress, value, data, version) { const v = []; const r = []; const s = []; - const msgHash = this.createMsgHash(txHash, toAddress, value, data, version); + const msgHash = this.createMsgHash(contractAddress, txHash, toAddress, value, data, version); for(let i = 0; i < arryOfUserIndexes.length; i++) { const sig = ethUtils.ecsign(Buffer.from(msgHash, 'hex'), Buffer.from(consts.PRIVATE_KEYS[arryOfUserIndexes[i]], 'hex')); From e9b8c0fe66c0fe58d6c1d3f46f69b716bf3fb112 Mon Sep 17 00:00:00 2001 From: shine2lay Date: Thu, 17 May 2018 16:07:55 -0700 Subject: [PATCH 2/7] remove unused code --- contracts/MainChain.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/MainChain.sol b/contracts/MainChain.sol index e91cb74..d7241d1 100644 --- a/contracts/MainChain.sol +++ b/contracts/MainChain.sol @@ -18,7 +18,6 @@ contract MainChain is Freezable { event BlackListed(bytes32 indexed txHash); event UnBlackListed(bytes32 indexed txhash); event EmergencyWithdrawal(bytes32 hashedParam, address indexed destination, uint256 value, bytes data); - event test(bytes32 msgHash); ////////////////////// // Modifiers From 4548956c3fcca7f1fff5b8f2c679093f0b0aadbf Mon Sep 17 00:00:00 2001 From: shine2lay Date: Thu, 17 May 2018 16:18:13 -0700 Subject: [PATCH 3/7] fix changes caused by bad rebase --- contracts/SideChain.sol | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index cb68520..1d0ba80 100644 --- a/contracts/SideChain.sol +++ b/contracts/SideChain.sol @@ -31,8 +31,8 @@ contract SideChain is Freezable { //////////////////////// // Storage Variables //////////////////////// - mapping (bytes32 => Transaction) sideChainTx; - mapping (bytes32 => mapping(address => bool)) isSignedSC; + mapping (bytes32 => Transaction) public sideChainTx; + mapping (bytes32 => mapping(address => bool)) public isSignedSC; uint256 sideChainTxCount; mapping (bytes32 => Transaction) mainChainTxs; @@ -81,17 +81,22 @@ contract SideChain is Freezable { /// @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) public { + function submitTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint256 value, bytes data) + ownerExists(msg.sender) + notNull(destination) + public { require(!isSignedSC[txHash][msg.sender]); - if(sideChainTx[txHash].destination != address(0)){ - require(sideChainTx[txHash].destination == destination); - require(sideChainTx[txHash].value == value); - } else { + bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION); + require(hashedTxParams == msgHash); + + if (sideChainTx[txHash].destination == address(0)) { addTransactionSC(txHash, destination, value, data); } isSignedSC[txHash][msg.sender] = true; + emit Confirmation(msg.sender, txHash); + executeTransaction(txHash); } /// @dev Allows an owner to revoke a confirmation for a transaction. @@ -109,7 +114,6 @@ contract SideChain is Freezable { /// @dev Allows anyone to execute a confirmed transaction. /// @param txHash to be executed function executeTransaction(bytes32 txHash) - ownerExists(msg.sender) public { require(isSignedSC[txHash][msg.sender]); require(!sideChainTx[txHash].executed); @@ -192,18 +196,6 @@ contract SideChain is Freezable { sideChainTxCount += 1; } - /// @dev Allows an owner to confirm a transaction. - /// @param txHash to be confirmed - function confirmTransaction(bytes32 txHash) - ownerExists(msg.sender) - private { - require(!isSignedSC[txHash][msg.sender]); - require(sideChainTx[txHash].destination != 0); - isSignedSC[txHash][msg.sender] = true; - emit Confirmation(msg.sender, txHash); - executeTransaction(txHash); - } - // call has been separated into its own function in order to take advantage // of the Solidity's code generator to produce a loop that copies tx.data into memory. function external_call(address destination, uint value, uint dataLength, bytes data) private returns(bool) { @@ -273,8 +265,7 @@ contract SideChain is Freezable { /// @param data Transaction data payload. function addTransactionMC(bytes32 txHash, address destination, uint256 value, bytes data) notNull(destination) - private - { + private { mainChainTxs[txHash] = Transaction({ destination: destination, value: value, From 0b7d921030c53d1a3ab752df553aeaf37081637b Mon Sep 17 00:00:00 2001 From: shine2lay Date: Fri, 18 May 2018 11:42:51 -0700 Subject: [PATCH 4/7] merge conflicts and fix eslint and solium errors --- .solcover.js | 1 + contracts/MainChain.sol | 493 ++++++++-------- contracts/SideChain.sol | 538 +++++++++--------- contracts/libs/Freezable.sol | 8 +- contracts/libs/MultiSigOwnable.sol | 13 +- .../MainChainUnitTest/addBlackListUnitTest.js | 111 +++- .../removeBlackListUnitTest.js | 116 +++- .../submitTransactionUnitTest.js | 389 +++++++++++-- .../submitSignatureMCUnit.js | 219 +++++-- test/utils/utils.js | 169 ++++-- 10 files changed, 1337 insertions(+), 720 deletions(-) diff --git a/.solcover.js b/.solcover.js index 7ced80b..5065393 100644 --- a/.solcover.js +++ b/.solcover.js @@ -1,4 +1,5 @@ module.exports = { + norpc: true, port: 8555, testrpcOptions: '--port 8555 -m \"candy maple cake sugar pudding cream honey rich smooth crumble sweet treat\"', skipFiles: [ diff --git a/contracts/MainChain.sol b/contracts/MainChain.sol index d7241d1..46a5d51 100644 --- a/contracts/MainChain.sol +++ b/contracts/MainChain.sol @@ -1,105 +1,105 @@ pragma solidity ^0.4.21; -import './libs/MultiSigOwnable.sol'; -import './libs/Freezable.sol'; +import "./libs/MultiSigOwnable.sol"; +import "./libs/Freezable.sol"; contract MainChain is Freezable { - ////////////////////// - // CONSTANTS - ///////////////////// - uint8 public constant VERSION = 1; - - ////////////////////// - // EVENTS - ///////////////////// - event Execution(bytes32 indexed txHash); - event ExecutionFailure(bytes32 indexed txHash); - event Deposit(address indexed sender, address indexed to, uint value); - event BlackListed(bytes32 indexed txHash); - event UnBlackListed(bytes32 indexed txhash); - event EmergencyWithdrawal(bytes32 hashedParam, address indexed destination, uint256 value, bytes data); - - ////////////////////// - // Modifiers - ///////////////////// - modifier transactionDoesNotExists(bytes32 txHash) { - require(transactions[txHash].destination == 0); - _; - } - - modifier notNull(address _address) { - require(_address != 0); - _; - } - - modifier txNotBlackListed(bytes32 txHash) { - require(!isBlackListed[txHash]); - _; - } - - modifier txBlackListed(bytes32 txHash) { - require(isBlackListed[txHash]); - _; - } - - modifier onlyWhenNotFrozen() { - require(!checkIfFrozen()); - _; - } - - modifier onlyWhenFrozen() { - require(checkIfFrozen()); - _; - } - - //////////////////////// - // Storage Variables - //////////////////////// - mapping (bytes32 => Transaction) public transactions; - mapping (bytes32 => EmergencyTx) public emergencyTxs; - mapping (address => mapping(address => bool)) public emergencyTxConfirmation; - mapping (bytes32 => bool) public isBlackListed; - uint256 public transactionCount; - - //////////////////////// - // Structs - //////////////////////// - struct Transaction { - address destination; - uint256 value; - bytes data; - } - - struct EmergencyTx { - address destination; - uint256 value; - bytes data; - bool executed; - address[] confirmations; - mapping(address => bool) isConfirmed; - } - - - - ///////////////////////// - // Public Functions - //////////////////////// - - /// @dev Contract constructor sets initial owners and required number of confirmations. - /// @param _owners List of initial owners. - /// @param _required Number of required confirmations. - function MainChain(address[] _owners, uint8 _required) Freezable(_owners, _required) public {} - - /// @dev Allows to deposit ETH into the contract - /// @param to the destination address where user wish to received the funds on sidechain - function deposit(address to) - onlyWhenNotFrozen - notNull(to) - payable - public { - require(msg.value > 0); - emit Deposit(msg.sender, to, msg.value); - } + ////////////////////// + // CONSTANTS + ///////////////////// + uint8 public constant VERSION = 1; + + ////////////////////// + // EVENTS + ///////////////////// + event Execution(bytes32 indexed txHash); + event ExecutionFailure(bytes32 indexed txHash); + event Deposit(address indexed sender, address indexed to, uint value); + event BlackListed(bytes32 indexed txHash); + event UnBlackListed(bytes32 indexed txhash); + event EmergencyWithdrawal(bytes32 hashedParam, address indexed destination, uint256 value, bytes data); + + ////////////////////// + // Modifiers + ///////////////////// + modifier transactionDoesNotExists(bytes32 txHash) { + require(transactions[txHash].destination == 0); + _; + } + + modifier notNull(address _address) { + require(_address != 0); + _; + } + + modifier txNotBlackListed(bytes32 txHash) { + require(!isBlackListed[txHash]); + _; + } + + modifier txBlackListed(bytes32 txHash) { + require(isBlackListed[txHash]); + _; + } + + modifier onlyWhenNotFrozen() { + require(!checkIfFrozen()); + _; + } + + modifier onlyWhenFrozen() { + require(checkIfFrozen()); + _; + } + + //////////////////////// + // Storage Variables + //////////////////////// + mapping (bytes32 => Transaction) public transactions; + mapping (bytes32 => EmergencyTx) public emergencyTxs; + mapping (address => mapping(address => bool)) public emergencyTxConfirmation; + mapping (bytes32 => bool) public isBlackListed; + uint256 public transactionCount; + + //////////////////////// + // Structs + //////////////////////// + struct Transaction { + address destination; + uint256 value; + bytes data; + } + + struct EmergencyTx { + address destination; + uint256 value; + bytes data; + bool executed; + address[] confirmations; + mapping(address => bool) isConfirmed; + } + + + + ///////////////////////// + // Public Functions + //////////////////////// + + /// @dev Contract constructor sets initial owners and required number of confirmations. + /// @param _owners List of initial owners. + /// @param _required Number of required confirmations. + function MainChain(address[] _owners, uint8 _required) Freezable(_owners, _required) public {} + + /// @dev Allows to deposit ETH into the contract + /// @param to the destination address where user wish to received the funds on sidechain + function deposit(address to) + onlyWhenNotFrozen + notNull(to) + payable + public { + require(msg.value > 0); + emit Deposit(msg.sender, to, msg.value); + } /// @dev submit a transaction to be processed /// @param txHash transaction hash of the deposit tx in side chain @@ -109,158 +109,157 @@ contract MainChain is Freezable { /// @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) + 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 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)); - - if (external_call(destination, value, data.length, data)) { - addTransaction(txHash, destination, value, data); - emit Execution(txHash); - } else { - emit ExecutionFailure(txHash); - } - } - - /// @dev Allow to add a txHash to blacklist which will prevent a submitTransaction with txHash to fail - /// @param txHash to be blackListed - function addBlackList(bytes32 txHash) - onlyByWallet - onlyWhenNotFrozen - txNotBlackListed(txHash) - public { - isBlackListed[txHash] = true; - - emit BlackListed(txHash); - } - - /// @dev Remove a particular txHash from the blacklist - /// @param txHash to be removed from blackListed - function removeBlackList(bytes32 txHash) - onlyByWallet - onlyWhenNotFrozen - txBlackListed(txHash) - public { - isBlackListed[txHash] = false; - - emit UnBlackListed(txHash); - } - - /// @dev Allows owner to move funds when frozen - /// @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) - public { - bytes32 hashedParam = keccak256(destination, value, data); - - EmergencyTx storage transaction = emergencyTxs[hashedParam]; - - // store the param for audit sake - if (transaction.destination == address(0)) { - transaction.destination = destination; - transaction.value = value; - transaction.data = data; - } - - require(!transaction.executed); // make sure the transaction is not executed - require(!transaction.isConfirmed[msg.sender]); // make sure the msg.sender is in the confirmed list - - transaction.isConfirmed[msg.sender] = true; - transaction.confirmations.push(msg.sender); - - if (transaction.confirmations.length >= required) { - if (external_call(destination, value, data.length, data)) { - transaction.executed = true; - emit EmergencyWithdrawal(hashedParam, destination, value, data); - } else { - emit ExecutionFailure(hashedParam); - } - } - } - - - ///////////////////////// - // Private Functions - //////////////////////// - - /// @dev checks all the signatures and make sure they are from the owners - /// @param msgHash messageHash that signers singed - /// @param v , r, s signatures - /// @return boolean - function hasEnoughRequiredSignatures(bytes32 msgHash, uint8[] v, bytes32[] r, bytes32[] s) view private returns(bool){ - address[] memory confirmed = new address[](v.length); - uint8 confirmedCount; - - for (uint8 i = 0; i < v.length; i++) { - address result = ecrecover(msgHash, v[i], r[i], s[i]); - - if (!duplicateExists(confirmed, result)) { - confirmed[i] = result; - confirmedCount++; - } - - if (confirmedCount >= required) return true; - } - return false; - } - - /// @dev check that no duplicate exists - /// @param list array to be checked - /// @param toCheck element to check for - function duplicateExists(address[] list, address toCheck) pure private returns(bool) { - for (uint8 i = 0; i < list.length; i++) { - if (list[i] == toCheck) return true; - } - return false; - } - - // call has been separated into its own function in order to take advantage - // of the Solidity's code generator to produce a loop that copies tx.data into memory. - function external_call(address destination, uint value, uint dataLength, bytes data) private returns(bool) { - bool result; - assembly { - let x := mload(0x40) // "Allocate" memory for output (0x40 is where "free memory" pointer is stored by convention) - let d := add(data, 32) // First 32 bytes are the padded length of data, so exclude that - result := call( - sub(gas, 34710), // 34710 is the value that solidity is currently emitting - // It includes callGas (700) + callVeryLow (3, to pay for SUB) + callValueTransferGas (9000) + - // callNewAccountGas (25000, in case the destination address does not exist and needs creating) - destination, - value, - d, - dataLength, // Size of the input (in bytes) - this is what fixes the padding problem - x, - 0 // Output is ignored, therefore the output size is zero - ) - } - return result; - } - - /// @dev Adds a new transaction to the transaction mapping, if transaction does not exist yet. - /// @param txHash Transaction transactionHash - /// @param destination Transaction target address. - /// @param value Transaction ether value. - /// @param data Transaction data payload. - function addTransaction(bytes32 txHash, address destination, uint value, bytes data) - notNull(destination) - private { - transactions[txHash] = Transaction({ - destination: destination, - value: value, - data: data - }); - transactionCount += 1; - } + require(v.length >= required); + + 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)); + + if (external_call(destination, value, data.length, data)) { + addTransaction(txHash, destination, value, data); + emit Execution(txHash); + } else { + emit ExecutionFailure(txHash); + } + } + + /// @dev Allow to add a txHash to blacklist which will prevent a submitTransaction with txHash to fail + /// @param txHash to be blackListed + function addBlackList(bytes32 txHash) + onlyByWallet + onlyWhenNotFrozen + txNotBlackListed(txHash) + public { + isBlackListed[txHash] = true; + + emit BlackListed(txHash); + } + + /// @dev Remove a particular txHash from the blacklist + /// @param txHash to be removed from blackListed + function removeBlackList(bytes32 txHash) + onlyByWallet + onlyWhenNotFrozen + txBlackListed(txHash) + public { + isBlackListed[txHash] = false; + + emit UnBlackListed(txHash); + } + + /// @dev Allows owner to move funds when frozen + /// @param destination target destination + /// @param value ether value + /// @param data data payload + function emergencyWithdrawal(address destination, uint256 value, bytes data) + onlyWhenFrozen + ownerExists(msg.sender) + public { + bytes32 hashedParam = keccak256(destination, value, data); + + EmergencyTx storage transaction = emergencyTxs[hashedParam]; + + // store the param for audit sake + if (transaction.destination == address(0)) { + transaction.destination = destination; + transaction.value = value; + transaction.data = data; + } + + require(!transaction.executed); // make sure the transaction is not executed + require(!transaction.isConfirmed[msg.sender]); // make sure the msg.sender is in the confirmed list + + transaction.isConfirmed[msg.sender] = true; + transaction.confirmations.push(msg.sender); + + if (transaction.confirmations.length >= required) { + if (external_call(destination, value, data.length, data)) { + transaction.executed = true; + emit EmergencyWithdrawal(hashedParam, destination, value, data); + } else { + emit ExecutionFailure(hashedParam); + } + } + } + + + ///////////////////////// + // Private Functions + //////////////////////// + + /// @dev checks all the signatures and make sure they are from the owners + /// @param msgHash messageHash that signers singed + /// @param v , r, s signatures + /// @return boolean + function hasEnoughRequiredSignatures(bytes32 msgHash, uint8[] v, bytes32[] r, bytes32[] s) view private returns(bool){ + address[] memory confirmed = new address[](v.length); + uint8 confirmedCount; + + for (uint8 i = 0; i < v.length; i++) { + address result = ecrecover(msgHash, v[i], r[i], s[i]); + + if (!duplicateExists(confirmed, result)) { + confirmed[i] = result; + confirmedCount++; + } + + if (confirmedCount >= required) return true; + } + return false; + } + + /// @dev check that no duplicate exists + /// @param list array to be checked + /// @param toCheck element to check for + function duplicateExists(address[] list, address toCheck) pure private returns(bool) { + for (uint8 i = 0; i < list.length; i++) { + if (list[i] == toCheck) return true; + } + return false; + } + + // call has been separated into its own function in order to take advantage + // of the Solidity's code generator to produce a loop that copies tx.data into memory. + function external_call(address destination, uint value, uint dataLength, bytes data) private returns(bool) { + bool result; + assembly { + let x := mload(0x40) // "Allocate" memory for output (0x40 is where "free memory" pointer is stored by convention) + let d := add(data, 32) // First 32 bytes are the padded length of data, so exclude that + result := call( + sub(gas, 34710), // 34710 is the value that solidity is currently emitting + // It includes callGas (700) + callVeryLow (3, to pay for SUB) + callValueTransferGas (9000) + + // callNewAccountGas (25000, in case the destination address does not exist and needs creating) + destination, + value, + d, + dataLength, // Size of the input (in bytes) - this is what fixes the padding problem + x, + 0 // Output is ignored, therefore the output size is zero + ) + } + return result; + } + + /// @dev Adds a new transaction to the transaction mapping, if transaction does not exist yet. + /// @param txHash Transaction transactionHash + /// @param destination Transaction target address. + /// @param value Transaction ether value. + /// @param data Transaction data payload. + function addTransaction(bytes32 txHash, address destination, uint value, bytes data) + notNull(destination) + private { + transactions[txHash] = Transaction({ + destination: destination, + value: value, + data: data + }); + transactionCount += 1; + } } diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index 1d0ba80..d463274 100644 --- a/contracts/SideChain.sol +++ b/contracts/SideChain.sol @@ -1,226 +1,228 @@ pragma solidity ^0.4.21; -import './libs/MultiSigOwnable.sol'; -import './libs/Freezable.sol'; +import "./libs/MultiSigOwnable.sol"; +import "./libs/Freezable.sol"; contract SideChain is Freezable { - ////////////////////// - // CONSTANTS - ///////////////////// - uint8 public constant VERSION = 1; - - ////////////////////// - // EVENTS - ///////////////////// - event Confirmation(address indexed sender, bytes32 indexed txHash); - event Revocation(address indexed sender, bytes32 indexed txHash); - event Submission(bytes32 indexed txHash); - event Execution(bytes32 indexed txHash); - event ExecutionFailure(bytes32 indexed txHash); - event Deposit(address indexed sender, address indexed to, uint value); - event SignatureAdded(bytes32 txHash, uint8 v, bytes32 r, bytes32 s); - - ////////////////////// - // Modifiers - ///////////////////// - modifier notNull(address _address) { - require(_address != 0); - _; - } - - //////////////////////// - // Storage Variables - //////////////////////// - mapping (bytes32 => Transaction) public sideChainTx; - mapping (bytes32 => mapping(address => bool)) public isSignedSC; - uint256 sideChainTxCount; - - mapping (bytes32 => Transaction) mainChainTxs; - mapping (bytes32 => Signatures) mainChainSigs; - mapping (bytes32 => mapping(address => bool)) public isSignedMC; - uint256 mainChainTxCount; - - //////////////////////// - // Structs - //////////////////////// - struct Transaction { - address destination; - uint256 value; - bytes data; - bool executed; - } - - struct Signatures { - uint8[] v; - bytes32[] r; - bytes32[] s; - } - - ///////////////////////// - // Public Functions - //////////////////////// - - /// @dev Contract constructor sets initial owners and required number of confirmations. - /// @param _owners List of initial owners. - /// @param _required Number of required confirmations. - function SideChain(address[] _owners, uint8 _required) Freezable(_owners, _required) public { - } - - /// @dev Allows to deposit ETH into the contract - /// @param to the destination address where user wish to received the funds on mainchain - function deposit(address to) - notNull(to) - payable - public { - require(msg.value > 0); - emit Deposit(msg.sender, to, msg.value); - } + ////////////////////// + // CONSTANTS + ///////////////////// + uint8 public constant VERSION = 1; + + ////////////////////// + // EVENTS + ///////////////////// + event Confirmation(address indexed sender, bytes32 indexed txHash); + event Revocation(address indexed sender, bytes32 indexed txHash); + event Submission(bytes32 indexed txHash); + event Execution(bytes32 indexed txHash); + event ExecutionFailure(bytes32 indexed txHash); + event Deposit(address indexed sender, address indexed to, uint value); + event SignatureAdded(bytes32 txHash, uint8 v, bytes32 r, bytes32 s); + + ////////////////////// + // Modifiers + ///////////////////// + modifier notNull(address _address) { + require(_address != 0); + _; + } + + //////////////////////// + // Storage Variables + //////////////////////// + mapping (bytes32 => Transaction) public sideChainTx; + mapping (bytes32 => mapping(address => bool)) public isSignedSC; + uint256 sideChainTxCount; + + mapping (bytes32 => Transaction) mainChainTxs; + mapping (bytes32 => Signatures) mainChainSigs; + mapping (bytes32 => mapping(address => bool)) public isSignedMC; + uint256 mainChainTxCount; + + //////////////////////// + // Structs + //////////////////////// + struct Transaction { + address destination; + uint256 value; + bytes data; + bool executed; + } + + struct Signatures { + uint8[] v; + bytes32[] r; + bytes32[] s; + } + + ///////////////////////// + // Public Functions + //////////////////////// + + /// @dev Contract constructor sets initial owners and required number of confirmations. + /// @param _owners List of initial owners. + /// @param _required Number of required confirmations. + function SideChain(address[] _owners, uint8 _required) Freezable(_owners, _required) public { + } + + /// @dev Allows to deposit ETH into the contract + /// @param to the destination address where user wish to received the funds on mainchain + function deposit(address to) + notNull(to) + payable + public { + require(msg.value > 0); + emit Deposit(msg.sender, to, msg.value); + } /// @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 msgHash, bytes32 txHash, address destination, uint256 value, bytes data) - ownerExists(msg.sender) - notNull(destination) - public { - require(!isSignedSC[txHash][msg.sender]); - - bytes32 hashedTxParams = keccak256(txHash, destination, value, data, VERSION); - require(hashedTxParams == msgHash); - - if (sideChainTx[txHash].destination == address(0)) { - addTransactionSC(txHash, destination, value, data); - } - - isSignedSC[txHash][msg.sender] = true; - emit Confirmation(msg.sender, txHash); - executeTransaction(txHash); - } - - /// @dev Allows an owner to revoke a confirmation for a transaction. - /// @param txHash to revoke confirmation - function revokeConfirmation(bytes32 txHash) - ownerExists(msg.sender) - public { - require(!sideChainTx[txHash].executed); - require(isSignedSC[txHash][msg.sender]); - - isSignedSC[txHash][msg.sender] = false; - emit Revocation(msg.sender, txHash); - } - - /// @dev Allows anyone to execute a confirmed transaction. - /// @param txHash to be executed - function executeTransaction(bytes32 txHash) - public { - require(isSignedSC[txHash][msg.sender]); - require(!sideChainTx[txHash].executed); - if (isConfirmed(txHash)) { - Transaction storage txn = sideChainTx[txHash]; - txn.executed = true; - if (external_call(txn.destination, txn.value, txn.data.length, txn.data)) - emit Execution(txHash); - else { - emit ExecutionFailure(txHash); - txn.executed = false; - } - } - } - - /// @dev Returns the confirmation status of a transaction. - /// @return Confirmation status. - function isConfirmed(bytes32 txHash) - view - public - returns(bool) { - uint count = 0; - for (uint i=0; i Date: Fri, 18 May 2018 11:47:25 -0700 Subject: [PATCH 5/7] remove settings not needed --- .solcover.js | 1 - 1 file changed, 1 deletion(-) diff --git a/.solcover.js b/.solcover.js index 5065393..7ced80b 100644 --- a/.solcover.js +++ b/.solcover.js @@ -1,5 +1,4 @@ module.exports = { - norpc: true, port: 8555, testrpcOptions: '--port 8555 -m \"candy maple cake sugar pudding cream honey rich smooth crumble sweet treat\"', skipFiles: [ From f7a3212f3bc982ceeb2dc6b3865b6a9875111773 Mon Sep 17 00:00:00 2001 From: shine2lay Date: Fri, 18 May 2018 13:35:03 -0700 Subject: [PATCH 6/7] store msgHash in the Transaction for parameter validation --- contracts/SideChain.sol | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index d463274..285b794 100644 --- a/contracts/SideChain.sol +++ b/contracts/SideChain.sol @@ -44,6 +44,7 @@ contract SideChain is Freezable { // Structs //////////////////////// struct Transaction { + bytes32 msgHash; address destination; uint256 value; bytes data; @@ -81,19 +82,21 @@ contract SideChain is Freezable { /// @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) + function submitTransactionSC(bytes32 txHash, address destination, uint256 value, bytes data) ownerExists(msg.sender) notNull(destination) public { require(!isSignedSC[txHash][msg.sender]); - 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); + 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); @@ -182,14 +185,16 @@ contract SideChain is Freezable { /// @dev Adds a new transaction to the transaction mapping, if transaction does not exist yet. + /// @param msgHash keccek256 hash of the parameters /// @param txHash Transaction transactionHash /// @param destination Transaction target address. /// @param value Transaction ether value. /// @param data Transaction data payload. - function addTransactionSC(bytes32 txHash, address destination, uint value, bytes data) + function addTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint value, bytes data) notNull(destination) private { sideChainTx[txHash] = Transaction({ + msgHash: msgHash, destination: destination, value: value, executed: false, From f5b4e67560a999fae0acfd90712a4617e5177fdf Mon Sep 17 00:00:00 2001 From: shine2lay Date: Fri, 18 May 2018 13:42:07 -0700 Subject: [PATCH 7/7] fix eslint --- contracts/SideChain.sol | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/contracts/SideChain.sol b/contracts/SideChain.sol index 285b794..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,7 +43,7 @@ contract SideChain is Freezable { //////////////////////// // Structs //////////////////////// - struct Transaction { + struct SideChainTransaction { bytes32 msgHash; address destination; uint256 value; @@ -51,6 +51,13 @@ contract SideChain is Freezable { bool executed; } + + struct MainChainTransaction { + address destination; + uint256 value; + bytes data; + } + struct Signatures { uint8[] v; bytes32[] r; @@ -121,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); @@ -193,7 +200,7 @@ contract SideChain is Freezable { function addTransactionSC(bytes32 msgHash, bytes32 txHash, address destination, uint value, bytes data) notNull(destination) private { - sideChainTx[txHash] = Transaction({ + sideChainTx[txHash] = SideChainTransaction({ msgHash: msgHash, destination: destination, value: value, @@ -273,10 +280,9 @@ contract SideChain is Freezable { function addTransactionMC(bytes32 txHash, address destination, uint256 value, bytes data) notNull(destination) private { - mainChainTxs[txHash] = Transaction({ + mainChainTxs[txHash] = MainChainTransaction({ destination: destination, value: value, - executed: false, data: data }); mainChainTxCount += 1; @@ -288,7 +294,7 @@ contract SideChain is Freezable { view public returns(address destination, uint256 value, bytes data, uint8[] v, bytes32[] r, bytes32[] s) { - Transaction storage tempTx = mainChainTxs[txHash]; + MainChainTransaction storage tempTx = mainChainTxs[txHash]; Signatures storage tempSigs = mainChainSigs[txHash]; return (tempTx.destination, tempTx.value, tempTx.data, tempSigs.v, tempSigs.r, tempSigs.s); }