removeController reverts if controllers[C] is 0 (#273)

* removeController throws if worker is already zero-address * remove a bunch of unnecessary periods for consistency, fix broken tests
circlefin · Jan 16, 2019 · 7ad74d7 · 7ad74d7
1 parent 43b5f74
commit 7ad74d7
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 35 deletions.
diff --git a/.gitignore b/.gitignore
@@ -12,3 +12,4 @@ token.json
 credentials.json
 echidna/
 validate/apikey.infura
+ganache-blockchain-log.txt
diff --git a/contracts/minting/Controller.sol b/contracts/minting/Controller.sol
@@ -45,7 +45,7 @@ contract Controller is Ownable {
      */
     modifier onlyController() {
         require(controllers[msg.sender] != address(0), 
-            "The value of controllers[msg.sender] must be non-zero.");
+            "The value of controllers[msg.sender] must be non-zero");
         _;
     }
 
@@ -68,7 +68,7 @@ contract Controller is Ownable {
         returns (bool)
     {
         require(_controller != address(0), "Controller must be a non-zero address");
-        require(_worker != address(0), "Worker must be a non-zero address.");
+        require(_worker != address(0), "Worker must be a non-zero address");
         controllers[_controller] = _worker;
         emit ControllerConfigured(_controller, _worker);
         return true;
@@ -85,6 +85,7 @@ contract Controller is Ownable {
         returns (bool)
     {
         require(_controller != address(0), "Controller must be a non-zero address");
+        require(controllers[_controller] != address(0), "Worker must be a non-zero address");
         controllers[_controller] = address(0);
         emit ControllerRemoved(_controller);
         return true;

diff --git a/contracts/minting/MintController.sol b/contracts/minting/MintController.sol
@@ -117,10 +117,10 @@ contract MintController is Controller {
         onlyController
         returns (bool)
     {
-        require(_allowanceIncrement > 0, "Allowance increment must be greater than 0.");
+        require(_allowanceIncrement > 0, "Allowance increment must be greater than 0");
         address minter = controllers[msg.sender];
         require(minterManager.isMinter(minter), 
-            "Can only increment allowance for minters in minterManager.");
+            "Can only increment allowance for minters in minterManager");
 
         uint256 currentAllowance = minterManager.minterAllowance(minter);
         uint256 newAllowance = currentAllowance.add(_allowanceIncrement);

diff --git a/test/minting/MintP0_ArgumentTests.js b/test/minting/MintP0_ArgumentTests.js
@@ -4,7 +4,7 @@ var MintController = artifacts.require('minting/MintController');
 var MasterMinter = artifacts.require('minting/MasterMinter');
 var FiatToken = artifacts.require('FiatTokenV1');
 
-var tokenUtils = require('./../TokenTestUtils.js');
+var tokenUtils = require('../TokenTestUtils.js');
 var newBigNumber = tokenUtils.newBigNumber;
 var checkMINTp0 = tokenUtils.checkMINTp0;
 var expectRevert = tokenUtils.expectRevert;
@@ -15,8 +15,8 @@ var maxAmount = tokenUtils.maxAmount;
 
 var clone = require('clone');
 
-var mintUtils = require('./../MintControllerUtils.js');
-var AccountUtils = require('./../AccountUtils.js');
+var mintUtils = require('../MintControllerUtils.js');
+var AccountUtils = require('../AccountUtils.js');
 var Accounts = AccountUtils.Accounts;
 var getAccountState = AccountUtils.getAccountState;
 var MintControllerState = AccountUtils.MintControllerState;
@@ -83,8 +83,9 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 
     it('arg007 removeController(0) throws', async function () {
-        await expectError(mintController.removeController(zeroAddress, {from: Accounts.mintOwnerAccount}),
-        "Controller must be a non-zero address");
+        // expect no changes
+        await expectError(mintController.removeController(zeroAddress, {from: Accounts.mintOwnerAccount}), "Controller must be a non-zero address");
+        await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
     });
 
     it('arg008 setMinterManager(0) works', async function () {
@@ -188,11 +189,20 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 
     it('arg020 removeController(C) works', async function() {
-        // expect no changes
-        await mintController.removeController(Accounts.controller1Account, {from: Accounts.mintOwnerAccount});
+        // make controller1Account a controller
+        await mintController.configureController(Accounts.controller1Account, Accounts.minterAccount, {from: Accounts.mintOwnerAccount});
+        var actualMinter = await mintController.controllers(Accounts.controller1Account);
+        addressEquals(Accounts.minterAccount, actualMinter);
+
+        // remove controller1Account
+        await mintController.removeController(Accounts.controller1Account, {from : Accounts.mintOwnerAccount});
         await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
+        actualMinter = await mintController.controllers(Accounts.controller1Account);
+        addressEquals(actualMinter, zeroAddress);
+    });
 
-        // now make controller1Account a controller
+    it('arg021 removeController throws if worker is already address(0)', async function () {
+        // make controller1Account a controller
         await mintController.configureController(Accounts.controller1Account, Accounts.minterAccount, {from: Accounts.mintOwnerAccount});
         var actualMinter = await mintController.controllers(Accounts.controller1Account);
         addressEquals(Accounts.minterAccount, actualMinter);
@@ -202,7 +212,11 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
         await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
         actualMinter = await mintController.controllers(Accounts.controller1Account);
         addressEquals(actualMinter, zeroAddress);
-    });
+
+        // attempting to remove the controller1Account again should throw because the worker is already set to address(0).
+        await expectError(mintController.removeController(Accounts.controller1Account, {from: Accounts.mintOwnerAccount}),
+            "Worker must be a non-zero address");
+    })
 }
 
 var testWrapper = require('./../TestWrapper');

diff --git a/test/minting2/MintControllerTests.js b/test/minting2/MintControllerTests.js
@@ -96,11 +96,11 @@ async function run_tests(newToken, accounts) {
    });
 
    it('only controller removes a minter', async function () {
-        await expectError(mintController.removeMinter({from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.removeMinter({from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
    });
 
    it('only controller configures a minter', async function () {
-        await expectError(mintController.configureMinter(0, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.configureMinter(0, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
    });
 
    it('increment minter allowance', async function () {
@@ -126,7 +126,7 @@ async function run_tests(newToken, accounts) {
    });
 
    it('only controller increments allowance', async function () {
-        await expectError(mintController.incrementMinterAllowance(0, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.incrementMinterAllowance(0, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
    });
 
    it('only active minters can have allowance incremented', async function () {
@@ -137,11 +137,11 @@ async function run_tests(newToken, accounts) {
         await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
 
         // increment minter allowance
-        await expectError(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}), "Can only increment allowance for minters in minterManager.");
+        await expectError(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}), "Can only increment allowance for minters in minterManager");
    });
 }
 
-var testWrapper = require('./../TestWrapper');
+var testWrapper = require('../TestWrapper');
 testWrapper.execute('MintController_Tests', run_tests);
 
 module.exports = {

diff --git a/test/minting2/MintP0_ABITests.js b/test/minting2/MintP0_ABITests.js
@@ -2,7 +2,7 @@ var MintController = artifacts.require('minting/MintController');
 var MasterMinter = artifacts.require('minting/MasterMinter');
 var FiatToken = artifacts.require('FiatTokenV1');
 
-var tokenUtils = require('./../TokenTestUtils.js');
+var tokenUtils = require('../TokenTestUtils.js');
 var checkMINTp0 = tokenUtils.checkMINTp0;
 var expectError = tokenUtils.expectError;
 var bigZero = tokenUtils.bigZero;
@@ -11,16 +11,16 @@ var solidityErrors = tokenUtils.solidityErrors;
 
 var clone = require('clone');
 
-var mintUtils = require('./../MintControllerUtils.js');
-var AccountUtils = require('./../AccountUtils.js');
+var mintUtils = require('../MintControllerUtils.js');
+var AccountUtils = require('../AccountUtils.js');
 var Accounts = AccountUtils.Accounts;
 var AccountPrivateKeys = AccountUtils.AccountPrivateKeys;
 var getAccountState = AccountUtils.getAccountState;
 var MintControllerState = AccountUtils.MintControllerState;
 var initializeTokenWithProxyAndMintController = mintUtils.initializeTokenWithProxyAndMintController;
 var checkMintControllerState = mintUtils.checkMintControllerState;
 
-var abiUtils = require('./../ABIUtils.js');
+var abiUtils = require('../ABIUtils.js');
 var makeRawTransaction = abiUtils.makeRawTransaction;
 var sendRawTransaction = abiUtils.sendRawTransaction;
 var msgData = abiUtils.msgData;

diff --git a/test/minting3/MintP0_BasicTests.js b/test/minting3/MintP0_BasicTests.js
@@ -2,7 +2,7 @@ var MintController = artifacts.require('minting/MintController');
 var MasterMinter = artifacts.require('minting/MasterMinter');
 var FiatToken = artifacts.require('FiatTokenV1');
 
-var tokenUtils = require('./../TokenTestUtils.js');
+var tokenUtils = require('../TokenTestUtils.js');
 var newBigNumber = tokenUtils.newBigNumber;
 var checkMINTp0 = tokenUtils.checkMINTp0;
 var expectRevert = tokenUtils.expectRevert;
@@ -13,8 +13,8 @@ var maxAmount = tokenUtils.maxAmount;
 
 var clone = require('clone');
 
-var mintUtils = require('./../MintControllerUtils.js');
-var AccountUtils = require('./../AccountUtils.js');
+var mintUtils = require('../MintControllerUtils.js');
+var AccountUtils = require('../AccountUtils.js');
 var Accounts = AccountUtils.Accounts;
 var getAccountState = AccountUtils.getAccountState;
 var MintControllerState = AccountUtils.MintControllerState;
@@ -105,7 +105,7 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 
     it('bt010 removeMinter reverts when msg.sender is not a controller', async function () {
-        await expectError(mintController.removeMinter({from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.removeMinter({from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
     });
 
     it('bt011 removeMinter sets minters[M] to 0', async function () {
@@ -129,7 +129,7 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 
     it('bt012 configureMinter reverts when msg.sender is not a controller', async function () {
-        await expectError(mintController.configureMinter(50, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.configureMinter(50, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
     });
 
     it('bt013 configureMinter works when controllers[msg.sender]=M', async function () {
@@ -148,7 +148,7 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 
     it('bt014 incrementMinterAllowance reverts if msg.sender is not a controller', async function () {
-        await expectError(mintController.incrementMinterAllowance(50, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero.");
+        await expectError(mintController.incrementMinterAllowance(50, {from: Accounts.controller1Account}), "The value of controllers[msg.sender] must be non-zero");
     });
 
     it('bt015 incrementMinterAllowance works when controllers[msg.sender]=M', async function () {
@@ -171,9 +171,9 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
          await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
     });
 
-    it('bt017 removeController does not revert when controllers[C] is 0', async function () {
+    it('bt017 removeController reverts when controllers[C] is 0', async function () {
         //  "remove" a controller that does not exist
-        await mintController.removeController(Accounts.controller1Account, {from: Accounts.mintOwnerAccount});
+        await expectError(mintController.removeController(Accounts.controller1Account, {from: Accounts.mintOwnerAccount}), "Worker must be a non-zero address");        
         await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
     });
 
@@ -414,7 +414,7 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
         var isMinter = await minterManager.isMinter(Accounts.minterAccount);
         assert.isFalse(isMinter);
 
-        await expectError(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}), "Can only increment allowance for minters in minterManager.");
+        await expectError(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}), "Can only increment allowance for minters in minterManager");
         expectedMintControllerState.controllers['controller1Account'] = Accounts.minterAccount;
         await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);
     });
@@ -607,6 +607,6 @@ async function run_MINT_tests(newToken, MintControllerArtifact, accounts) {
     });
 }
 
-var testWrapper = require('./../TestWrapper');
+var testWrapper = require('../TestWrapper');
 testWrapper.execute('MINTp0_BasicTests MintController', run_tests_MintController);
 testWrapper.execute('MINTp0_BasicTests MasterMinter', run_tests_MasterMinter);
diff --git a/verification/Spreadsheets/MINTp0_ArgumentTests.csv b/verification/Spreadsheets/MINTp0_ArgumentTests.csv
@@ -6,7 +6,7 @@ Controller.sol,configureController,newController == 0,arg003,"arg003 configureCo
 Controller.sol,configureController,newController == msg.sender,arg004,"arg004 configureController(msg.sender, M) works"
 Controller.sol,configureController,newController == newMinter,arg005,"arg005 configureController(M, M) works"
 Controller.sol,configureController,newMinter == 0,arg006,"arg006 configureController(C, 0) throws"
-Controller.sol,removeController,newController == 0,arg007,arg007 removeController(0) throws
+Controller.sol,removeController,controller == 0,arg007,arg007 removeController(0) throws
 MintController.sol,setMinterManager,newMinterManager = 0,arg008,arg008 setMinterManager(0) works
 MintController.sol,setMinterManager,newMinterManager == minterManager,arg009,arg009 setMinterManager(oldMinterManager) works
 MintController.sol,setMinterManager,newMinterManager == user,arg010,arg010 setMinterManager(user_account) works
@@ -19,4 +19,5 @@ MintController.sol,incrementMinterAllowance,increment == oldAllowance,arg016,arg
 MintController.sol,incrementMinterAllowance,increment == 2^256-1,arg017,arg017 incrementMinterAllowance(MAX) throws
 MintController.sol,incrementMinterAllowance,increment + oldAllowance == 2^256 -1,arg018,arg018 incrementMinterAlllowance(BIG) throws
 Controller.sol,configureController,newController == 0,arg019,"arg019 configureController(0, 0) throws"
-Controller.sol,removeController,newController == msg.sender,arg020,arg020 removeController(C) works
+Controller.sol,removeController,controller == msg.sender,arg020,arg020 removeController(C) works
+Controller.sol,removeController,controller where controllers[controller] == 0,arg021,arg021 removeController throws if worker is already address(0)
diff --git a/verification/Spreadsheets/MINTp0_BasicTests.csv b/verification/Spreadsheets/MINTp0_BasicTests.csv
@@ -9,7 +9,7 @@ Controller.sol,configureController,controllers[C] == newM,bt023,"bt023 configure
 Controller.sol,constructor,controllers[ALL] = 0,bt016,bt016 Constructor sets all controllers to 0
 Controller.sol,removeController,msg.sender == owner,bt008,bt008 removeController works when owner is msg.sender
 Controller.sol,removeController,msg.sender != owner,bt009,bt009 removeController reverts when owner is not msg.sender
-Controller.sol,removeController,controllers[C]=0,bt017,bt017 removeController does not revert when controllers[C] is 0
+Controller.sol,removeController,controllers[C]=0,bt017,bt017 removeController reverts when controllers[C] is 0
 Controller.sol,removeController,controllers[C]=M,bt018,bt018 removeController removes an arbitrary controller
 FiatTokenV1.sol,constructor,minterManager.isMinter[ALL] == false,bt045,bt045 constructor - minterManager.isMinter[ALL] is false
 FiatTokenV1.sol,constructor,minterManager.minterAllowance[ALL] == 0,bt046,bt046 constructor - minterManager.minterAllowance[ALL] = 0