diff --git a/ethereum/contracts/icco/conductor/ConductorGetters.sol b/ethereum/contracts/icco/conductor/ConductorGetters.sol index 48510ea2..b81bf8e8 100644 --- a/ethereum/contracts/icco/conductor/ConductorGetters.sol +++ b/ethereum/contracts/icco/conductor/ConductorGetters.sol @@ -15,6 +15,10 @@ contract ConductorGetters is ConductorState { return _state.owner; } + function pendingOwner() public view returns (address) { + return _state.pendingOwner; + } + function isInitialized(address impl) public view returns (bool) { return _state.initializedImplementations[impl]; } diff --git a/ethereum/contracts/icco/conductor/ConductorGovernance.sol b/ethereum/contracts/icco/conductor/ConductorGovernance.sol index 243f216f..d1940f06 100644 --- a/ethereum/contracts/icco/conductor/ConductorGovernance.sol +++ b/ethereum/contracts/icco/conductor/ConductorGovernance.sol @@ -55,16 +55,36 @@ contract ConductorGovernance is ConductorGetters, ConductorSetters, ERC1967Upgra emit ConsistencyLevelUpdated(currentConsistencyLevel, newConsistencyLevel); } - /// @dev transferOwnership serves to change the ownership of the Conductor contract - function transferOwnership(uint16 conductorChainId, address newOwner) public onlyOwner { + /** + * @dev submitOwnershipTransferRequest serves to begin the ownership transfer process of the contracts + * - it saves an address for the new owner in the pending state + */ + function submitOwnershipTransferRequest(uint16 conductorChainId, address newOwner) public onlyOwner { require(conductorChainId == chainId(), "wrong chain id"); require(newOwner != address(0), "new owner cannot be the zero address"); - address currentOwner = owner(); - - setOwner(newOwner); + setPendingOwner(newOwner); + } - emit OwnershipTransfered(currentOwner, newOwner); + /** + * @dev confirmOwnershipTransferRequest serves to finalize an ownership transfer + * - it checks that the caller is the pendingOwner to validate the wallet address + * - it updates the owner state variable with the pendingOwner state variable + */ + function confirmOwnershipTransferRequest() public { + /// cache the new owner address + address newOwner = pendingOwner(); + + require(msg.sender == newOwner, "caller must be pendingOwner"); + + /// cache currentOwner for Event + address currentOwner = owner(); + + /// @dev update the owner in the contract state and reset the pending owner + setOwner(newOwner); + setPendingOwner(address(0)); + + emit OwnershipTransfered(currentOwner, newOwner); } modifier onlyOwner() { diff --git a/ethereum/contracts/icco/conductor/ConductorSetters.sol b/ethereum/contracts/icco/conductor/ConductorSetters.sol index 4f2c2239..2d94db93 100644 --- a/ethereum/contracts/icco/conductor/ConductorSetters.sol +++ b/ethereum/contracts/icco/conductor/ConductorSetters.sol @@ -11,6 +11,10 @@ contract ConductorSetters is ConductorState, Context { _state.owner = owner_; } + function setPendingOwner(address newOwner) internal { + _state.pendingOwner = newOwner; + } + function setContributor(uint16 chainId, bytes32 emitter) internal { _state.contributorImplementations[chainId] = emitter; } diff --git a/ethereum/contracts/icco/conductor/ConductorState.sol b/ethereum/contracts/icco/conductor/ConductorState.sol index f4050da9..cecfdb1f 100644 --- a/ethereum/contracts/icco/conductor/ConductorState.sol +++ b/ethereum/contracts/icco/conductor/ConductorState.sol @@ -36,6 +36,9 @@ contract ConductorStorage { /// contract deployer address owner; + + /// intermediate state when transfering contract ownership + address pendingOwner; /// number of confirmations for wormhole messages uint8 consistencyLevel; diff --git a/ethereum/contracts/icco/contributor/ContributorGetters.sol b/ethereum/contracts/icco/contributor/ContributorGetters.sol index fe76a073..ab76614b 100644 --- a/ethereum/contracts/icco/contributor/ContributorGetters.sol +++ b/ethereum/contracts/icco/contributor/ContributorGetters.sol @@ -15,6 +15,10 @@ contract ContributorGetters is ContributorState { return _state.owner; } + function pendingOwner() public view returns (address) { + return _state.pendingOwner; + } + function authority(uint256 saleId_) public view returns (address) { return _state.sales[saleId_].authority; } diff --git a/ethereum/contracts/icco/contributor/ContributorGovernance.sol b/ethereum/contracts/icco/contributor/ContributorGovernance.sol index 0e68efca..911ac6f7 100644 --- a/ethereum/contracts/icco/contributor/ContributorGovernance.sol +++ b/ethereum/contracts/icco/contributor/ContributorGovernance.sol @@ -47,18 +47,38 @@ contract ContributorGovernance is ContributorGetters, ContributorSetters, ERC196 setConsistencyLevel(newConsistencyLevel); emit ConsistencyLevelUpdated(currentConsistencyLevel, newConsistencyLevel); - } + } - /// @dev transferOwnership serves to change the ownership of the Contributor contract - function transferOwnership(uint16 contributorChainId, address newOwner) public onlyOwner { - require(contributorChainId == chainId(), "wrong chain id"); + /** + * @dev submitOwnershipTransferRequest serves to begin the ownership transfer process of the contracts + * - it saves an address for the new owner in the pending state + */ + function submitOwnershipTransferRequest(uint16 contributorChainId, address newOwner) public onlyOwner { + require(contributorChainId == chainId(), "wrong chain id"); require(newOwner != address(0), "new owner cannot be the zero address"); + setPendingOwner(newOwner); + } + + /** + * @dev confirmOwnershipTransferRequest serves to finalize an ownership transfer + * - it checks that the caller is the pendingOwner to validate the wallet address + * - it updates the owner state variable with the pendingOwner state variable + */ + function confirmOwnershipTransferRequest() public { + /// cache the new owner address + address newOwner = pendingOwner(); + + require(msg.sender == newOwner, "caller must be pendingOwner"); + + /// cache currentOwner for Event address currentOwner = owner(); + /// @dev update the owner in the contract state and reset the pending owner setOwner(newOwner); + setPendingOwner(address(0)); - emit OwnershipTransfered(currentOwner, newOwner); + emit OwnershipTransfered(currentOwner, newOwner); } modifier onlyOwner() { diff --git a/ethereum/contracts/icco/contributor/ContributorSetters.sol b/ethereum/contracts/icco/contributor/ContributorSetters.sol index 6edb7ab3..92d8406c 100644 --- a/ethereum/contracts/icco/contributor/ContributorSetters.sol +++ b/ethereum/contracts/icco/contributor/ContributorSetters.sol @@ -15,6 +15,10 @@ contract ContributorSetters is ContributorState, Context { _state.owner = owner_; } + function setPendingOwner(address newOwner) internal { + _state.pendingOwner = newOwner; + } + function setChainId(uint16 chainId) internal { _state.provider.chainId = chainId; } diff --git a/ethereum/contracts/icco/contributor/ContributorState.sol b/ethereum/contracts/icco/contributor/ContributorState.sol index 1c5264b7..5601f57a 100644 --- a/ethereum/contracts/icco/contributor/ContributorState.sol +++ b/ethereum/contracts/icco/contributor/ContributorState.sol @@ -57,6 +57,10 @@ contract ContributorStorage { /// deployer of the contracts address owner; + + /// intermediate state when transfering contract ownership + address pendingOwner; + /// number of confirmations for wormhole messages uint8 consistencyLevel; diff --git a/ethereum/test/icco.js b/ethereum/test/icco.js index ec53e8c6..2d28dc60 100644 --- a/ethereum/test/icco.js +++ b/ethereum/test/icco.js @@ -44,6 +44,7 @@ contract("ICCO", function(accounts) { const WORMHOLE = new web3.eth.Contract(WormholeImplementationFullABI, config.wormhole); const CONDUCTOR_BYTES32_ADDRESS = "0x000000000000000000000000" + TokenSaleConductor.address.substr(2); const KYC_AUTHORITY = "0x1dF62f291b2E969fB0849d99D9Ce41e2F137006e"; + const ZERO_ADDRESS = ethers.constants.AddressZero; const WORMHOLE_FEE = 1000; it("should set wormhole fee", async function() { @@ -393,64 +394,40 @@ contract("ICCO", function(accounts) { assert.ok(conductorFailed); }); - it("conductor and contributor should allow the owner to transfer ownership", async function() { + it("conductor should allow the owner to transfer ownership", async function() { // test variables const currentOwner = accounts[0]; const newOwner = accounts[1]; - const contributorContract = new web3.eth.Contract(ContributorImplementationFullABI, TokenSaleContributor.address); const conductorContract = new web3.eth.Contract(ConductorImplementationFullABI, TokenSaleConductor.address); - // transfer ownership - const contributorTx = await contributorContract.methods.transferOwnership(TEST_CHAIN_ID, newOwner).send({ + await conductorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, newOwner).send({ value: "0", from: currentOwner, // contract owner gasLimit: GAS_LIMIT, }); - const conductorTx = await conductorContract.methods.transferOwnership(TEST_CHAIN_ID, newOwner).send({ + const conductorTx = await conductorContract.methods.confirmOwnershipTransferRequest().send({ value: "0", - from: currentOwner, // contract owner + from: newOwner, // pending owner gasLimit: GAS_LIMIT, }); // check getters after the action - let contributorOwner = await contributorContract.methods.owner().call(); let conductorOwner = await conductorContract.methods.owner().call(); - assert.equal(contributorOwner, newOwner); assert.equal(conductorOwner, newOwner); - // confirm that the ConsistencyLevelUpdate event is emitted for contributor - let contributorEventOutput = contributorTx["events"]["OwnershipTransfered"]["returnValues"]; - - assert.equal(contributorEventOutput["oldOwner"].toLowerCase(), currentOwner.toLowerCase()); - assert.equal(contributorEventOutput["newOwner"].toLowerCase(), newOwner.toLowerCase()); - - // confirm that the ConsistencyLevelUpdate event is emitted for conductor + // confirm that the OwnershipTransfered event is emitted for conductor let conductorEventOutput = conductorTx["events"]["OwnershipTransfered"]["returnValues"]; assert.equal(conductorEventOutput["oldOwner"].toLowerCase(), currentOwner.toLowerCase()); assert.equal(conductorEventOutput["newOwner"].toLowerCase(), newOwner.toLowerCase()); - // make sure only the owner can transfer ownership - let contributorFailed = false; - try { - await contributorContract.methods.transferOwnership(TEST_CHAIN_ID, currentOwner).send({ - value: "0", - from: currentOwner, // no longer the current owner - gasLimit: GAS_LIMIT, - }); - } catch (e) { - assert.equal( - e.message, - "Returned error: VM Exception while processing transaction: revert caller is not the owner" - ); - contributorFailed = true; - } + // try to submit an ownership transfer request w/ non-owner wallet conductorFailed = false; try { - await conductorContract.methods.transferOwnership(TEST_CHAIN_ID, currentOwner).send({ + await conductorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, currentOwner).send({ value: "0", from: currentOwner, // no longer the current owner gasLimit: GAS_LIMIT, @@ -463,28 +440,150 @@ contract("ICCO", function(accounts) { conductorFailed = true; } - assert.ok(contributorFailed); assert.ok(conductorFailed); - // revert ownership back to currentOwner - await contributorContract.methods.transferOwnership(TEST_CHAIN_ID, currentOwner).send({ + // try to confirm the ownership change with the wrong wallet + conductorFailed = false; + try { + await conductorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, newOwner).send({ + value: "0", + from: newOwner, // contract owner + gasLimit: GAS_LIMIT, + }); + + await conductorContract.methods.confirmOwnershipTransferRequest().send({ + value: "0", + from: currentOwner, // pending owner + gasLimit: GAS_LIMIT, + }); + } catch (e) { + assert.equal( + e.message, + "Returned error: VM Exception while processing transaction: revert caller must be pendingOwner" + ); + conductorFailed = true; + } + + assert.ok(conductorFailed); + + // transfer ownership back to original owner + await conductorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, currentOwner).send({ value: "0", - from: newOwner, + from: newOwner, // contract owner gasLimit: GAS_LIMIT, }); - await conductorContract.methods.transferOwnership(TEST_CHAIN_ID, currentOwner).send({ + await conductorContract.methods.confirmOwnershipTransferRequest().send({ value: "0", - from: newOwner, + from: currentOwner, // pending owner gasLimit: GAS_LIMIT, }); - // check getters before the action - contributorOwner = await contributorContract.methods.owner().call(); + // check getters after the action conductorOwner = await conductorContract.methods.owner().call(); - assert.equal(contributorOwner, currentOwner); assert.equal(conductorOwner, currentOwner); + + // confirm that the pending owner address was reset + const pendingOwner = await conductorContract.methods.pendingOwner().call(); + + assert.equal(pendingOwner, ZERO_ADDRESS); + }); + + it("contributor should allow the owner to transfer ownership", async function() { + // test variables + const currentOwner = accounts[0]; + const newOwner = accounts[1]; + + const contributorContract = new web3.eth.Contract(ContributorImplementationFullABI, TokenSaleContributor.address); + + await contributorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, newOwner).send({ + value: "0", + from: currentOwner, // contract owner + gasLimit: GAS_LIMIT, + }); + + const contributorTx = await contributorContract.methods.confirmOwnershipTransferRequest().send({ + value: "0", + from: newOwner, // pending owner + gasLimit: GAS_LIMIT, + }); + + // check getters after the action + let contributorOwner = await contributorContract.methods.owner().call(); + + assert.equal(contributorOwner, newOwner); + + // confirm that the OwnershipTransfered event is emitted for contributor + let contributorEventOutput = contributorTx["events"]["OwnershipTransfered"]["returnValues"]; + + assert.equal(contributorEventOutput["oldOwner"].toLowerCase(), currentOwner.toLowerCase()); + assert.equal(contributorEventOutput["newOwner"].toLowerCase(), newOwner.toLowerCase()); + + // try to submit an ownership transfer request w/ non-owner wallet + contributorFailed = false; + try { + await contributorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, currentOwner).send({ + value: "0", + from: currentOwner, // no longer the current owner + gasLimit: GAS_LIMIT, + }); + } catch (e) { + assert.equal( + e.message, + "Returned error: VM Exception while processing transaction: revert caller is not the owner" + ); + contributorFailed = true; + } + + assert.ok(contributorFailed); + + // try to confirm the ownership change with the wrong wallet + contributorFailed = false; + try { + await contributorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, newOwner).send({ + value: "0", + from: newOwner, // contract owner + gasLimit: GAS_LIMIT, + }); + + await contributorContract.methods.confirmOwnershipTransferRequest().send({ + value: "0", + from: currentOwner, // pending owner + gasLimit: GAS_LIMIT, + }); + } catch (e) { + assert.equal( + e.message, + "Returned error: VM Exception while processing transaction: revert caller must be pendingOwner" + ); + contributorFailed = true; + } + + assert.ok(contributorFailed); + + // transfer ownership back to original owner + await contributorContract.methods.submitOwnershipTransferRequest(TEST_CHAIN_ID, currentOwner).send({ + value: "0", + from: newOwner, // contract owner + gasLimit: GAS_LIMIT, + }); + + await contributorContract.methods.confirmOwnershipTransferRequest().send({ + value: "0", + from: currentOwner, // pending owner + gasLimit: GAS_LIMIT, + }); + + // check getters after the action + contributorOwner = await contributorContract.methods.owner().call(); + + assert.equal(contributorOwner, currentOwner); + + // confirm that the pending owner address was reset + const pendingOwner = await contributorContract.methods.pendingOwner().call(); + + assert.equal(pendingOwner, ZERO_ADDRESS); }); // global sale test variables