From 063917bde24b780d66bb5a75dd3d9be1ade8c5cb Mon Sep 17 00:00:00 2001 From: Truls Asheim Date: Fri, 11 Jan 2019 11:16:34 +0100 Subject: [PATCH] Minting token recipient (#51) * Owner should not be initial minter * Add minter recipient address restriction * Move mintExplicitSender function and checks to ERC20Mintable contract * Emit event when minting recipient change * Added event map for mintable * Reorder functions * Fix test env setup for restricted minter * Fix tests for restricted minter * Remove TODO * Add test case as requested by @peteremiljensen --- contracts/access/roles/MinterRole.sol | 4 - contracts/mocks/ExternalERC20MintableMock.sol | 15 +++- contracts/mocks/TokenXExplicitSenderMock.sol | 2 +- contracts/mocks/TokenXMock.sol | 3 +- .../token/ERC20/ExternalERC20Mintable.sol | 38 ++++++-- contracts/token/IUpgradableTokenX.sol | 4 + contracts/token/TokenX.sol | 11 +++ contracts/token/TokenXExplicitSender.sol | 26 ++++-- migrations/4_setup_accounts.js | 13 +-- test/UpgradeToken.test.js | 4 +- test/roles/MinterRole.test.js | 3 +- .../ERC20/ExternalERC20Mintable.events.js | 8 ++ .../token/ERC20/ExternalERC20Mintable.test.js | 10 ++- .../ERC20/behaviors/ERC20Mintable.behavior.js | 86 +++++++++++++++---- test/token/TokenX.test.js | 25 +++--- 15 files changed, 194 insertions(+), 58 deletions(-) create mode 100644 test/token/ERC20/ExternalERC20Mintable.events.js diff --git a/contracts/access/roles/MinterRole.sol b/contracts/access/roles/MinterRole.sol index 53ed2ec..82e1f45 100644 --- a/contracts/access/roles/MinterRole.sol +++ b/contracts/access/roles/MinterRole.sol @@ -11,10 +11,6 @@ contract MinterRole is Ownable { Roles.Role private minters; - constructor() internal { - _addMinter(msg.sender); - } - modifier onlyMinter() { require(isMinter(msg.sender), "not minter"); _; diff --git a/contracts/mocks/ExternalERC20MintableMock.sol b/contracts/mocks/ExternalERC20MintableMock.sol index 8010f08..2ea7695 100644 --- a/contracts/mocks/ExternalERC20MintableMock.sol +++ b/contracts/mocks/ExternalERC20MintableMock.sol @@ -3,10 +3,21 @@ pragma solidity ^0.4.24; import "../token/ERC20/ExternalERC20Mintable.sol"; import "./MinterRoleMock.sol"; -contract ExternalERC20MintableMock is ExternalERC20Mintable, MinterRoleMock { +contract ExternalERC20MintableMock is ExternalERC20Mintable, MinterRoleMock { - constructor() ExternalERC20(new ExternalERC20Storage()) public { + constructor(address initMintingRecipient) + ExternalERC20(new ExternalERC20Storage()) + ExternalERC20Mintable(initMintingRecipient) + public { _externalERC20Storage.transferImplementor(this); _externalERC20Storage.transferOwnership(msg.sender); } + + function mint(address to, uint256 amount) public { + _mintExplicitSender(msg.sender, to, amount); + } + + function changeMintingRecipient(address to) public { + _changeMintingRecipient(msg.sender, to); + } } diff --git a/contracts/mocks/TokenXExplicitSenderMock.sol b/contracts/mocks/TokenXExplicitSenderMock.sol index aa086f5..67ac9dc 100644 --- a/contracts/mocks/TokenXExplicitSenderMock.sol +++ b/contracts/mocks/TokenXExplicitSenderMock.sol @@ -17,6 +17,6 @@ contract TokenXExplicitSenderMock is TokenXExplicitSender { public TokenXExplicitSender( name, symbol, decimals, accesslist, whitelistEnabled, - stor, upgradedFrom, initialDeployment) + stor, address(0xf00f), upgradedFrom, initialDeployment) {} } diff --git a/contracts/mocks/TokenXMock.sol b/contracts/mocks/TokenXMock.sol index 797b21f..4f04797 100644 --- a/contracts/mocks/TokenXMock.sol +++ b/contracts/mocks/TokenXMock.sol @@ -15,6 +15,7 @@ contract TokenXMock is TokenX, PauserRoleMock { Accesslist accesslist, bool whitelistEnabled, ExternalERC20Storage stor, + address mintingRecip, IUpgradableTokenX upgradedFrom, bool initialDeployment, address initialAccount, @@ -22,7 +23,7 @@ contract TokenXMock is TokenX, PauserRoleMock { ) TokenX( name, symbol, decimals, - accesslist, whitelistEnabled, stor, upgradedFrom, + accesslist, whitelistEnabled, stor, mintingRecip, upgradedFrom, initialDeployment ) public diff --git a/contracts/token/ERC20/ExternalERC20Mintable.sol b/contracts/token/ERC20/ExternalERC20Mintable.sol index 3be8416..4408da7 100644 --- a/contracts/token/ERC20/ExternalERC20Mintable.sol +++ b/contracts/token/ERC20/ExternalERC20Mintable.sol @@ -8,21 +8,49 @@ import "../../access/roles/MinterRole.sol"; * @dev ERC20 minting logic */ contract ExternalERC20Mintable is ExternalERC20, MinterRole { + + address private mintingRecipientAccount; + + event MintingRecipientAccountChanged(address prev, address next); + + constructor(address _mintingRecipientAccount) internal { + _changeMintingRecipient(msg.sender, _mintingRecipientAccount); + } + + /** + * @dev Allows the owner to change the current minting recipient account + * @param _mintingRecipientAccount address of new minting recipient + */ + function _changeMintingRecipient( + address sender, + address _mintingRecipientAccount + ) + internal + { + require(owner() == sender, "is not owner"); + require(_mintingRecipientAccount != address(0), + "zero minting recipient"); + address prev = mintingRecipientAccount; + mintingRecipientAccount = _mintingRecipientAccount; + emit MintingRecipientAccountChanged(prev, mintingRecipientAccount); + } + /** * @dev Function to mint tokens * @param to The address that will receive the minted tokens. * @param value The amount of tokens to mint. * @return A boolean that indicates if the operation was successful. */ - function mint( + function _mintExplicitSender( + address sender, address to, uint256 value ) - public - onlyMinter - returns (bool) + internal + requireMinter(sender) { + require(to == mintingRecipientAccount, + "not minting to mintingRecipientAccount"); _mint(to, value); - return true; } } diff --git a/contracts/token/IUpgradableTokenX.sol b/contracts/token/IUpgradableTokenX.sol index 7fd4093..87143c2 100644 --- a/contracts/token/IUpgradableTokenX.sol +++ b/contracts/token/IUpgradableTokenX.sol @@ -59,6 +59,10 @@ contract IUpgradableTokenX { public returns (bool); + function changeMintingRecipientExplicitSender(address sender, + address mintingRecip) + public; + function burnExplicitSender(address sender, uint256 value) public; function burnFromExplicitSender(address sender, diff --git a/contracts/token/TokenX.sol b/contracts/token/TokenX.sol index 9cd23df..2d99180 100644 --- a/contracts/token/TokenX.sol +++ b/contracts/token/TokenX.sol @@ -18,6 +18,7 @@ contract TokenX is ITokenX, TokenXExplicitSender { Accesslist accesslist, bool whitelistEnabled, ExternalERC20Storage externalERC20Storage, + address mintingRecipientAccount, address upgradedFrom, bool initialDeployment ) @@ -29,6 +30,7 @@ contract TokenX is ITokenX, TokenXExplicitSender { accesslist, whitelistEnabled, externalERC20Storage, + mintingRecipientAccount, upgradedFrom, initialDeployment ) { @@ -196,6 +198,14 @@ contract TokenX is ITokenX, TokenXExplicitSender { } } + function changeMintingRecipient(address mintingRecip) public { + if (isUpgraded()) { + upgradedToken.changeMintingRecipientExplicitSender(msg.sender, mintingRecip); + } else { + super.changeMintingRecipient(mintingRecip); + } + } + function pause () public { if (isUpgraded()) { revert("Token is upgraded. Call pause from new token."); @@ -211,4 +221,5 @@ contract TokenX is ITokenX, TokenXExplicitSender { super.unpause(); } } + } diff --git a/contracts/token/TokenXExplicitSender.sol b/contracts/token/TokenXExplicitSender.sol index 49854ec..40a205b 100644 --- a/contracts/token/TokenXExplicitSender.sol +++ b/contracts/token/TokenXExplicitSender.sol @@ -18,9 +18,9 @@ import "./IUpgradableTokenX.sol"; contract TokenXExplicitSender is IUpgradableTokenX, ExternalERC20, ExternalERC20Burnable, + ExternalERC20Mintable, ERC20Detailed, AccesslistGuarded, - MinterRole, BurnerRole, Pausable { @@ -54,11 +54,13 @@ contract TokenXExplicitSender is IUpgradableTokenX, Accesslist accesslist, bool whitelistEnabled, ExternalERC20Storage externalERC20Storage, + address mintingRecipientAccount, address upgradedFrom, bool initialDeployment ) internal ExternalERC20(externalERC20Storage) + ExternalERC20Mintable(mintingRecipientAccount) ERC20Detailed(name, symbol, decimals) AccesslistGuarded(accesslist, whitelistEnabled) { @@ -282,13 +284,20 @@ contract TokenXExplicitSender is IUpgradableTokenX, public isEnabled senderIsProxy - requireMinter(sender) returns (bool success) { - super._mint(to, value); + super._mintExplicitSender(sender, to, value); return true; } + function changeMintingRecipientExplicitSender(address sender, address mintingRecip) + public + isEnabled + senderIsProxy + { + super._changeMintingRecipient(sender, mintingRecip); + } + function transfer(address to, uint256 value) public isEnabled @@ -356,12 +365,19 @@ contract TokenXExplicitSender is IUpgradableTokenX, function mint(address to, uint256 value) public isEnabled - onlyMinter returns (bool) { - super._mint(to, value); + super._mintExplicitSender(msg.sender, to, value); return true; } + function changeMintingRecipient(address _mintingRecipientAddress) + public + isEnabled + onlyOwner + { + super._changeMintingRecipient(msg.sender, _mintingRecipientAddress); + } + } diff --git a/migrations/4_setup_accounts.js b/migrations/4_setup_accounts.js index 5dabe8f..b85f9cf 100644 --- a/migrations/4_setup_accounts.js +++ b/migrations/4_setup_accounts.js @@ -12,7 +12,7 @@ module.exports = function (deployer, _network, accounts) { } }; -async function setupAccounts ([owner, whitelistAdmin, whitelisted, ...restAccounts]) { +async function setupAccounts ([owner, whitelistAdmin, whitelisted, minter, ...restAccounts]) { /* The purpose of this is to automatically setup the test environment accounts. DO NOT use in production yet. @@ -20,6 +20,9 @@ async function setupAccounts ([owner, whitelistAdmin, whitelisted, ...restAccoun const intialMintValue = 100; + // FIXME: Use some real address here + const mintTargetAccount = 0xd00f; + // Setup whitelists const accesslistContract = await Accesslist.deployed(); @@ -50,16 +53,16 @@ async function setupAccounts ([owner, whitelistAdmin, whitelisted, ...restAccoun const token = await TokenX.new( td.name, td.symbol, td.decimals, accesslistContract.address, td.whitelistEnabled, externalERC20Storage.address, - 0, true, + mintTargetAccount, 0, true, { from: owner }); + token.addMinter(minter, { from: owner }); await tokenManagerContract.addToken(td.name, token.address); return token; - }) - ); + })); // Mint tokens await Promise.all( - tokens.map((t) => t.mint(owner, intialMintValue, { from: owner })) + tokens.map((t) => t.mint(mintTargetAccount, intialMintValue, { from: minter })) ); } diff --git a/test/UpgradeToken.test.js b/test/UpgradeToken.test.js index 97de8d0..621bed6 100644 --- a/test/UpgradeToken.test.js +++ b/test/UpgradeToken.test.js @@ -32,14 +32,14 @@ contract('Upgrade Token', async function ([_, tokenManagerOwner, oldTokenOwner, const oldToken = await TokenX.new( tokenName, 'e', oldTokenDecimals, - accesslist.address, true, storage.address, 0, true, + accesslist.address, true, storage.address, 0xf00f, 0, true, { from: oldTokenOwner } ); const newToken = await TokenX.new( tokenName, 'e', newTokenDecimals, accesslist.address, true, - storage.address, oldToken.address, false, + storage.address, 0xf00f, oldToken.address, false, { from: newTokenOwner } ); diff --git a/test/roles/MinterRole.test.js b/test/roles/MinterRole.test.js index 343d713..5419cc7 100644 --- a/test/roles/MinterRole.test.js +++ b/test/roles/MinterRole.test.js @@ -7,7 +7,8 @@ const MinterRoleMock = artifacts.require('MinterRoleMock'); contract('MinterRole', function ([_, minter, otherMinter, ...otherAccounts]) { beforeEach(async function () { this.contract = await MinterRoleMock.new({ from: minter }); - await this.contract.addMinter(otherMinter, { from: minter }); + [minter, otherMinter].forEach( + async (x) => this.contract.addMinter(x, { from: minter })); }); shouldBehaveLikePublicRole(minter, otherMinter, otherAccounts, 'minter'); diff --git a/test/token/ERC20/ExternalERC20Mintable.events.js b/test/token/ERC20/ExternalERC20Mintable.events.js new file mode 100644 index 0000000..3fe39b8 --- /dev/null +++ b/test/token/ERC20/ExternalERC20Mintable.events.js @@ -0,0 +1,8 @@ +const utils = require('./../..//utils.js'); + +module.exports = utils.makeEventMap({ + changeMintingRecipient: (prevAddr, nextAddr) => [ + { eventName: 'MintingRecipientAccountChanged', + paramMap: { prev: prevAddr, + next: nextAddr } }] +}); diff --git a/test/token/ERC20/ExternalERC20Mintable.test.js b/test/token/ERC20/ExternalERC20Mintable.test.js index 5940596..191008e 100644 --- a/test/token/ERC20/ExternalERC20Mintable.test.js +++ b/test/token/ERC20/ExternalERC20Mintable.test.js @@ -9,20 +9,22 @@ const ExternalERC20MintableMock = artifacts.require('ExternalERC20MintableMock') const { shouldBehaveLikePublicRole } = require('../../roles/behaviors/PublicRole.behavior'); contract('ExternalERC20Mintable', function ([_, minter, otherMinter, ...otherAccounts]) { + const mintingRecipientAccount = '0x000000000000000000000000000000000000d00f'; beforeEach(async function () { - this.token = await ExternalERC20MintableMock.new({ from: minter }); + this.token = await ExternalERC20MintableMock.new(mintingRecipientAccount, { from: minter }); + this.token.addMinter(minter, { from: minter }); }); describe('minter role', function () { beforeEach(async function () { this.contract = this.token; - await this.contract.addMinter(otherMinter, { from: minter }); + this.token.addMinter(otherMinter, { from: minter }); }); shouldBehaveLikePublicRole(minter, otherMinter, otherAccounts, 'minter'); }); - shouldBehaveLikeERC20Mintable(minter, otherAccounts); + shouldBehaveLikeERC20Mintable(minter, minter, otherAccounts, mintingRecipientAccount); describe('When sharing storage', function () { beforeEach(async function () { @@ -37,7 +39,7 @@ contract('ExternalERC20Mintable', function ([_, minter, otherMinter, ...otherAcc (await this.token.totalSupply()).should.be.bignumber.equal(0); (await this.token2.totalSupply()).should.be.bignumber.equal(0); - await this.token.mint(minter, 100, { from: minter }); + await this.token.mint(mintingRecipientAccount, 100, { from: minter }); (await this.token.totalSupply()).should.be.bignumber.equal(100); (await this.token2.totalSupply()).should.be.bignumber.equal(100); diff --git a/test/token/ERC20/behaviors/ERC20Mintable.behavior.js b/test/token/ERC20/behaviors/ERC20Mintable.behavior.js index 2b537fb..e703e78 100644 --- a/test/token/ERC20/behaviors/ERC20Mintable.behavior.js +++ b/test/token/ERC20/behaviors/ERC20Mintable.behavior.js @@ -11,39 +11,89 @@ require('chai') .use(require('chai-bignumber')(BigNumber)) .should(); -function shouldBehaveLikeERC20Mintable (minter, [anyone]) { +function shouldBehaveLikeERC20Mintable (owner, minter, [anyone], mintingRecip) { + function shouldMint (amount, recip) { + const from = minter; + beforeEach(async function () { + ({ logs: this.logs } = await this.token.mint(recip, amount, { from })); + }); + + it('mints the requested amount', async function () { + (await this.token.balanceOf(recip)).should.be.bignumber.equal(amount); + }); + + it('emits a mint and a transfer event', async function () { + expectEvent.inLogs(this.logs, 'Transfer', { + from: ZERO_ADDRESS, + to: recip, + value: amount + }); + }); + } + describe('as a mintable token', function () { describe('mint', function () { const amount = 100; context('when the sender has minting permission', function () { - const from = minter; - context('for a zero amount', function () { - shouldMint(0); + shouldMint(0, mintingRecip); }); context('for a non-zero amount', function () { - shouldMint(amount); + shouldMint(amount, mintingRecip); }); + }); - function shouldMint (amount) { - beforeEach(async function () { - ({ logs: this.logs } = await this.token.mint(anyone, amount, { from })); - }); + context('when minting to a disallowed recipient', function () { + it('reverts', async function () { + await shouldFail.reverting( + this.token.mint(anyone, amount, { from: minter })); + }); + }); - it('mints the requested amount', async function () { - (await this.token.balanceOf(anyone)).should.be.bignumber.equal(amount); + context('when changing recipient to zero address', function () { + it('reverts', async function () { + await shouldFail.reverting( + this.token.changeMintingRecipient(0, { from: minter })); + }); + }); + + context('when sender is not owner', function () { + it('reverts', async function () { + await shouldFail.reverting( + this.token.changeMintingRecipient(anyone, { from: anyone })); + }); + }); + + it('reverts when changing recipient ', async function () { + await shouldFail.reverting( + this.token.changeMintingRecipient(0, { from: minter })); + }); + + context('when changing minting recipient', async function () { + const newMintingRecip = '0x000000000000000000000000000000000000d00e'; + + beforeEach(async function () { + ({ logs: this.logs2 } = + await this.token.changeMintingRecipient(newMintingRecip, { from: owner })); + }); + + shouldMint(amount, newMintingRecip); + + describe('when minting to previous recipient', function () { + it('reverts', async function () { + await shouldFail.reverting( + this.token.mint(mintingRecip, amount, { from: minter })); }); + }); - it('emits a mint and a transfer event', async function () { - expectEvent.inLogs(this.logs, 'Transfer', { - from: ZERO_ADDRESS, - to: anyone, - value: amount - }); + it('emits a minting recipient changed event', async function () { + expectEvent.inLogs(this.logs2, 'MintingRecipientAccountChanged', { + prev: mintingRecip, + next: newMintingRecip }); - } + }); }); context('when the sender doesn\'t have minting permission', function () { diff --git a/test/token/TokenX.test.js b/test/token/TokenX.test.js index ef78888..9f307c8 100644 --- a/test/token/TokenX.test.js +++ b/test/token/TokenX.test.js @@ -38,7 +38,8 @@ const explicitSenderOps = [ ['increaseAllowanceExplicitSender', [util.ZERO_ADDRESS, util.ZERO_ADDRESS, 0]], ['decreaseAllowanceExplicitSender', [util.ZERO_ADDRESS, util.ZERO_ADDRESS, 0]], ['burnExplicitSender', [util.ZERO_ADDRESS, 0]], - ['burnFromExplicitSender', [util.ZERO_ADDRESS, util.ZERO_ADDRESS, 0]] + ['burnFromExplicitSender', [util.ZERO_ADDRESS, util.ZERO_ADDRESS, 0]], + ['changeMintingRecipientExplicitSender', [util.ZERO_ADDRESS, util.ZERO_ADDRESS]] ]; const otherOps = [ ['transfer', [util.ZERO_ADDRESS, 0]], @@ -47,7 +48,9 @@ const otherOps = [ ['increaseAllowance', [util.ZERO_ADDRESS, 0]], ['decreaseAllowance', [util.ZERO_ADDRESS, 0]], ['burn', [0]], - ['burnFrom', [util.ZERO_ADDRESS, 0]] + ['burnFrom', [util.ZERO_ADDRESS, 0]], + ['mint', [util.ZERO_ADDRESS, 0]], + ['changeMintingRecipient', [util.ZERO_ADDRESS]] ]; function unopgradedTokenBehavior () { @@ -196,6 +199,8 @@ contract('TokenX', async function ( const symbolOrig = 'e'; const symbolUpgraded = 'f'; + const mintingRecipientAccount = '0x000000000000000000000000000000000000d00f'; + describe('constructor', function () { let storage; @@ -206,7 +211,7 @@ contract('TokenX', async function ( it('reverts when both upgradedFrom and initialDeployment are set', async function () { await util.assertRevertsReason( TokenXMock.new(tokNameOrig, symbolOrig, 10, - 0xf00f, true, storage.address, + 0xf00f, true, storage.address, mintingRecipientAccount, 0xf00f, true, owner, 100, { from: owner }), 'Cannot both be upgraded and initial deployment.'); }); @@ -214,7 +219,7 @@ contract('TokenX', async function ( it('reverts when niether upgradedFrom or initialDeployment are set', async function () { await util.assertRevertsReason( TokenXMock.new(tokNameOrig, symbolOrig, 10, - 0xf00f, true, storage.address, + 0xf00f, true, storage.address, mintingRecipientAccount, 0, false, owner, 100, { from: owner }), 'Cannot both be upgraded and initial deployment.'); }); @@ -233,13 +238,13 @@ contract('TokenX', async function ( storage = await ExternalERC20Storage.new({ from: owner }); token = await TokenXMock.new(tokNameOrig, symbolOrig, 10, - accesslist.address, true, storage.address, + accesslist.address, true, storage.address, mintingRecipientAccount, 0, true, owner, 100, { from: owner }); tokenE = TokenXE.wrap(token); upgradeToken = await TokenXMock.new( tokNameUpgraded, symbolUpgraded, 20, - accesslist.address, true, storage.address, token.address, false, 0, 0, - { from: owner }); + accesslist.address, true, storage.address, mintingRecipientAccount, + token.address, false, 0, 0, { from: owner }); [token, upgradeToken].forEach(async function (f) { await f.addMinter(minter, { from: owner }); await f.addPauser(pauser, { from: owner }); @@ -273,7 +278,7 @@ contract('TokenX', async function ( describe('Original token', function () { unopgradedTokenBehavior(); shouldBehaveLikeERC20PublicAPI(owner, whitelisted, whitelisted1); - shouldBehaveLikeERC20Mintable(minter, [user]); + shouldBehaveLikeERC20Mintable(owner, minter, [user], mintingRecipientAccount); shouldBehaveLikeERC20Burnable(owner, 100, [burner]); shouldBehaveLikeERC20Pausable(owner, otherPauser, whitelisted, whitelisted1, @@ -363,7 +368,7 @@ contract('TokenX', async function ( }); shouldBehaveLikeERC20PublicAPI(owner, whitelisted, whitelisted1); - shouldBehaveLikeERC20Mintable(minter, [user]); + shouldBehaveLikeERC20Mintable(owner, minter, [user], mintingRecipientAccount); shouldBehaveLikeERC20Burnable(owner, 100, [burner]); shouldBehaveLikeERC20Pausable(owner, otherPauser, whitelisted, whitelisted1, @@ -395,7 +400,7 @@ contract('TokenX', async function ( this.newToken = undefined; }); shouldBehaveLikeERC20PublicAPI(owner, whitelisted, whitelisted1); - shouldBehaveLikeERC20Mintable(minter, [user]); + shouldBehaveLikeERC20Mintable(owner, minter, [user], mintingRecipientAccount); shouldBehaveLikeERC20Burnable(owner, 100, [burner]); proxyPausableBehavior(); ERC20Permissions(owner, whitelisted, user, user1,