From 2dd71cc6642a5feaa0ab16cef55aaf8e548fa765 Mon Sep 17 00:00:00 2001 From: Sarvesh Jain Date: Tue, 25 Jun 2019 12:00:15 +0530 Subject: [PATCH 1/3] Destructing staker proxy via ost composer contract --- contracts/gateway/OSTComposer.sol | 24 ++++----- contracts/gateway/StakerProxy.sol | 12 ++--- .../gateway/MockStakerProxy.sol} | 24 +++++++-- ...aker_proxy.js => destruct_staker_proxy.js} | 39 ++++++--------- test/gateway/staker_proxy/self_destruct.js | 49 +++++++++++++++++++ 5 files changed, 100 insertions(+), 48 deletions(-) rename contracts/{gateway/ComposerInterface.sol => test/gateway/MockStakerProxy.sol} (61%) rename test/gateway/ost_composer/{remove_staker_proxy.js => destruct_staker_proxy.js} (71%) create mode 100644 test/gateway/staker_proxy/self_destruct.js diff --git a/contracts/gateway/OSTComposer.sol b/contracts/gateway/OSTComposer.sol index be722e9e..0a02fc48 100644 --- a/contracts/gateway/OSTComposer.sol +++ b/contracts/gateway/OSTComposer.sol @@ -25,7 +25,6 @@ import "./StakerProxy.sol"; import "./EIP20GatewayInterface.sol"; import "../lib/EIP20Interface.sol"; import "../lib/Mutex.sol"; -import "./ComposerInterface.sol"; /** * @title OSTComposer implements Organized contract. Reentrancy is prevented @@ -33,7 +32,7 @@ import "./ComposerInterface.sol"; * * @notice It facilitates the staker to get the OSTPrime on sidechains. */ -contract OSTComposer is Organized, Mutex, ComposerInterface { +contract OSTComposer is Organized, Mutex { /* Constants */ @@ -365,19 +364,22 @@ contract OSTComposer is Organized, Mutex, ComposerInterface { } /** - * @notice It can only be called by StakerProxy contract of the staker. It - * deletes the StakerProxy contract of the staker. - * - * @param _owner Owner of the StakerProxy contract. + * @notice It can only be called by owner of the staker proxy. It + * deletes the StakerProxy contract of the staker and calls self + * destruct on StakerProxy contract. */ - function removeStakerProxy( - address _owner - ) + function destructStakerProxy() external - onlyStakerProxy(_owner) { + + StakerProxy stakerProxy = stakerProxies[msg.sender]; + require( + address(stakerProxy) != address(0), + "Staker proxy doesnot exist for this owner." + ); // Resetting the proxy address of the staker. - delete stakerProxies[_owner]; + delete stakerProxies[msg.sender]; + stakerProxy.selfDestruct(); } diff --git a/contracts/gateway/StakerProxy.sol b/contracts/gateway/StakerProxy.sol index c2f6d975..533f9f7d 100644 --- a/contracts/gateway/StakerProxy.sol +++ b/contracts/gateway/StakerProxy.sol @@ -20,7 +20,6 @@ pragma solidity ^0.5.0; // // ---------------------------------------------------------------------------- -import "./ComposerInterface.sol"; import "./EIP20GatewayInterface.sol"; import "../lib/EIP20Interface.sol"; import "../lib/Mutex.sol"; @@ -38,7 +37,7 @@ contract StakerProxy is Mutex { /* Storage */ /** The composer that deployed this contract. */ - ComposerInterface public composer; + address public composer; /** The composer deployed the StakerProxy on behalf of the owner. */ address payable public owner; @@ -70,13 +69,12 @@ contract StakerProxy is Mutex { /* Special Functions */ /** - * @notice Must be constructed by a contract that implements the - * `ComposerInterface`. + * @notice Must be constructed by a composer contract. * * @param _owner The owner that this proxy is deployed for. */ constructor(address payable _owner) public { - composer = ComposerInterface(msg.sender); + composer = msg.sender; owner = _owner; } @@ -160,9 +158,7 @@ contract StakerProxy is Mutex { * to transfer all remaining token balance of this contract before * calling this method. */ - function selfDestruct() external onlyOwner { - composer.removeStakerProxy(owner); - + function selfDestruct() external onlyComposer { selfdestruct(owner); } diff --git a/contracts/gateway/ComposerInterface.sol b/contracts/test/gateway/MockStakerProxy.sol similarity index 61% rename from contracts/gateway/ComposerInterface.sol rename to contracts/test/gateway/MockStakerProxy.sol index 2e68be69..49531d9e 100644 --- a/contracts/gateway/ComposerInterface.sol +++ b/contracts/test/gateway/MockStakerProxy.sol @@ -20,13 +20,27 @@ pragma solidity ^0.5.0; // // ---------------------------------------------------------------------------- +/** + * @title MockStakerProxy is a contract used for unit testing of ostComposer method `destructStakerProxy`. + */ +contract MockStakerProxy { -interface ComposerInterface { + /* Storage */ + + /** Flag to assert if self destruct is called. */ + bool public selfDestruted; + + + /* Special Functions */ + + constructor () public { + selfDestruted = false; + } /** - * @notice It deletes the StakerProxy contract for the staker. - * - * @param _owner Owner of the StakerProxy contract. + * @notice Mock method called from ost composer during unit testing of `destructStakerProxy`. */ - function removeStakerProxy(address _owner) external; + function selfDestruct() external { + selfDestruted = true; + } } diff --git a/test/gateway/ost_composer/remove_staker_proxy.js b/test/gateway/ost_composer/destruct_staker_proxy.js similarity index 71% rename from test/gateway/ost_composer/remove_staker_proxy.js rename to test/gateway/ost_composer/destruct_staker_proxy.js index 99fb565a..99025f5e 100644 --- a/test/gateway/ost_composer/remove_staker_proxy.js +++ b/test/gateway/ost_composer/destruct_staker_proxy.js @@ -20,16 +20,14 @@ const OSTComposer = artifacts.require('TestOSTComposer'); const MockOrganization = artifacts.require('MockOrganization'); -const StakerProxy = artifacts.require('StakerProxy'); -const BN = require('bn.js'); +const StakerProxy = artifacts.require('MockStakerProxy'); const Utils = require('../../test_lib/utils'); -contract('OSTComposer.removeStakerProxy() ', (accounts) => { +contract('OSTComposer.destructStakerProxy() ', (accounts) => { let organization; let ostComposer; const stakeRequest = {}; let stakerProxy; - let expectedActiveGatewayCount; let owner; let worker; beforeEach(async () => { @@ -38,15 +36,13 @@ contract('OSTComposer.removeStakerProxy() ', (accounts) => { organization = await MockOrganization.new(owner, worker); stakeRequest.staker = accounts[2]; ostComposer = await OSTComposer.new(organization.address); - expectedActiveGatewayCount = new BN(0); - stakerProxy = accounts[10]; - await ostComposer.setStakerProxy(stakeRequest.staker, stakerProxy); + stakerProxy = await StakerProxy.new(); + await ostComposer.setStakerProxy(stakeRequest.staker, stakerProxy.address); }); it('should be able to successfully remove staker proxy', async () => { - const response = await ostComposer.removeStakerProxy( - stakeRequest.staker, - { from: stakerProxy }, + const response = await ostComposer.destructStakerProxy( + { from: stakeRequest.staker }, ); assert.strictEqual(response.receipt.status, true, 'Receipt status is unsuccessful'); @@ -57,27 +53,22 @@ contract('OSTComposer.removeStakerProxy() ', (accounts) => { Utils.NULL_ADDRESS, 'Staker\'s proxy address must be reset to null', ); - }); - it('should fail when non proxy address request removal of staker proxy', async () => { - const nonProxy = accounts[8]; - await Utils.expectRevert( - ostComposer.removeStakerProxy( - stakeRequest.staker, - { from: nonProxy }, - ), - 'Caller is invalid proxy address.', + const isSelfDestructed = await stakerProxy.selfDestruted.call(); + assert.strictEqual( + isSelfDestructed, + true, + 'Staker proxy self destruct must be called', ); }); it('should fail when owner doesn\'t have any deployed staker proxy', async () => { - const nonOwner = accounts[12]; + const nonProxy = accounts[8]; await Utils.expectRevert( - ostComposer.removeStakerProxy( - nonOwner, - { from: stakerProxy }, + ostComposer.destructStakerProxy( + { from: nonProxy }, ), - 'Caller is invalid proxy address.', + 'Staker proxy doesnot exist for this owner.', ); }); }); diff --git a/test/gateway/staker_proxy/self_destruct.js b/test/gateway/staker_proxy/self_destruct.js new file mode 100644 index 00000000..6206cf77 --- /dev/null +++ b/test/gateway/staker_proxy/self_destruct.js @@ -0,0 +1,49 @@ +// Copyright 2019 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/ +// +// ---------------------------------------------------------------------------- + +'use strict'; + +const StakerProxy = artifacts.require('./StakerProxy.sol'); +const Utils = require('../../test_lib/utils'); + +contract('StakerProxy.selfDestruct()', (accounts) => { + const [composer, owner, anyOtherAddress] = accounts; + let stakerProxy; + + beforeEach(async () => { + stakerProxy = await StakerProxy.new( + owner, + { from: composer }, + ); + }); + + it('should allow self destruct by composer ', async () => { + const response = await stakerProxy.selfDestruct({ from: composer }); + + assert.strictEqual(response.receipt.status, true, 'Receipt status is unsuccessful'); + }); + + it('should not allow self destruct by non composer address ', async () => { + await Utils.expectRevert( + stakerProxy.selfDestruct({ from: anyOtherAddress }), + 'This function can only be called by the composer.', + ); + }); +}); From c9a6d1464e52c05cf1b16bcd0e49ae0b7cbc494f Mon Sep 17 00:00:00 2001 From: Sarvesh Jain Date: Tue, 25 Jun 2019 16:54:09 +0530 Subject: [PATCH 2/3] Fixed formating and enhanced require message --- contracts/gateway/OSTComposer.sol | 2 +- contracts/test/gateway/MockStakerProxy.sol | 6 ++++-- test/gateway/ost_composer/destruct_staker_proxy.js | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/contracts/gateway/OSTComposer.sol b/contracts/gateway/OSTComposer.sol index 0a02fc48..ab5a8ee9 100644 --- a/contracts/gateway/OSTComposer.sol +++ b/contracts/gateway/OSTComposer.sol @@ -375,7 +375,7 @@ contract OSTComposer is Organized, Mutex { StakerProxy stakerProxy = stakerProxies[msg.sender]; require( address(stakerProxy) != address(0), - "Staker proxy doesnot exist for this owner." + "Staker proxy does not exist for the caller." ); // Resetting the proxy address of the staker. delete stakerProxies[msg.sender]; diff --git a/contracts/test/gateway/MockStakerProxy.sol b/contracts/test/gateway/MockStakerProxy.sol index 49531d9e..df213fe4 100644 --- a/contracts/test/gateway/MockStakerProxy.sol +++ b/contracts/test/gateway/MockStakerProxy.sol @@ -21,7 +21,8 @@ pragma solidity ^0.5.0; // ---------------------------------------------------------------------------- /** - * @title MockStakerProxy is a contract used for unit testing of ostComposer method `destructStakerProxy`. + * @title MockStakerProxy is a contract used for unit testing of ostComposer + * method `destructStakerProxy`. */ contract MockStakerProxy { @@ -38,7 +39,8 @@ contract MockStakerProxy { } /** - * @notice Mock method called from ost composer during unit testing of `destructStakerProxy`. + * @notice Mock method called from ost composer during unit testing of + * `destructStakerProxy`. */ function selfDestruct() external { selfDestruted = true; diff --git a/test/gateway/ost_composer/destruct_staker_proxy.js b/test/gateway/ost_composer/destruct_staker_proxy.js index 99025f5e..fcb3fb4a 100644 --- a/test/gateway/ost_composer/destruct_staker_proxy.js +++ b/test/gateway/ost_composer/destruct_staker_proxy.js @@ -68,7 +68,7 @@ contract('OSTComposer.destructStakerProxy() ', (accounts) => { ostComposer.destructStakerProxy( { from: nonProxy }, ), - 'Staker proxy doesnot exist for this owner.', + 'Staker proxy does not exist for the caller.', ); }); }); From 6dda214e9d19abc5dccd4334033b70e246124712 Mon Sep 17 00:00:00 2001 From: Sarvesh Jain Date: Tue, 25 Jun 2019 17:01:35 +0530 Subject: [PATCH 3/3] Added assertion of contract code after self distructing --- test/gateway/staker_proxy/self_destruct.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/gateway/staker_proxy/self_destruct.js b/test/gateway/staker_proxy/self_destruct.js index 6206cf77..a361766c 100644 --- a/test/gateway/staker_proxy/self_destruct.js +++ b/test/gateway/staker_proxy/self_destruct.js @@ -38,6 +38,14 @@ contract('StakerProxy.selfDestruct()', (accounts) => { const response = await stakerProxy.selfDestruct({ from: composer }); assert.strictEqual(response.receipt.status, true, 'Receipt status is unsuccessful'); + + const stakerProxyCode = await web3.eth.getCode(stakerProxy.address); + + assert.strictEqual( + stakerProxyCode, + '0x', + 'Staker proxy contract code should be 0x after self destructing', + ); }); it('should not allow self destruct by non composer address ', async () => {