Add modifier and tests for removeAuthorizedAddressAtIndex

3 files changed, 79 insertions, 2 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
index 8cb4254c5..37c12f861 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinAuthorizable.sol
@@ -69,7 +69,7 @@ contract MixinAuthorizable is
         );
 
         delete authorized[target];
-        for (uint i = 0; i < authorities.length; i++) {
+        for (uint256 i = 0; i < authorities.length; i++) {
             if (authorities[i] == target) {
                 authorities[i] = authorities[authorities.length - 1];
                 authorities.length -= 1;
@@ -87,8 +87,13 @@ contract MixinAuthorizable is
         uint256 index
     )
         external
+        onlyOwner
     {
         require(
+            authorized[target],
+            TARGET_NOT_AUTHORIZED
+        );
+        require(
             index < authorities.length,
             INDEX_OUT_OF_BOUNDS
         );
diff --git a/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol b/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
index 296c6c856..99b93d0d2 100644
--- a/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
+++ b/packages/contracts/src/contracts/current/utils/Ownable/Ownable.sol
@@ -13,6 +13,9 @@ import "./IOwnable.sol";
 contract Ownable is IOwnable {
     address public owner;
 
+    // Revert reasons
+    string constant ONLY_CONTRACT_OWNER = "ONLY_CONTRACT_OWNER";
+
     constructor ()
         public
     {
@@ -22,7 +25,7 @@ contract Ownable is IOwnable {
     modifier onlyOwner() {
         require(
             msg.sender == owner,
-            "Only contract owner is allowed to call this method."
+            ONLY_CONTRACT_OWNER
         );
         _;
     }
diff --git a/packages/contracts/test/asset_proxy/authorizable.ts b/packages/contracts/test/asset_proxy/authorizable.ts
index ed582b63e..c35dc7882 100644
--- a/packages/contracts/test/asset_proxy/authorizable.ts
+++ b/packages/contracts/test/asset_proxy/authorizable.ts
@@ -1,4 +1,5 @@
 import { BlockchainLifecycle } from '@0xproject/dev-utils';
+import { BigNumber } from '@0xproject/utils';
 import * as chai from 'chai';
 
 import { MixinAuthorizableContract } from '../../src/generated_contract_wrappers/mixin_authorizable';
@@ -102,6 +103,74 @@ describe('Authorizable', () => {
         });
     });
 
+    describe('removeAuthorizedAddressAtIndex', () => {
+        it('should throw if not called by owner', async () => {
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.addAuthorizedAddress.sendTransactionAsync(address, { from: owner }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            const index = new BigNumber(0);
+            return expectRevertOrAlwaysFailingTransactionAsync(
+                authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, {
+                    from: notOwner,
+                }),
+            );
+        });
+        it('should throw if index is >= authorities.length', async () => {
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.addAuthorizedAddress.sendTransactionAsync(address, { from: owner }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            const index = new BigNumber(1);
+            return expectRevertOrAlwaysFailingTransactionAsync(
+                authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, {
+                    from: owner,
+                }),
+            );
+        });
+        it('should throw if owner attempts to remove an address that is not authorized', async () => {
+            const index = new BigNumber(0);
+            return expectRevertOrAlwaysFailingTransactionAsync(
+                authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, {
+                    from: owner,
+                }),
+            );
+        });
+        it('should throw if address at index does not match target', async () => {
+            const address1 = address;
+            const address2 = notOwner;
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.addAuthorizedAddress.sendTransactionAsync(address1, { from: owner }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.addAuthorizedAddress.sendTransactionAsync(address2, { from: owner }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            const address1Index = new BigNumber(0);
+            return expectRevertOrAlwaysFailingTransactionAsync(
+                authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address2, address1Index, {
+                    from: owner,
+                }),
+            );
+        });
+        it('should allow owner to remove an authorized address', async () => {
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.addAuthorizedAddress.sendTransactionAsync(address, { from: owner }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            const index = new BigNumber(0);
+            await web3Wrapper.awaitTransactionSuccessAsync(
+                await authorizable.removeAuthorizedAddressAtIndex.sendTransactionAsync(address, index, {
+                    from: owner,
+                }),
+                constants.AWAIT_TRANSACTION_MINED_MS,
+            );
+            const isAuthorized = await authorizable.authorized.callAsync(address);
+            expect(isAuthorized).to.be.false();
+        });
+    });
+
     describe('getAuthorizedAddresses', () => {
         it('should return all authorized addresses', async () => {
             const initial = await authorizable.getAuthorizedAddresses.callAsync();