From 5a444ecb49f1d8f634cd91b749960a916c600372 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Sat, 11 Nov 2017 01:12:37 +0300 Subject: [PATCH 1/2] Add ability to override Ownable base contracts --- contracts/Multiownable.sol | 34 +++++++++++++++++---------- contracts/MultiownableOverOwnable.sol | 32 +++++++++++++++++++++++++ test/Multiownable.js | 25 ++++++++++++++++++++ test/impl/MultisigMintableToken.sol | 19 +++++++++++++++ 4 files changed, 98 insertions(+), 12 deletions(-) create mode 100644 contracts/MultiownableOverOwnable.sol create mode 100644 test/impl/MultisigMintableToken.sol diff --git a/contracts/Multiownable.sol b/contracts/Multiownable.sol index 834be38..495a0f0 100644 --- a/contracts/Multiownable.sol +++ b/contracts/Multiownable.sol @@ -52,8 +52,27 @@ contract Multiownable { modifier onlyManyOwners { if (insideOnlyManyOwners == msg.sender) { _; - return; + } else if (checkOnlyManyOwners()) { + insideOnlyManyOwners = msg.sender; + _; + insideOnlyManyOwners = address(0); } + } + + // CONSTRUCTOR + + function Multiownable() public { + owners.push(msg.sender); + ownersIndices[msg.sender] = 1; + howManyOwnersDecide = 1; + } + + // INTERNAL METHODS + + /** + * @dev onlyManyOwners modifier helper + */ + function checkOnlyManyOwners() internal constant returns(bool) { require(isOwner(msg.sender)); uint ownerIndex = ownersIndices[msg.sender] - 1; @@ -70,18 +89,9 @@ contract Multiownable { // If all owners confirm same operation if (votesCountByOperation[operation] == howManyOwnersDecide) { deleteOperation(operation); - insideOnlyManyOwners = msg.sender; - _; - insideOnlyManyOwners = address(0); + return true; } - } - - // CONSTRUCTOR - - function Multiownable() public { - owners.push(msg.sender); - ownersIndices[msg.sender] = 1; - howManyOwnersDecide = 1; + return false; } // INTERNAL METHODS diff --git a/contracts/MultiownableOverOwnable.sol b/contracts/MultiownableOverOwnable.sol new file mode 100644 index 0000000..3cdf1b0 --- /dev/null +++ b/contracts/MultiownableOverOwnable.sol @@ -0,0 +1,32 @@ +pragma solidity ^0.4.11; + +import 'zeppelin-solidity/contracts/ownership/Ownable.sol'; +import './Multiownable.sol'; + + +contract MultiownableOverOwnable is Ownable, Multiownable { + + /** + * @dev Allows to override Ownable onlyOwner modifier to onlyAnyOwner + */ + modifier onlyOwner_overrideForAnyOwner { + require(isOwner(msg.sender)); + owner = msg.sender; + _; + } + + /** + * @dev Allows to override Ownable onlyOwner modifier to onlyManyOwners + */ + modifier onlyOwner_overrideForManyOwners { + if (insideOnlyManyOwners == msg.sender) { + _; + } else if (checkOnlyManyOwners()) { + insideOnlyManyOwners = msg.sender; + owner = msg.sender; + _; + insideOnlyManyOwners = address(0); + } + } + +} \ No newline at end of file diff --git a/test/Multiownable.js b/test/Multiownable.js index e21679e..5987cee 100644 --- a/test/Multiownable.js +++ b/test/Multiownable.js @@ -16,6 +16,7 @@ import EVMThrow from './helpers/EVMThrow'; const Multiownable = artifacts.require('Multiownable.sol'); const MultiownableImpl = artifacts.require('./impl/MultiownableImpl.sol'); +const MultisigMintableToken = artifacts.require('./impl/MultisigMintableToken.sol'); contract('Multiownable', function ([_, wallet1, wallet2, wallet3, wallet4, wallet5]) { @@ -415,4 +416,28 @@ contract('Multiownable', function ([_, wallet1, wallet2, wallet3, wallet4, walle ]).should.be.rejectedWith(EVMThrow); }) + it('should override onlyOwner_overrideForAnyOwner', async function() { + const token = await MultisigMintableToken.new(); + await token.transferOwnership([wallet1, wallet2]); + + await token.mint(_, 100).should.be.rejectedWith(EVMThrow); + + (await token.totalSupply.call()).should.be.bignumber.equal(0); + await token.mint(_, 100, {from: wallet1}); + (await token.totalSupply.call()).should.be.bignumber.equal(0); + await token.mint(_, 100, {from: wallet2}); + (await token.totalSupply.call()).should.be.bignumber.equal(100); + }) + + it('should override onlyOwner_overrideForManyOwners', async function() { + const token = await MultisigMintableToken.new(); + await token.transferOwnership([wallet1, wallet2]); + + await token.finishMinting().should.be.rejectedWith(EVMThrow); + + (await token.mintingFinished.call()).should.be.false; + await token.finishMinting({from: wallet2}); + (await token.mintingFinished.call()).should.be.true; + }) + }) diff --git a/test/impl/MultisigMintableToken.sol b/test/impl/MultisigMintableToken.sol new file mode 100644 index 0000000..c922b19 --- /dev/null +++ b/test/impl/MultisigMintableToken.sol @@ -0,0 +1,19 @@ +pragma solidity ^0.4.11; + +import "zeppelin-solidity/contracts/token/MintableToken.sol"; +import "../../contracts/MultiownableOverOwnable.sol"; + + +contract MultisigMintableToken is MintableToken, MultiownableOverOwnable { + + // Only all owners can mint + function mint(address _to, uint256 _amount) onlyOwner_overrideForManyOwners canMint public returns (bool) { + return super.mint(_to, _amount); + } + + // Any of the owners can finish + function finishMinting() onlyOwner_overrideForAnyOwner public returns (bool) { + return super.finishMinting(); + } + +} \ No newline at end of file From ff9fa391c0d0262b078a759d11532bbac446a308 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Sat, 18 Nov 2017 12:02:47 +0300 Subject: [PATCH 2/2] Fix tests coverage --- test/Multiownable.js | 10 ++++++++++ test/impl/MultisigMintableToken.sol | 25 ++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/test/Multiownable.js b/test/Multiownable.js index 5987cee..6893dc9 100644 --- a/test/Multiownable.js +++ b/test/Multiownable.js @@ -387,6 +387,16 @@ contract('Multiownable', function ([_, wallet1, wallet2, wallet3, wallet4, walle (await obj.value.call()).should.be.bignumber.equal(100); }) + it('should works for nested methods with onlyOwner_overrideForManyOwners modifier', async function() { + const obj = await MultisigMintableToken.new(); + await obj.transferOwnership([wallet1, wallet2]); + + await obj.nestedFirst(100, {from: wallet1}); + await obj.nestedFirst(100, {from: wallet2}); + + (await obj.value.call()).should.be.bignumber.equal(100); + }) + it('should not allow to transfer ownership to several equal users', async function() { const obj = await Multiownable.new(); await obj.transferOwnership([wallet1, wallet1]).should.be.rejectedWith(EVMThrow); diff --git a/test/impl/MultisigMintableToken.sol b/test/impl/MultisigMintableToken.sol index c922b19..9aec9cf 100644 --- a/test/impl/MultisigMintableToken.sol +++ b/test/impl/MultisigMintableToken.sol @@ -4,7 +4,22 @@ import "zeppelin-solidity/contracts/token/MintableToken.sol"; import "../../contracts/MultiownableOverOwnable.sol"; -contract MultisigMintableToken is MintableToken, MultiownableOverOwnable { +contract MintableTokenExt is MintableToken { + + uint public value = 0; + + function nestedFirst(uint _value) public onlyOwner { + nestedSecond(_value); + } + + function nestedSecond(uint _value) public onlyOwner { + value = _value; + } + +} + + +contract MultisigMintableToken is MintableTokenExt, MultiownableOverOwnable { // Only all owners can mint function mint(address _to, uint256 _amount) onlyOwner_overrideForManyOwners canMint public returns (bool) { @@ -16,4 +31,12 @@ contract MultisigMintableToken is MintableToken, MultiownableOverOwnable { return super.finishMinting(); } + function nestedFirst(uint _value) public onlyOwner_overrideForManyOwners { + super.nestedFirst(_value); + } + + function nestedSecond(uint _value) public onlyOwner_overrideForManyOwners { + super.nestedSecond(_value); + } + } \ No newline at end of file