From 59fb48d94addd61f973ff2072d3faba773beca57 Mon Sep 17 00:00:00 2001 From: Truls Asheim Date: Sat, 26 Jan 2019 11:19:52 +0100 Subject: [PATCH] Only allow implementor to change balances (Closes #76) --- .../token/ERC20/ExternalERC20Storage.sol | 24 ++++-- test/token/ERC20/ExternalERC20Storage.test.js | 80 +++++++++---------- 2 files changed, 56 insertions(+), 48 deletions(-) diff --git a/contracts/token/ERC20/ExternalERC20Storage.sol b/contracts/token/ERC20/ExternalERC20Storage.sol index 75b54fc..dd69d46 100644 --- a/contracts/token/ERC20/ExternalERC20Storage.sol +++ b/contracts/token/ERC20/ExternalERC20Storage.sol @@ -55,38 +55,38 @@ contract ExternalERC20Storage is Ownable { function setBalance(address owner, uint256 value) public - onlyImplementorOrOwner + onlyImplementor { balances[owner] = value; } /** * @dev Sets new allowance. - * Can only be done by owner or implementor contract. + * Can only be called by implementor contract. */ function setAllowed(address owner, address spender, uint256 value) public - onlyImplementorOrOwner + onlyImplementor { allowed[owner][spender] = value; } /** * @dev Change totalSupply. - * Can only be done by owner or implementor contract. + * Can only be called by implementor contract. */ function setTotalSupply(uint256 value) public - onlyImplementorOrOwner + onlyImplementor { totalSupply = value; } /** * @dev Transfer implementor to new contract - * Can only be done by owner or implementor contract. + * Can only be called by owner or implementor contract. */ function transferImplementor(address newImplementor) public @@ -101,7 +101,7 @@ contract ExternalERC20Storage is Ownable { } /** - * @dev Asserts if sender is neither owner nor implementor. + * @dev Asserts that sender is either owner or implementor. */ modifier onlyImplementorOrOwner() { require(isImplementor() || isOwner(), "Is not implementor or owner"); @@ -109,7 +109,15 @@ contract ExternalERC20Storage is Ownable { } /** - * @dev Asserts if the given address is the null-address + * @dev Asserts that sender is the implementor. + */ + modifier onlyImplementor() { + require(isImplementor(), "Is not implementor"); + _; + } + + /** + * @dev Asserts that the given address is not the null-address */ modifier requireNonZero(address addr) { require(addr != address(0), "Expected a non-zero address"); diff --git a/test/token/ERC20/ExternalERC20Storage.test.js b/test/token/ERC20/ExternalERC20Storage.test.js index 07e8e6c..2c649b6 100644 --- a/test/token/ERC20/ExternalERC20Storage.test.js +++ b/test/token/ERC20/ExternalERC20Storage.test.js @@ -123,99 +123,99 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun }); describe('setBalance', function () { - const commonOwnerImplementorTests = function (ownerOrImplementorAddress) { - it('should allow changing balance', async function () { - const newBalance = 200; - - (await this.token.balances(anotherAccount)).should.not.be.bignumber.equal(newBalance); - await this.token.setBalance(anotherAccount, newBalance, { from: ownerOrImplementorAddress }); - (await this.token.balances(anotherAccount)).should.be.bignumber.equal(newBalance); - }); - }; + const newBalance = 200; describe('when is owner', function () { - commonOwnerImplementorTests(owner); + it('should not allow changing balance', async function () { + await utils.assertRevertsReason( + this.token.setBalance(anotherAccount, newBalance, { from: owner }), + 'Is not implementor'); + }); }); describe('when is implementor', function () { - commonOwnerImplementorTests(implementor); + it('should allow changing balance', async function () { + + (await this.token.balances(anotherAccount)).should.not.be.bignumber.equal(newBalance); + await this.token.setBalance(anotherAccount, newBalance, { from: implementor }); + (await this.token.balances(anotherAccount)).should.be.bignumber.equal(newBalance); + }); }); describe('when is not owner or implementor', function () { it('should not allow changing balance', async function () { - const newBalance = 200; const oldBalance = await this.token.balances(anotherAccount); oldBalance.should.not.be.bignumber.equal(newBalance); await utils.assertRevertsReason( this.token.setBalance(anotherAccount, newBalance, { from: thirdAccount }), - 'Is not implementor or owner'); + 'Is not implementor'); (await this.token.balances(anotherAccount)).should.be.bignumber.equal(oldBalance); }); }); }); describe('setAllowed', function () { - const commonOwnerImplementorTests = function (ownerOrImplementorAddress) { + const newAllowance = 200; + + describe('when is owner', function () { + it('should not allow changing allowance', async function () { + await utils.assertRevertsReason( + this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: owner }), + 'Is not implementor'); + }); + }); + + describe('when is implementor', function () { it('should allow changing allowance', async function () { - const newAllowance = 200; (await this.token.allowed(anotherAccount, thirdAccount)).should.not.be.bignumber.equal(newAllowance); - await this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: ownerOrImplementorAddress }); + await this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: implementor }); (await this.token.allowed(anotherAccount, thirdAccount)).should.be.bignumber.equal(newAllowance); }); - }; - - describe('when is owner', function () { - commonOwnerImplementorTests(owner); - }); - describe('when is implementor', function () { - commonOwnerImplementorTests(implementor); }); describe('when is not owner or implementor', function () { - it('should not allow changing balance', async function () { - const newAllowance = 200; + it('should not allow changing allowance', async function () { const oldAllowance = (await this.token.allowed(anotherAccount, thirdAccount)); oldAllowance.should.not.be.bignumber.equal(newAllowance); await utils.assertRevertsReason( this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: anotherAccount }), - 'Is not implementor or owner'); + 'Is not implementor'); (await this.token.allowed(anotherAccount, thirdAccount)).should.be.bignumber.equal(oldAllowance); }); }); }); describe('setTotalSupply', function () { - const commonOwnerImplementorTests = function (ownerOrImplementorAddress) { - it('should allow changing total supply', async function () { - const newTotalSupply = 200; - - (await this.token.totalSupply()).should.not.be.bignumber.equal(newTotalSupply); - await this.token.setTotalSupply(newTotalSupply, { from: ownerOrImplementorAddress }); - (await this.token.totalSupply()).should.be.bignumber.equal(newTotalSupply); - }); - }; + const newTotalSupply = 200; describe('when is owner', function () { - commonOwnerImplementorTests(owner); + it('should not allow changing total supply', async function () { + await utils.assertRevertsReason( + this.token.setTotalSupply(newTotalSupply, { from: owner }), + 'Is not implementor'); + }); }); describe('when is implementor', function () { - commonOwnerImplementorTests(implementor); + it('should allow changing total supply', async function () { + (await this.token.totalSupply()).should.not.be.bignumber.equal(newTotalSupply); + await this.token.setTotalSupply(newTotalSupply, { from: implementor }); + (await this.token.totalSupply()).should.be.bignumber.equal(newTotalSupply); + }); }); describe('when is not owner or implementor', function () { - it('should not allow changing balance', async function () { - const newTotalSupply = 200; + it('should not allow changing total supply', async function () { const oldTotalSupply = await this.token.totalSupply(); oldTotalSupply.should.not.be.bignumber.equal(newTotalSupply); await utils.assertRevertsReason( this.token.setTotalSupply(newTotalSupply, { from: thirdAccount }), - 'Is not implementor or owner'); + 'Is not implementor'); (await this.token.totalSupply()).should.be.bignumber.equal(oldTotalSupply); }); });