From 8d63509f341366b41eb86ad08832f3d4bdf60ef7 Mon Sep 17 00:00:00 2001 From: Gulshan Vasnani Date: Wed, 19 Dec 2018 12:27:27 +0530 Subject: [PATCH 1/3] Changes done to ensure that only organization is able to anchor state roots --- contracts/gateway/SafeCore.sol | 2 +- test/gateway/safe_core/commit_state_root.js | 155 ++++++++++-------- .../get_latest_state_root_block_height.js | 2 +- test/gateway/safe_core/get_state_root.js | 2 +- 4 files changed, 89 insertions(+), 72 deletions(-) diff --git a/contracts/gateway/SafeCore.sol b/contracts/gateway/SafeCore.sol index 0b31175d..1e1e0caa 100644 --- a/contracts/gateway/SafeCore.sol +++ b/contracts/gateway/SafeCore.sol @@ -177,7 +177,7 @@ contract SafeCore is StateRootInterface, Organized, CircularBufferUint { bytes32 _stateRoot ) external - onlyWorker + onlyOrganization returns (bool success_) { // State root should be valid diff --git a/test/gateway/safe_core/commit_state_root.js b/test/gateway/safe_core/commit_state_root.js index 2b793b03..ecaf656c 100644 --- a/test/gateway/safe_core/commit_state_root.js +++ b/test/gateway/safe_core/commit_state_root.js @@ -29,7 +29,7 @@ const zeroBytes = "0x0000000000000000000000000000000000000000000000000000000000000000"; contract('SafeCore.commitStateRoot()', function (accounts) { - + let remoteChainId, blockHeight, stateRoot, @@ -38,9 +38,9 @@ contract('SafeCore.commitStateRoot()', function (accounts) { safeCore, owner, worker; - + beforeEach(async function () { - + owner = accounts[2]; worker = accounts[3]; remoteChainId = new BN(1410); @@ -48,7 +48,7 @@ contract('SafeCore.commitStateRoot()', function (accounts) { stateRoot = web3.utils.sha3("dummy_state_root"); maxNumberOfStateRoots = new BN(10); membersManager = await MockMembersManager.new(owner, worker); - + safeCore = await SafeCore.new( remoteChainId, blockHeight, @@ -56,143 +56,160 @@ contract('SafeCore.commitStateRoot()', function (accounts) { maxNumberOfStateRoots, membersManager.address, ); - + stateRoot = web3.utils.sha3("dummy_state_root_1"); - + }); - + it('should fail when state root is zero', async () => { - + stateRoot = zeroBytes; blockHeight = blockHeight.addn(1); - + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + {from: owner}, ), 'State root must not be zero.', ); - + }); - + it('should fail when block height is less than the latest committed ' + 'state root\'s block height', async () => { - - blockHeight = blockHeight.subn(1); - - await Utils.expectRevert( - safeCore.commitStateRoot( - blockHeight, - stateRoot, - { from: worker }, - ), - 'Given block height is lower or equal to highest committed state root block height.', - ); - - }); - + + blockHeight = blockHeight.subn(1); + + await Utils.expectRevert( + safeCore.commitStateRoot( + blockHeight, + stateRoot, + {from: owner}, + ), + 'Given block height is lower or equal to highest committed state root block height.', + ); + + }); + it('should fail when block height is equal to the latest committed ' + 'state root\'s block height', async () => { - - await Utils.expectRevert( - safeCore.commitStateRoot( - blockHeight, - stateRoot, - { from: worker }, - ), - 'Given block height is lower or equal to highest committed state root block height.', - ); - - }); - - it('should fail when caller is not worker address', async () => { - + + await Utils.expectRevert( + safeCore.commitStateRoot( + blockHeight, + stateRoot, + {from: owner}, + ), + 'Given block height is lower or equal to highest committed state root block height.', + ); + + }); + + it('should fail when caller is not owner address', async () => { + blockHeight = blockHeight.addn(1); - + let nonOwner = accounts[6]; + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, stateRoot, - { from: accounts[6] }, + {from: nonOwner}, ), - 'Only whitelisted workers are allowed to call this method.', + 'Only the organization is allowed to call this method.', ); - + }); - + + it('should fail when caller is not admin address', async () => { + + blockHeight = blockHeight.addn(1); + let nonOrganization = accounts[6]; + + await Utils.expectRevert( + safeCore.commitStateRoot( + blockHeight, + stateRoot, + {from: nonOrganization}, + ), + 'Only the organization is allowed to call this method.', + ); + + }); + it('should pass with correct params', async () => { - + blockHeight = blockHeight.addn(1); - + let result = await safeCore.commitStateRoot.call( blockHeight, stateRoot, - { from: worker }, + {from: owner}, ); - + assert.strictEqual( result, true, 'Return value of commitStateRoot must be true.', ); - + await safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + {from: owner}, ); - + let latestBlockHeight = await safeCore.getLatestStateRootBlockHeight.call(); assert.strictEqual( blockHeight.eq(latestBlockHeight), true, `Latest block height from the contract must be ${blockHeight}.`, ); - + let latestStateRoot = await safeCore.getStateRoot.call(blockHeight); assert.strictEqual( latestStateRoot, stateRoot, `Latest state root from the contract must be ${stateRoot}.`, ); - + }); - + it('should emit `StateRootAvailable` event', async () => { - + blockHeight = blockHeight.addn(1); - + let tx = await safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + {from: owner}, ); - + let event = EventDecoder.getEvents(tx, safeCore); - + assert.isDefined( event.StateRootAvailable, 'Event `StateRootAvailable` must be emitted.', ); - + let eventData = event.StateRootAvailable; - + assert.strictEqual( eventData._stateRoot, stateRoot, `The _stateRoot value in the event should be equal to ${stateRoot}` ); - + assert.strictEqual( blockHeight.eq(eventData._blockHeight), true, `The _blockHeight in the event should be equal to ${blockHeight}` ); - + }); - + it('should store only the given number of max store roots', async () => { /* * It should store the given state roots and they should be @@ -206,9 +223,9 @@ contract('SafeCore.commitStateRoot()', function (accounts) { await safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + {from: owner}, ); - + // Check that the older state root has been deleted when i > max state roots. if (maxNumberOfStateRoots.ltn(i)) { let prunedBlockHeight = blockHeight.sub(maxNumberOfStateRoots); @@ -221,7 +238,7 @@ contract('SafeCore.commitStateRoot()', function (accounts) { 'There should not be any state root stored at a ' + 'pruned height. It should have been reset by now.', ); - + /* * The state root that is one block younger than the pruned * one should still be available. @@ -238,5 +255,5 @@ contract('SafeCore.commitStateRoot()', function (accounts) { } } }); - + }); diff --git a/test/gateway/safe_core/get_latest_state_root_block_height.js b/test/gateway/safe_core/get_latest_state_root_block_height.js index 530a2de8..a1ec83ff 100644 --- a/test/gateway/safe_core/get_latest_state_root_block_height.js +++ b/test/gateway/safe_core/get_latest_state_root_block_height.js @@ -72,7 +72,7 @@ contract('SafeCore.getLatestStateRootBlockHeight()', function (accounts) { await safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + { from: owner }, ); let latestBlockHeight = await safeCore.getLatestStateRootBlockHeight.call(); diff --git a/test/gateway/safe_core/get_state_root.js b/test/gateway/safe_core/get_state_root.js index a4c8b1f9..342c5b48 100644 --- a/test/gateway/safe_core/get_state_root.js +++ b/test/gateway/safe_core/get_state_root.js @@ -90,7 +90,7 @@ contract('SafeCore.getStateRoot()', function (accounts) { await safeCore.commitStateRoot( blockHeight, stateRoot, - { from: worker }, + { from: owner }, ); let latestStateRoot = await safeCore.getStateRoot.call(blockHeight); From 8f39c82f16c6654fae6befa76be7d04c1f2bec52 Mon Sep 17 00:00:00 2001 From: Gulshan Vasnani Date: Wed, 19 Dec 2018 23:17:05 +0530 Subject: [PATCH 2/3] Removed duplicate tests and fixed js styling --- test/gateway/safe_core/commit_state_root.js | 100 ++++++++------------ 1 file changed, 42 insertions(+), 58 deletions(-) diff --git a/test/gateway/safe_core/commit_state_root.js b/test/gateway/safe_core/commit_state_root.js index ecaf656c..3368edd1 100644 --- a/test/gateway/safe_core/commit_state_root.js +++ b/test/gateway/safe_core/commit_state_root.js @@ -29,7 +29,7 @@ const zeroBytes = "0x0000000000000000000000000000000000000000000000000000000000000000"; contract('SafeCore.commitStateRoot()', function (accounts) { - + let remoteChainId, blockHeight, stateRoot, @@ -38,9 +38,9 @@ contract('SafeCore.commitStateRoot()', function (accounts) { safeCore, owner, worker; - + beforeEach(async function () { - + owner = accounts[2]; worker = accounts[3]; remoteChainId = new BN(1410); @@ -48,7 +48,7 @@ contract('SafeCore.commitStateRoot()', function (accounts) { stateRoot = web3.utils.sha3("dummy_state_root"); maxNumberOfStateRoots = new BN(10); membersManager = await MockMembersManager.new(owner, worker); - + safeCore = await SafeCore.new( remoteChainId, blockHeight, @@ -56,16 +56,16 @@ contract('SafeCore.commitStateRoot()', function (accounts) { maxNumberOfStateRoots, membersManager.address, ); - + stateRoot = web3.utils.sha3("dummy_state_root_1"); - + }); - + it('should fail when state root is zero', async () => { - + stateRoot = zeroBytes; blockHeight = blockHeight.addn(1); - + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, @@ -74,14 +74,14 @@ contract('SafeCore.commitStateRoot()', function (accounts) { ), 'State root must not be zero.', ); - + }); - + it('should fail when block height is less than the latest committed ' + 'state root\'s block height', async () => { - + blockHeight = blockHeight.subn(1); - + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, @@ -90,12 +90,12 @@ contract('SafeCore.commitStateRoot()', function (accounts) { ), 'Given block height is lower or equal to highest committed state root block height.', ); - + }); - + it('should fail when block height is equal to the latest committed ' + 'state root\'s block height', async () => { - + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, @@ -104,14 +104,14 @@ contract('SafeCore.commitStateRoot()', function (accounts) { ), 'Given block height is lower or equal to highest committed state root block height.', ); - + }); - + it('should fail when caller is not owner address', async () => { - + blockHeight = blockHeight.addn(1); let nonOwner = accounts[6]; - + await Utils.expectRevert( safeCore.commitStateRoot( blockHeight, @@ -120,96 +120,80 @@ contract('SafeCore.commitStateRoot()', function (accounts) { ), 'Only the organization is allowed to call this method.', ); - - }); - - it('should fail when caller is not admin address', async () => { - - blockHeight = blockHeight.addn(1); - let nonOrganization = accounts[6]; - - await Utils.expectRevert( - safeCore.commitStateRoot( - blockHeight, - stateRoot, - {from: nonOrganization}, - ), - 'Only the organization is allowed to call this method.', - ); - + }); - + it('should pass with correct params', async () => { - + blockHeight = blockHeight.addn(1); - + let result = await safeCore.commitStateRoot.call( blockHeight, stateRoot, {from: owner}, ); - + assert.strictEqual( result, true, 'Return value of commitStateRoot must be true.', ); - + await safeCore.commitStateRoot( blockHeight, stateRoot, {from: owner}, ); - + let latestBlockHeight = await safeCore.getLatestStateRootBlockHeight.call(); assert.strictEqual( blockHeight.eq(latestBlockHeight), true, `Latest block height from the contract must be ${blockHeight}.`, ); - + let latestStateRoot = await safeCore.getStateRoot.call(blockHeight); assert.strictEqual( latestStateRoot, stateRoot, `Latest state root from the contract must be ${stateRoot}.`, ); - + }); - + it('should emit `StateRootAvailable` event', async () => { - + blockHeight = blockHeight.addn(1); - + let tx = await safeCore.commitStateRoot( blockHeight, stateRoot, {from: owner}, ); - + let event = EventDecoder.getEvents(tx, safeCore); - + assert.isDefined( event.StateRootAvailable, 'Event `StateRootAvailable` must be emitted.', ); - + let eventData = event.StateRootAvailable; - + assert.strictEqual( eventData._stateRoot, stateRoot, `The _stateRoot value in the event should be equal to ${stateRoot}` ); - + assert.strictEqual( blockHeight.eq(eventData._blockHeight), true, `The _blockHeight in the event should be equal to ${blockHeight}` ); - + }); - + it('should store only the given number of max store roots', async () => { /* * It should store the given state roots and they should be @@ -225,7 +209,7 @@ contract('SafeCore.commitStateRoot()', function (accounts) { stateRoot, {from: owner}, ); - + // Check that the older state root has been deleted when i > max state roots. if (maxNumberOfStateRoots.ltn(i)) { let prunedBlockHeight = blockHeight.sub(maxNumberOfStateRoots); @@ -238,7 +222,7 @@ contract('SafeCore.commitStateRoot()', function (accounts) { 'There should not be any state root stored at a ' + 'pruned height. It should have been reset by now.', ); - + /* * The state root that is one block younger than the pruned * one should still be available. @@ -255,5 +239,5 @@ contract('SafeCore.commitStateRoot()', function (accounts) { } } }); - + }); From e164a8047b2d7442ae6356d8b67d8f0bf4307712 Mon Sep 17 00:00:00 2001 From: Gulshan Vasnani Date: Fri, 21 Dec 2018 13:07:16 +0530 Subject: [PATCH 3/3] Added license in the contract,updated documentation and styling --- contracts/gateway/GatewayBase.sol | 32 +++++++++++++++---- test/gateway/anchor/anchor_state_root.js | 2 +- .../get_latest_state_root_block_height.js | 2 +- test/gateway/anchor/get_state_root.js | 2 +- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/contracts/gateway/GatewayBase.sol b/contracts/gateway/GatewayBase.sol index e15e719b..94673a5e 100644 --- a/contracts/gateway/GatewayBase.sol +++ b/contracts/gateway/GatewayBase.sol @@ -1,5 +1,25 @@ pragma solidity ^0.5.0; +// Copyright 2018 OpenST Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// ---------------------------------------------------------------------------- +// +// http://www.simpletoken.org/ +// +// ---------------------------------------------------------------------------- + import "./EIP20Interface.sol"; import "../lib/MessageBus.sol"; import "../StateRootInterface.sol"; @@ -8,7 +28,6 @@ import "../lib/IsMemberInterface.sol"; import "../lib/Organized.sol"; import "../lib/SafeMath.sol"; - /** * @title GatewayBase contract. * @@ -157,12 +176,13 @@ contract GatewayBase is Organized { /* External functions */ /** - * @notice proveGateway can be called by anyone to verify merkle proof of + * @notice This can be called by anyone to verify merkle proof of * gateway/co-gateway contract address. Trust factor is brought by - * stateRoots mapping. It's important to note that in replay calls of - * proveGateway bytes _rlpParentNodes variable is not validated. In - * this case input storage root derived from merkle proof account - * nodes is verified with stored storage root of given blockHeight. + * state roots by the contract which implements StateRootInterface. + * It's important to note that in replay calls of proveGateway + * bytes _rlpParentNodes variable is not validated. In this case + * input storage root derived from merkle proof account nodes is + * verified with stored storage root of given blockHeight. * GatewayProven event has parameter wasAlreadyProved to * differentiate between first call and replay calls. * diff --git a/test/gateway/anchor/anchor_state_root.js b/test/gateway/anchor/anchor_state_root.js index 79d473df..5b3dbc23 100644 --- a/test/gateway/anchor/anchor_state_root.js +++ b/test/gateway/anchor/anchor_state_root.js @@ -70,7 +70,7 @@ contract('Anchor.anchorStateRoot()', function (accounts) { anchor.anchorStateRoot( blockHeight, stateRoot, - {from: worker}, + {from: owner}, ), 'State root must not be zero.', ); diff --git a/test/gateway/anchor/get_latest_state_root_block_height.js b/test/gateway/anchor/get_latest_state_root_block_height.js index 612345a4..d72dd9d0 100644 --- a/test/gateway/anchor/get_latest_state_root_block_height.js +++ b/test/gateway/anchor/get_latest_state_root_block_height.js @@ -72,7 +72,7 @@ contract('Anchor.getLatestStateRootBlockHeight()', function (accounts) { await anchor.anchorStateRoot( blockHeight, stateRoot, - { from: owner }, + {from: owner}, ); let latestBlockHeight = await anchor.getLatestStateRootBlockHeight.call(); diff --git a/test/gateway/anchor/get_state_root.js b/test/gateway/anchor/get_state_root.js index c21c9778..de271b13 100644 --- a/test/gateway/anchor/get_state_root.js +++ b/test/gateway/anchor/get_state_root.js @@ -90,7 +90,7 @@ contract('Anchor.getStateRoot()', function (accounts) { await anchor.anchorStateRoot( blockHeight, stateRoot, - { from: owner }, + {from: owner}, ); let latestStateRoot = await anchor.getStateRoot.call(blockHeight);