certusone
/
wormhole-icco
mirror of https://github.com/certusone/wormhole-icco.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Ownership can be transferred to an invalid address (#53) 

* Add ownership transfer pending state

* Reset pending owner to zero address after ownership transfer

Co-authored-by: Drew <dsterioti@users.noreply.github.com>
This commit is contained in:
Drew 2022-07-19 12:46:13 -05:00 committed by GitHub
parent 6a5c688f53
commit d75e7ee3c3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 213 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
ethereum/contracts/icco/conductor/ConductorGetters.sol
View File

@ -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];
}

32
ethereum/contracts/icco/conductor/ConductorGovernance.sol
View File

@ -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() {

4
ethereum/contracts/icco/conductor/ConductorSetters.sol
View File

@ -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;
}

3
ethereum/contracts/icco/conductor/ConductorState.sol
View File

@ -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;

4
ethereum/contracts/icco/contributor/ContributorGetters.sol
View File

@ -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;
}

30
ethereum/contracts/icco/contributor/ContributorGovernance.sol
View File

@ -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() {

4
ethereum/contracts/icco/contributor/ContributorSetters.sol
View File

@ -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;
}

4
ethereum/contracts/icco/contributor/ContributorState.sol
View File

@ -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;

179
ethereum/test/icco.js
View File

@ -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