From c625c84b693dc9aa066358d99785ede9e32d1951 Mon Sep 17 00:00:00 2001 From: abhayks1 Date: Wed, 13 Mar 2019 22:02:04 +0530 Subject: [PATCH] Commit includes - Reentrancy evaluation and prevention mechanisms in GatewayComposer --- .travis.yml | 1 + contracts/GatewayComposer.sol | 72 +++++++++++++++++++++++--- test/gateway_composer/constructor.js | 22 ++++++++ test/gateway_composer/request_stake.js | 31 +++++++++++ test/gateway_composer/revert_stake.js | 21 ++++++++ 5 files changed, 141 insertions(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index a308df9..858627a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,7 @@ branches: only: - master - develop + - /^release-.*/ notifications: email: recipients: diff --git a/contracts/GatewayComposer.sol b/contracts/GatewayComposer.sol index 53c534c..2cd1165 100644 --- a/contracts/GatewayComposer.sol +++ b/contracts/GatewayComposer.sol @@ -56,6 +56,9 @@ contract GatewayComposer { mapping (bytes32 => StakeRequest) public stakeRequests; + /* Mutex lock status. */ + bool private isMutexLock; + /* Modifiers */ @@ -68,6 +71,15 @@ contract GatewayComposer { _; } + /** Checks that mutex lock is acquired or not. */ + modifier isMutexLockAcquired() { + require( + isMutexLock == false, + "Mutex lock is already acquired." + ); + _; + } + /* Special Functions */ @@ -78,6 +90,9 @@ contract GatewayComposer { * - owner address should not be zero * - ValueToken address should not be zero * - BrandedToken address should not be zero + * - owner address should not be equal to valueToken + * - owner address should not be equal to brandedToken + * - ValueToken address should be equal to BrandedToken.valueToken() * * @param _owner Address of the staker on the value chain. * @param _valueToken EIP20Token which is staked. @@ -102,6 +117,14 @@ contract GatewayComposer { address(_brandedToken) != address(0), "BrandedToken address is zero." ); + require( + _owner != address(_valueToken), + "ValueToken address is same as owner address." + ); + require( + _owner != address(_brandedToken), + "BrandedToken address is same as owner address." + ); require( address(_valueToken) == address(_brandedToken.valueToken()), "ValueToken should match BrandedToken.valueToken." @@ -124,6 +147,7 @@ contract GatewayComposer { * - stakeVT can't be 0 * - mintBT amount and converted stakeVT amount should be equal * - gateway address can't be zero + * - owner address should not be equal to gateway address * - beneficiary address can't be zero * - successful execution of ValueToken transfer * - successful execution of ValueToken approve @@ -160,8 +184,11 @@ contract GatewayComposer { ) external onlyOwner + isMutexLockAcquired returns (bytes32 stakeRequestHash_) { + acquireLock(); + require( _stakeVT > uint256(0), "Stake amount is zero." @@ -174,6 +201,10 @@ contract GatewayComposer { _gateway != address(0), "Gateway address is zero." ); + require( + owner != _gateway, + "Gateway address is same as owner address." + ); require( _beneficiary != address(0), "Beneficiary address is zero." @@ -197,6 +228,8 @@ contract GatewayComposer { gasLimit: _gasLimit, nonce: _nonce }); + + releaseLock(); } /** @@ -233,7 +266,10 @@ contract GatewayComposer { external returns (bytes32 messageHash_) { - StakeRequest storage stakeRequest = stakeRequests[_stakeRequestHash]; + StakeRequest memory stakeRequest = stakeRequests[_stakeRequestHash]; + + delete stakeRequests[_stakeRequestHash]; + require( stakeRequest.stakeVT > uint256(0), "Stake request not found." @@ -276,8 +312,6 @@ contract GatewayComposer { stakeRequest.nonce, _hashLock ); - - delete stakeRequests[_stakeRequestHash]; } /** @@ -305,7 +339,10 @@ contract GatewayComposer { onlyOwner returns (bool success_) { - StakeRequest storage stakeRequest = stakeRequests[_stakeRequestHash]; + StakeRequest memory stakeRequest = stakeRequests[_stakeRequestHash]; + + delete stakeRequests[_stakeRequestHash]; + require( stakeRequest.stakeVT > uint256(0), "Stake request not found." @@ -319,8 +356,6 @@ contract GatewayComposer { "ValueToken transfer returned false." ); - delete stakeRequests[_stakeRequestHash]; - success_ = true; } @@ -435,6 +470,7 @@ contract GatewayComposer { * @dev Function requires: * - msg.sender is owner * - gateway address can't be zero + * - Gateway address should not be equal to owner address * - successful execution of ValueToken transferFrom * - successful execution of ValueToken approve * @@ -462,6 +498,10 @@ contract GatewayComposer { _gateway != address(0), "Gateway address is zero." ); + require( + owner != _gateway, + "Gateway address is same as owner address." + ); require( valueToken.transferFrom(msg.sender, address(this), _penalty), "ValueToken transferFrom returned false." @@ -475,4 +515,24 @@ contract GatewayComposer { return true; } + + /* Private Functions */ + + /** + * @notice This acquires mutex lock. + */ + function acquireLock() + private + { + isMutexLock = true; + } + + /** + * @notice This releases the mutex lock. + */ + function releaseLock() + private + { + isMutexLock = false; + } } diff --git a/test/gateway_composer/constructor.js b/test/gateway_composer/constructor.js index fb0b4e3..04228b0 100644 --- a/test/gateway_composer/constructor.js +++ b/test/gateway_composer/constructor.js @@ -72,6 +72,28 @@ contract('GatewayComposer::constructor', async (accounts) => { 'BrandedToken address is zero.'); }); + it('Reverts if ValueToken address is same as owner address.', async () => { + await utils.expectRevert(GatewayComposer.new( + valueToken, + valueToken, + brandedToken, + { from: deployer }, + ), + 'It should revert as valueToken address is same as owner address.', + 'ValueToken address is same as owner address.'); + }); + + it('Reverts if BrandedToken address is same as owner address.', async () => { + await utils.expectRevert(GatewayComposer.new( + brandedToken, + valueToken, + brandedToken, + { from: deployer }, + ), + 'It should revert as BrandedToken address is same as owner address.', + 'BrandedToken address is same as owner address.'); + }); + it('Reverts if ValueToken is not equal to BrandedToken.valueToken.', async () => { brandedToken = await BrandedToken.new( accountProvider.get(), diff --git a/test/gateway_composer/request_stake.js b/test/gateway_composer/request_stake.js index a054060..08264c2 100644 --- a/test/gateway_composer/request_stake.js +++ b/test/gateway_composer/request_stake.js @@ -165,6 +165,37 @@ contract('GatewayComposer::requestStake', async (accounts) => { 'Gateway address is zero.'); }); + it('Fails when gateway address is same as owner address.', async () => { + const { + valueToken, + gatewayComposer, + owner, + brandedToken, + } = await gatewayComposerUtils.setupGatewayComposer(accountProvider); + + const { + stakeAmount, + } = await gatewayComposerUtils.approveGatewayComposer( + valueToken, + gatewayComposer, + owner, + ); + + const mintAmount = await brandedToken.convertToBrandedTokens(stakeAmount); + await utils.expectRevert(gatewayComposer.requestStake( + stakeAmount, + mintAmount, + owner, + beneficiary, + gasPrice, + gasLimit, + nonce, + { from: owner }, + ), + 'Should revert because gateway address and owner address are same.', + 'Gateway address is same as owner address.'); + }); + it('Fails when beneficiary address is zero.', async () => { const { valueToken, diff --git a/test/gateway_composer/revert_stake.js b/test/gateway_composer/revert_stake.js index dbdaf8a..a64fd8a 100644 --- a/test/gateway_composer/revert_stake.js +++ b/test/gateway_composer/revert_stake.js @@ -73,6 +73,27 @@ contract('GatewayComposer::revertStake', async (accounts) => { ); }); + it('Fails when gateway address is same as owner address', async () => { + const { + gatewayComposer, + owner, + } = await gatewayComposerUtils.setupGatewayComposer(accountProvider); + + const penalty = 0; + const messageHash = web3.utils.soliditySha3('hash'); + + await utils.expectRevert( + gatewayComposer.revertStake( + owner, + penalty, + messageHash, + { from: owner }, + ), + 'Should revert as gateway address and owner address are same.', + 'Gateway address is same as owner address.', + ); + }); + it('Fails if valueToken transferFrom returns false', async () => { const { gatewayComposer,