From 3facd3f0216fc4ce0f3087fdc9f0ba726c0c75ab Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Wed, 31 Jul 2019 12:25:50 +0200
Subject: [PATCH 01/25] Merge Pool into Agent

---
 apps/agent/contracts/Agent.sol                | 134 ++++++++
 .../contracts/test/mocks/ExecutionTarget.sol  |   6 +
 apps/agent/test/agent_shared.js               | 296 +++++++++++++++++-
 3 files changed, 425 insertions(+), 11 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 6290824058..ac8440b89b 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -14,22 +14,100 @@ import "@aragon/os/contracts/common/IForwarder.sol";
 
 
 contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
+    /* Hardcoded constants to save gas
+    bytes32 public constant SAFE_EXECUTE_ROLE = keccak256("SAFE_EXECUTE_ROLE");
     bytes32 public constant EXECUTE_ROLE = keccak256("EXECUTE_ROLE");
     bytes32 public constant RUN_SCRIPT_ROLE = keccak256("RUN_SCRIPT_ROLE");
+    bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = keccak256("ADD_PROTECTED_TOKEN_ROLE");
+    bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = keccak256("REMOVE_PROTECTED_TOKEN_ROLE");
     bytes32 public constant ADD_PRESIGNED_HASH_ROLE = keccak256("ADD_PRESIGNED_HASH_ROLE");
     bytes32 public constant DESIGNATE_SIGNER_ROLE = keccak256("DESIGNATE_SIGNER_ROLE");
+    */
+
+    bytes32 public constant SAFE_EXECUTE_ROLE = 0x0a1ad7b87f5846153c6d5a1f761d71c7d0cfd122384f56066cd33239b7933694;
+    bytes32 public constant EXECUTE_ROLE = 0xcebf517aa4440d1d125e0355aae64401211d0848a23c02cc5d29a14822580ba4;
+    bytes32 public constant RUN_SCRIPT_ROLE = 0xb421f7ad7646747f3051c50c0b8e2377839296cd4973e27f63821d73e390338f;
+    bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = 0x6eb2a499556bfa2872f5aa15812b956cc4a71b4d64eb3553f7073c7e41415aaa;
+    bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = 0x71eee93d500f6f065e38b27d242a756466a00a52a1dbcd6b4260f01a8640402a;
+    bytes32 public constant ADD_PRESIGNED_HASH_ROLE = 0x0b29780bb523a130b3b01f231ef49ed2fa2781645591a0b0a44ca98f15a5994c;
+    bytes32 public constant DESIGNATE_SIGNER_ROLE = 0x23ce341656c3f14df6692eebd4757791e33662b7dcf9970c8308303da5472b7c;
+
+    uint256 public constant PROTECTED_TOKENS_CAP = 10;
 
     bytes4 private constant ERC165_INTERFACE_ID = 0x01ffc9a7;
 
+    string private constant ERROR_TOKENS_CAP_REACHED = "AGENT_TOKENS_CAP_REACHED";
+    string private constant ERROR_TOKEN_NOT_ETH_OR_CONTRACT = "AGENT_TOKEN_NOT_ETH_OR_CONTRACT";
+    string private constant ERROR_TOKEN_ALREADY_PROTECTED = "AGENT_TOKEN_ALREADY_PROTECTED";
+    string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
+    string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
+    string private constant ERROR_PROTECTED_TOKENS_MODIFIED = "AGENT_PROTECTED_TOKENS_MODIFIED";
+    string private constant ERROR_BALANCE_NOT_CONSTANT = "AGENT_BALANCE_NOT_CONSTANT";
     string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";
 
     mapping (bytes32 => bool) public isPresigned;
     address public designatedSigner;
+    address[] public protectedTokens;
 
+    event SafeExecute(address indexed sender, address indexed target, bytes data);
     event Execute(address indexed sender, address indexed target, uint256 ethValue, bytes data);
+    event AddProtectedToken(address indexed token);
+    event RemoveProtectedToken(address indexed token);
     event PresignHash(address indexed sender, bytes32 indexed hash);
     event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);
 
+    /**
+    * @notice Safe execute '`@radspec(_target, _data)`' on `_target`
+    * @param _target Address where the action is being executed
+    * @param _data Calldata for the action
+    * @return Exits call frame forwarding the return data of the executed call (either error or success data)
+    */
+    function safeExecute(address _target, bytes _data) external auth(SAFE_EXECUTE_ROLE) {
+        address[] memory _protectedTokens = new address[](protectedTokens.length);
+        uint256[] memory balances = new uint256[](protectedTokens.length);
+        bytes32 size;
+        bytes32 ptr;
+
+        for (uint256 i = 0; i < protectedTokens.length; i++) {
+            address token = protectedTokens[i];
+            // we don't care if target is ETH [0x00...0] as it can't be spent anyhow [though you can't invoke anything at 0x00...0]
+            require(_target != token || token == ETH, ERROR_TARGET_PROTECTED);
+            // we copy the protected tokens array to check whether the storage array has been modified during the underlying call
+            _protectedTokens[i] = token;
+            // we copy the balances to check whether they have been modified during the underlying call
+            balances[i] = balance(token);
+        }
+
+        bool result = _target.call(_data);
+
+        assembly {
+            size := returndatasize
+            ptr := mload(0x40)
+            mstore(0x40, add(ptr, size))
+            returndatacopy(ptr, 0, size)
+        }
+
+        if (result) {
+            // if the underlying call has succeeded, we check that the protected tokens
+            // and their balances have not been modified and return the call's return data
+            for (uint256 j = 0; j < _protectedTokens.length; j++) {
+                require(protectedTokens[j] == _protectedTokens[j], ERROR_PROTECTED_TOKENS_MODIFIED);
+                require(balances[j] == balance(_protectedTokens[j]), ERROR_BALANCE_NOT_CONSTANT);
+            }
+
+            emit SafeExecute(msg.sender, _target, _data);
+
+            assembly {
+                return(ptr, size)
+            }
+        } else {
+            // if the underlying call has failed, we revert and forward [possible] returned error data
+            assembly {
+                revert(ptr, size)
+            }
+        }
+    }
+
     /**
     * @notice Execute '`@radspec(_target, _data)`' on `_target``_ethValue == 0 ? '' : ' (Sending' + @tokenAmount(0x0000000000000000000000000000000000000000, _ethValue) + ')'`
     * @param _target Address where the action is being executed
@@ -59,6 +137,28 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         }
     }
 
+    /**
+    * @notice Add `_token.symbol(): string` to the list of protected tokens
+    * @param _token Address of the token to be protected
+    */
+    function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
+        require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
+        require(isContract(_token) || _token == ETH, ERROR_TOKEN_NOT_ETH_OR_CONTRACT);
+        require(!tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
+
+        _addProtectedToken(_token);
+    }
+
+    /**
+    * @notice Remove `_token.symbol(): string` from the list of protected tokens
+    * @param _token Address of the token to be unprotected
+    */
+    function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
+        require(tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
+
+      _removeProtectedToken(_token);
+    }
+
     /**
     * @notice Set `_designatedSigner` as the designated signer of the app, which will be able to sign messages on behalf of the app
     * @param _designatedSigner Address that will be able to sign messages on behalf of the app
@@ -141,6 +241,20 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         return canPerform(sender, RUN_SCRIPT_ROLE, params);
     }
 
+    function _addProtectedToken(address _token) internal {
+        protectedTokens.push(_token);
+
+        emit AddProtectedToken(_token);
+    }
+
+    function _removeProtectedToken(address _token) internal {
+        protectedTokens[protectedTokenIndex(_token)] = protectedTokens[protectedTokens.length - 1];
+        delete protectedTokens[protectedTokens.length - 1];
+        protectedTokens.length --;
+
+        emit RemoveProtectedToken(_token);
+    }
+
     function getScriptACLParam(bytes evmScript) internal pure returns (uint256) {
         return uint256(keccak256(abi.encodePacked(evmScript)));
     }
@@ -152,4 +266,24 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
 
         assembly { sig := mload(add(data, 0x20)) }
     }
+
+    function tokenIsProtected(address _token) internal view returns (bool) {
+        for (uint256 i = 0; i < protectedTokens.length; i++) {
+            if (protectedTokens[i] == _token) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    function protectedTokenIndex(address _token) internal view returns (uint256) {
+        for (uint i = 0; i < protectedTokens.length; i++) {
+            if (protectedTokens[i] == _token) {
+              return i;
+            }
+        }
+
+        revert(ERROR_TOKEN_NOT_PROTECTED);
+    }
 }
diff --git a/apps/agent/contracts/test/mocks/ExecutionTarget.sol b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
index fd04b8611c..723adda0eb 100644
--- a/apps/agent/contracts/test/mocks/ExecutionTarget.sol
+++ b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
@@ -1,5 +1,7 @@
 pragma solidity 0.4.24;
 
+import "@aragon/test-helpers/contracts/TokenMock.sol";
+
 
 contract ExecutionTarget {
     uint public counter;
@@ -15,6 +17,10 @@ contract ExecutionTarget {
         counter = x;
     }
 
+    function transferTokenFrom(address _token) external {
+        TokenMock(_token).transferFrom(msg.sender, this, 1);
+    }
+
     function fail() external pure {
         revert(REASON);
     }
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index dcad6d105f..100006daf6 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -29,6 +29,7 @@ module.exports = (
   const DesignatedSigner = artifacts.require('DesignatedSigner')
   const DestinationMock = artifacts.require('DestinationMock')
   const EtherTokenConstantMock = artifacts.require('EtherTokenConstantMock')
+  const TokenMock = artifacts.require('TokenMock')
 
   const NO_SIG = '0x'
   const ERC165_SUPPORT_INVALID_ID = '0xffffffff'
@@ -39,7 +40,7 @@ module.exports = (
   context(`> Shared tests for Agent-like apps`, () => {
     let daoFact, agentBase, dao, acl, agent, agentAppId
 
-    let ETH, ANY_ENTITY, APP_MANAGER_ROLE, EXECUTE_ROLE, RUN_SCRIPT_ROLE, ADD_PRESIGNED_HASH_ROLE, DESIGNATE_SIGNER_ROLE, ERC1271_INTERFACE_ID
+    let ETH, ANY_ENTITY, APP_MANAGER_ROLE, EXECUTE_ROLE, SAFE_EXECUTE_ROLE, RUN_SCRIPT_ROLE, ADD_PROTECTED_TOKEN_ROLE, REMOVE_PROTECTED_TOKEN_ROLE, ADD_PRESIGNED_HASH_ROLE, DESIGNATE_SIGNER_ROLE, ERC1271_INTERFACE_ID
 
     // Error strings
     const errors = makeErrorMappingProxy({
@@ -54,6 +55,8 @@ module.exports = (
     })
 
     const root = accounts[0]
+    const authorized = accounts[1]
+    const unauthorized = accounts[2]
 
     const encodeFunctionCall = (contract, functionName, ...params) =>
       contract[functionName].request(...params).params[0]
@@ -68,8 +71,11 @@ module.exports = (
       // Setup constants
       ANY_ENTITY = await aclBase.ANY_ENTITY()
       APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
+      SAFE_EXECUTE_ROLE = await agentBase.SAFE_EXECUTE_ROLE()
       EXECUTE_ROLE = await agentBase.EXECUTE_ROLE()
       RUN_SCRIPT_ROLE = await agentBase.RUN_SCRIPT_ROLE()
+      ADD_PROTECTED_TOKEN_ROLE = await agentBase.ADD_PROTECTED_TOKEN_ROLE()
+      REMOVE_PROTECTED_TOKEN_ROLE = await agentBase.REMOVE_PROTECTED_TOKEN_ROLE()
       ADD_PRESIGNED_HASH_ROLE = await agentBase.ADD_PRESIGNED_HASH_ROLE()
       DESIGNATE_SIGNER_ROLE = await agentBase.DESIGNATE_SIGNER_ROLE()
       ERC1271_INTERFACE_ID = await agentBase.ERC1271_INTERFACE_ID()
@@ -94,6 +100,143 @@ module.exports = (
       await agent.initialize()
     })
 
+    context('> Safe executing actions', () => {
+      const noData = '0x'
+      const amount = 1000
+      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9
+
+      beforeEach(async () => {
+        target = await ExecutionTarget.new()
+        token1 = await TokenMock.new(agent.address, amount)
+        token2 = await TokenMock.new(agent.address, amount)
+        token3 = await TokenMock.new(agent.address, amount)
+        token4 = await TokenMock.new(agent.address, amount)
+        token5 = await TokenMock.new(agent.address, amount)
+        token6 = await TokenMock.new(agent.address, amount)
+        token7 = await TokenMock.new(agent.address, amount)
+        token8 = await TokenMock.new(agent.address, amount)
+        token9 = await TokenMock.new(agent.address, amount)
+
+        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
+
+        await agent.addProtectedToken(ETH, { from: authorized })
+        await agent.addProtectedToken(token1.address, { from: authorized })
+        await agent.addProtectedToken(token2.address, { from: authorized })
+        await agent.addProtectedToken(token3.address, { from: authorized })
+        await agent.addProtectedToken(token4.address, { from: authorized })
+        await agent.addProtectedToken(token5.address, { from: authorized })
+        await agent.addProtectedToken(token6.address, { from: authorized })
+        await agent.addProtectedToken(token7.address, { from: authorized })
+        await agent.addProtectedToken(token8.address, { from: authorized })
+        await agent.addProtectedToken(token9.address, { from: authorized })
+
+        assert.equal(await target.counter(), 0)
+        assert.equal(await token1.balanceOf(agent.address), amount)
+        assert.equal(await token2.balanceOf(agent.address), amount)
+      })
+
+      context('> sender has SAFE_EXECUTE_ROLE', () => {
+        context('> and target is not a protected ERC20', () => {
+          it('it can execute actions', async () => {
+            const N = 1102
+            const data = target.contract.setCounter.getData(N)
+            const receipt = await agent.safeExecute(target.address, data, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await target.counter(), N)
+          })
+
+          it('it can execute actions without data', async () => {
+            const receipt = await agent.safeExecute(target.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await target.counter(), 1) // fallback just runs ExecutionTarget.execute()
+          })
+
+          it('it can execute cheap fallback actions', async () => {
+            const cheapFallbackTarget = await DestinationMock.new(false)
+            const receipt = await agent.safeExecute(cheapFallbackTarget.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can execute expensive fallback actions', async () => {
+            const expensiveFallbackTarget = await DestinationMock.new(true)
+            assert.equal(await expensiveFallbackTarget.counter(), 0)
+            const receipt = await agent.safeExecute(expensiveFallbackTarget.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await expensiveFallbackTarget.counter(), 1) // fallback increments counter
+          })
+
+          it('it can execute with data when target is not a contract', async () => {
+            const nonContract = accounts[8] // random account
+            const randomData = '0x12345678'
+            const receipt = await agent.safeExecute(nonContract, randomData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can execute without data when target is not a contract', async () => {
+            const nonContract = accounts[8] // random account
+            const receipt = await agent.safeExecute(nonContract, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can forward success return data', async () => {
+            const { to, data } = encodeFunctionCall(target, 'execute')
+
+            // We make a call to easily get what data could be gotten inside the EVM
+            // Contract -> agent.safeExecute -> Target.func (would allow Contract to have access to this data)
+            const call = encodeFunctionCall(agent, 'safeExecute', to, data, { from: authorized })
+            const returnData = await web3Call(call)
+
+            // ExecutionTarget.execute() increments the counter by 1
+            assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
+          })
+
+          it('it should revert if executed action reverts', async () => {
+            // TODO: Check revert data was correctly forwarded
+            // ganache currently doesn't support fetching this data
+            const data = target.contract.fail.getData()
+            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+          })
+        })
+
+        context('> but target is a protected ERC20', () => {
+          it('it should revert', async () => {
+            const approve = token1.contract.approve.getData(target.address, 10)
+
+            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
+          })
+        })
+
+        context('> and target is not a protected ERC20 but action affects a protected ERC20 balance', () => {
+          it('it should revert', async () => {
+            await agent.removeProtectedToken(token9.address, { from: authorized }) // leave a spot to add a new token
+            const token10 = await TokenMock.new(agent.address, amount)
+            const approve = token10.contract.approve.getData(target.address, 10)
+            await agent.safeExecute(token10.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+            await agent.addProtectedToken(token10.address, { from: authorized }) // token10 is now protected
+            const data = target.contract.transferTokenFrom.getData(token10.address)
+
+            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+          })
+        })
+      })
+
+      context('> sender does not have SAFE_EXECUTE_ROLE', () => {
+        it('it should revert', async () => {
+          const data = target.contract.execute.getData()
+
+          await assertRevert(() => agent.safeExecute(target.address, data, { from: unauthorized }))
+        })
+      })
+    })
+
     context('> Executing actions', () => {
       const [_, nonExecutor, executor] = accounts
       let executionTarget
@@ -217,7 +360,7 @@ module.exports = (
           })
 
           context('depending on the sig ACL param', () => {
-            const [ granteeEqualToSig, granteeUnequalToSig ] = accounts.slice(6) // random slice from accounts
+            const [granteeEqualToSig, granteeUnequalToSig] = accounts.slice(6) // random slice from accounts
 
             beforeEach(async () => {
               const sig = executionTarget.contract.setCounter.getData(1).slice(2, 10)
@@ -305,16 +448,147 @@ module.exports = (
       })
     })
 
+    context('> Adding protected tokens', () => {
+      beforeEach(async () => {
+        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+      })
+
+      context('> sender has ADD_PROTECTED_TOKEN_ROLE', () => {
+        context('> and token is ETH or ERC20', () => {
+          context('> and token is not already protected', () => {
+            context('> and protected tokens cap has not yet been reached', () => {
+              it('it should add protected token', async () => {
+                const token2 = await TokenMock.new(agent.address, 10000)
+                const token3 = await TokenMock.new(agent.address, 10000)
+
+                const receipt1 = await agent.addProtectedToken(ETH, { from: authorized })
+                const receipt2 = await agent.addProtectedToken(token2.address, { from: authorized })
+                const receipt3 = await agent.addProtectedToken(token3.address, { from: authorized })
+
+                assertAmountOfEvents(receipt1, 'AddProtectedToken')
+                assertAmountOfEvents(receipt2, 'AddProtectedToken')
+                assertAmountOfEvents(receipt3, 'AddProtectedToken')
+                assert.equal(await agent.protectedTokens(0), ETH)
+                assert.equal(await agent.protectedTokens(1), token2.address)
+                assert.equal(await agent.protectedTokens(2), token3.address)
+              })
+            })
+
+            context('> but protected tokens cap has been reached', () => {
+              beforeEach(async () => {
+                const token1 = await TokenMock.new(agent.address, 1000)
+                const token2 = await TokenMock.new(agent.address, 1000)
+                const token3 = await TokenMock.new(agent.address, 1000)
+                const token4 = await TokenMock.new(agent.address, 1000)
+                const token5 = await TokenMock.new(agent.address, 1000)
+                const token6 = await TokenMock.new(agent.address, 1000)
+                const token7 = await TokenMock.new(agent.address, 1000)
+                const token8 = await TokenMock.new(agent.address, 1000)
+                const token9 = await TokenMock.new(agent.address, 1000)
+
+                await agent.addProtectedToken(ETH, { from: authorized })
+                await agent.addProtectedToken(token1.address, { from: authorized })
+                await agent.addProtectedToken(token2.address, { from: authorized })
+                await agent.addProtectedToken(token3.address, { from: authorized })
+                await agent.addProtectedToken(token4.address, { from: authorized })
+                await agent.addProtectedToken(token5.address, { from: authorized })
+                await agent.addProtectedToken(token6.address, { from: authorized })
+                await agent.addProtectedToken(token7.address, { from: authorized })
+                await agent.addProtectedToken(token8.address, { from: authorized })
+                await agent.addProtectedToken(token9.address, { from: authorized })
+              })
+
+              it('it should revert', async () => {
+                const token10 = await TokenMock.new(agent.address, 10000)
+
+                await assertRevert(() => agent.addProtectedToken(token10.address, { from: authorized }))
+              })
+            })
+          })
+
+          context('> but token is already protected', () => {
+            it('it should revert', async () => {
+              const token = await TokenMock.new(agent.address, 10000)
+              await agent.addProtectedToken(token.address, { from: authorized })
+
+              await assertRevert(() => agent.addProtectedToken(token.address, { from: authorized }))
+            })
+          })
+        })
+
+        context('> but token is not ETH or ERC20', () => {
+          it('it should revert', async () => {
+            await assertRevert(() => agent.addProtectedToken(root, { from: authorized }))
+          })
+        })
+      })
+
+      context('> sender does not have ADD_PROTECTED_TOKEN_ROLE', () => {
+        it('it should revert', async () => {
+          const token = await TokenMock.new(agent.address, 10000)
+
+          await assertRevert(() => agent.addProtectedToken(token.address, { from: unauthorized }))
+        })
+      })
+    })
+
+    context('> Removing protected tokens', () => {
+      beforeEach(async () => {
+        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
+      })
+
+      context('> sender has REMOVE_PROTECTED_TOKEN_ROLE', () => {
+        context('> and token is actually protected', () => {
+          it('it should remove protected token', async () => {
+            const token2 = await TokenMock.new(agent.address, 10000)
+            const token3 = await TokenMock.new(agent.address, 10000)
+
+            await agent.addProtectedToken(ETH, { from: authorized })
+            await agent.addProtectedToken(token2.address, { from: authorized })
+            await agent.addProtectedToken(token3.address, { from: authorized })
+
+            const receipt1 = await agent.removeProtectedToken(token3.address, { from: authorized })
+            const receipt2 = await agent.removeProtectedToken(ETH, { from: authorized })
+
+            assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
+            assertAmountOfEvents(receipt2, 'RemoveProtectedToken')
+            assert.equal(await agent.protectedTokens(0), token2.address)
+            await assertRevert(() => agent.protectedTokens(1)) // this should try to overflow the length of the protectedTokens array and thus revert
+          })
+        })
+
+        context('> but token is not actually protected', () => {
+          it('it should revert', async () => {
+            const token1 = await TokenMock.new(agent.address, 10000)
+            const token2 = await TokenMock.new(agent.address, 10000)
+            await agent.addProtectedToken(token1.address, { from: authorized })
+
+            await assertRevert(() => agent.removeProtectedToken(token2.address, { from: authorized }))
+          })
+        })
+      })
+
+      context('> sender does not have REMOVE_PROTECTED_TOKEN_ROLE', () => {
+        it('it should revert', async () => {
+          const token = await TokenMock.new(agent.address, 10000)
+          await agent.addProtectedToken(token.address, { from: authorized })
+
+          await assertRevert(() => agent.removeProtectedToken(token.address, { from: unauthorized }))
+        })
+      })
+    })
+
     context('> Signing messages', () => {
       const [_, nobody, presigner, signerDesignator] = accounts
       const HASH = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing
 
       const SIGNATURE_MODES = {
         Invalid: '0x00',
-        EIP712:  '0x01',
+        EIP712: '0x01',
         EthSign: '0x02',
         ERC1271: '0x03',
-        NMode:   '0x04',
+        NMode: '0x04',
       }
 
       const ERC1271_RETURN_VALID_SIGNATURE = '0x20c13b0b'
@@ -376,11 +650,11 @@ module.exports = (
         }
 
         const eip712Sign = async (hash, key) => ({
-            mode: ethUtil.toBuffer(SIGNATURE_MODES.EIP712),
-            ...ethUtil.ecsign(
-              Buffer.from(hash.slice(2), 'hex'),
-              Buffer.from(key, 'hex')
-            )
+          mode: ethUtil.toBuffer(SIGNATURE_MODES.EIP712),
+          ...ethUtil.ecsign(
+            Buffer.from(hash.slice(2), 'hex'),
+            Buffer.from(key, 'hex')
+          )
         })
 
         const signFunctionGenerator = (signFunction, signatureModifier) => (
@@ -388,8 +662,8 @@ module.exports = (
             const sig = await signFunction(hash, signerOrKey)
             const v =
               useInvalidV
-              ? ethUtil.toBuffer(2) // force set an invalid v
-              : ethUtil.toBuffer(sig.v - (useLegacySig ? 0 : 27))
+                ? ethUtil.toBuffer(2) // force set an invalid v
+                : ethUtil.toBuffer(sig.v - (useLegacySig ? 0 : 27))
 
             const signature = '0x' + Buffer.concat([sig.mode, sig.r, sig.s, v]).toString('hex')
             return signatureModifier(signature)

From 7b3d92f08c6ba9cab0b96e8cb09f14c5ab3cbb61 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 16:56:04 +0200
Subject: [PATCH 02/25] Update roles

---
 apps/agent/arapp.json | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/apps/agent/arapp.json b/apps/agent/arapp.json
index 7c00d402c7..67dfe2ec23 100644
--- a/apps/agent/arapp.json
+++ b/apps/agent/arapp.json
@@ -33,6 +33,21 @@
     }
   },
   "roles": [
+    {
+      "name": "Execute safe actions",
+      "id": "SAFE_EXECUTE_ROLE",
+      "params": []
+    },
+    {
+      "name": "Add protected tokens",
+      "id": "ADD_PROTECTED_TOKEN_ROLE",
+      "params": []
+    },
+    {
+      "name": "Remove protected tokens",
+      "id": "REMOVE_PROTECTED_TOKEN_ROLE",
+      "params": []
+    },
     {
       "name": "Execute actions",
       "id": "EXECUTE_ROLE",
@@ -62,6 +77,15 @@
       "params": [
         "EVM Script"
       ]
+    },
+    {
+      "name": "Transfer Agent's tokens",
+      "id": "TRANSFER_ROLE",
+      "params": [
+        "Token address",
+        "Receiver address",
+        "Token amount"
+      ]
     }
   ],
   "path": "contracts/Agent.sol"

From 17497315a3fc987f76685c235ea760a8212e7980 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:17:59 +0200
Subject: [PATCH 03/25] Fix indentation issue

---
 apps/agent/contracts/Agent.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index ac8440b89b..be898f9627 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -156,7 +156,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
         require(tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
 
-      _removeProtectedToken(_token);
+        _removeProtectedToken(_token);
     }
 
     /**

From c7fa7e90c4737a46b0490bf94324887e3ea0c7d1 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:29:28 +0200
Subject: [PATCH 04/25] Allow protected balance increase

---
 apps/agent/contracts/Agent.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index be898f9627..8cb7237ad1 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -42,7 +42,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
     string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
     string private constant ERROR_PROTECTED_TOKENS_MODIFIED = "AGENT_PROTECTED_TOKENS_MODIFIED";
-    string private constant ERROR_BALANCE_NOT_CONSTANT = "AGENT_BALANCE_NOT_CONSTANT";
+    string private constant ERROR_PROTECTED_BALANCE_LOWERED = "AGENT_PROTECTED_BALANCE_LOWERED";
     string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";
 
     mapping (bytes32 => bool) public isPresigned;
@@ -92,7 +92,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
             // and their balances have not been modified and return the call's return data
             for (uint256 j = 0; j < _protectedTokens.length; j++) {
                 require(protectedTokens[j] == _protectedTokens[j], ERROR_PROTECTED_TOKENS_MODIFIED);
-                require(balances[j] == balance(_protectedTokens[j]), ERROR_BALANCE_NOT_CONSTANT);
+                require(balance(protectedTokens[j]) >= balances[j], ERROR_PROTECTED_BALANCE_LOWERED);
             }
 
             emit SafeExecute(msg.sender, _target, _data);

From 87db2c550f3cbade0509b70d2db76fd66ec417bd Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:36:49 +0200
Subject: [PATCH 05/25] Cache protectedTokens.length and check it has not been
 altered during script execution

---
 apps/agent/contracts/Agent.sol | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 8cb7237ad1..ef58177add 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -63,12 +63,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @return Exits call frame forwarding the return data of the executed call (either error or success data)
     */
     function safeExecute(address _target, bytes _data) external auth(SAFE_EXECUTE_ROLE) {
-        address[] memory _protectedTokens = new address[](protectedTokens.length);
-        uint256[] memory balances = new uint256[](protectedTokens.length);
+        uint256 protectedTokensLength = protectedTokens.length;
+        address[] memory _protectedTokens = new address[](protectedTokensLength);
+        uint256[] memory balances = new uint256[](protectedTokensLength);
         bytes32 size;
         bytes32 ptr;
 
-        for (uint256 i = 0; i < protectedTokens.length; i++) {
+        for (uint256 i = 0; i < protectedTokensLength; i++) {
             address token = protectedTokens[i];
             // we don't care if target is ETH [0x00...0] as it can't be spent anyhow [though you can't invoke anything at 0x00...0]
             require(_target != token || token == ETH, ERROR_TARGET_PROTECTED);
@@ -90,7 +91,8 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         if (result) {
             // if the underlying call has succeeded, we check that the protected tokens
             // and their balances have not been modified and return the call's return data
-            for (uint256 j = 0; j < _protectedTokens.length; j++) {
+            require(protectedTokens.length == protectedTokensLength, ERROR_PROTECTED_TOKENS_MODIFIED);
+            for (uint256 j = 0; j < protectedTokensLength; j++) {
                 require(protectedTokens[j] == _protectedTokens[j], ERROR_PROTECTED_TOKENS_MODIFIED);
                 require(balance(protectedTokens[j]) >= balances[j], ERROR_PROTECTED_BALANCE_LOWERED);
             }

From 365298ebbed3fd276d6d53d479fa9f339ae56427 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:39:56 +0200
Subject: [PATCH 06/25] Update radspec description

---
 apps/agent/contracts/Agent.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index ef58177add..5be0cc6bc8 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -57,7 +57,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);
 
     /**
-    * @notice Safe execute '`@radspec(_target, _data)`' on `_target`
+    * @notice Execute '`@radspec(_target, _data)`' on `_target` ensuring that protected tokens can't be spent
     * @param _target Address where the action is being executed
     * @param _data Calldata for the action
     * @return Exits call frame forwarding the return data of the executed call (either error or success data)

From 26aa1c20fe7cf2a2f9c03b79fc0d4b58f5e3b03d Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:44:10 +0200
Subject: [PATCH 07/25] Update SAFE_EXECUTE_ROLE to be permissioned

---
 apps/agent/arapp.json          | 5 ++++-
 apps/agent/contracts/Agent.sol | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/apps/agent/arapp.json b/apps/agent/arapp.json
index 67dfe2ec23..d2c2c0fefc 100644
--- a/apps/agent/arapp.json
+++ b/apps/agent/arapp.json
@@ -36,7 +36,10 @@
     {
       "name": "Execute safe actions",
       "id": "SAFE_EXECUTE_ROLE",
-      "params": []
+      "params": [
+        "Target address",
+        "Signature"
+      ]
     },
     {
       "name": "Add protected tokens",
diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 5be0cc6bc8..b864d3ab6e 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -62,7 +62,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @param _data Calldata for the action
     * @return Exits call frame forwarding the return data of the executed call (either error or success data)
     */
-    function safeExecute(address _target, bytes _data) external auth(SAFE_EXECUTE_ROLE) {
+    function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(getSig(_data)))) {
         uint256 protectedTokensLength = protectedTokens.length;
         address[] memory _protectedTokens = new address[](protectedTokensLength);
         uint256[] memory balances = new uint256[](protectedTokensLength);

From 1c262ddd8c553830a93da13f48e7650125ac3c92 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 17:47:57 +0200
Subject: [PATCH 08/25] Remove use of ETH as a fake ERC20

---
 apps/agent/contracts/Agent.sol | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index b864d3ab6e..658e914f60 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -37,7 +37,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     bytes4 private constant ERC165_INTERFACE_ID = 0x01ffc9a7;
 
     string private constant ERROR_TOKENS_CAP_REACHED = "AGENT_TOKENS_CAP_REACHED";
-    string private constant ERROR_TOKEN_NOT_ETH_OR_CONTRACT = "AGENT_TOKEN_NOT_ETH_OR_CONTRACT";
+    string private constant ERROR_TOKEN_NOT_ERC20 = "AGENT_TOKEN_NOT_ERC20";
     string private constant ERROR_TOKEN_ALREADY_PROTECTED = "AGENT_TOKEN_ALREADY_PROTECTED";
     string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
     string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
@@ -71,8 +71,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
 
         for (uint256 i = 0; i < protectedTokensLength; i++) {
             address token = protectedTokens[i];
-            // we don't care if target is ETH [0x00...0] as it can't be spent anyhow [though you can't invoke anything at 0x00...0]
-            require(_target != token || token == ETH, ERROR_TARGET_PROTECTED);
+            require(_target != token, ERROR_TARGET_PROTECTED);
             // we copy the protected tokens array to check whether the storage array has been modified during the underlying call
             _protectedTokens[i] = token;
             // we copy the balances to check whether they have been modified during the underlying call
@@ -145,7 +144,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
         require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
-        require(isContract(_token) || _token == ETH, ERROR_TOKEN_NOT_ETH_OR_CONTRACT);
+        require(isContract(_token), ERROR_TOKEN_NOT_ERC20);
         require(!tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
 
         _addProtectedToken(_token);

From 05784dfc65cab93edd735a565465d8cd8e4e3317 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 18:06:44 +0200
Subject: [PATCH 09/25] Move ERC20 check into its own function

---
 apps/agent/contracts/Agent.sol | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 658e914f60..df71f2ce12 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -144,7 +144,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
         require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
-        require(isContract(_token), ERROR_TOKEN_NOT_ERC20);
+        require(isERC20(_token), ERROR_TOKEN_NOT_ERC20);
         require(!tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
 
         _addProtectedToken(_token);
@@ -278,6 +278,10 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         return false;
     }
 
+    function isERC20(address _token) internal view returns (bool) {
+        return isContract(_token);
+    }
+
     function protectedTokenIndex(address _token) internal view returns (uint256) {
         for (uint i = 0; i < protectedTokens.length; i++) {
             if (protectedTokens[i] == _token) {

From 0aa07c64b1349c1b51c3b98bd2a5ca5315a5fe82 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Fri, 2 Aug 2019 18:09:29 +0200
Subject: [PATCH 10/25] Update tests

---
 apps/agent/test/agent_shared.js | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 100006daf6..8e965529cd 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -103,7 +103,7 @@ module.exports = (
     context('> Safe executing actions', () => {
       const noData = '0x'
       const amount = 1000
-      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9
+      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9, token10
 
       beforeEach(async () => {
         target = await ExecutionTarget.new()
@@ -116,12 +116,12 @@ module.exports = (
         token7 = await TokenMock.new(agent.address, amount)
         token8 = await TokenMock.new(agent.address, amount)
         token9 = await TokenMock.new(agent.address, amount)
+        token10 = await TokenMock.new(agent.address, amount)
 
         await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
         await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
         await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
 
-        await agent.addProtectedToken(ETH, { from: authorized })
         await agent.addProtectedToken(token1.address, { from: authorized })
         await agent.addProtectedToken(token2.address, { from: authorized })
         await agent.addProtectedToken(token3.address, { from: authorized })
@@ -131,6 +131,7 @@ module.exports = (
         await agent.addProtectedToken(token7.address, { from: authorized })
         await agent.addProtectedToken(token8.address, { from: authorized })
         await agent.addProtectedToken(token9.address, { from: authorized })
+        await agent.addProtectedToken(token10.address, { from: authorized })
 
         assert.equal(await target.counter(), 0)
         assert.equal(await token1.balanceOf(agent.address), amount)
@@ -454,23 +455,20 @@ module.exports = (
       })
 
       context('> sender has ADD_PROTECTED_TOKEN_ROLE', () => {
-        context('> and token is ETH or ERC20', () => {
+        context('> and token is ERC20', () => {
           context('> and token is not already protected', () => {
             context('> and protected tokens cap has not yet been reached', () => {
               it('it should add protected token', async () => {
+                const token1 = await TokenMock.new(agent.address, 10000)
                 const token2 = await TokenMock.new(agent.address, 10000)
-                const token3 = await TokenMock.new(agent.address, 10000)
 
-                const receipt1 = await agent.addProtectedToken(ETH, { from: authorized })
+                const receipt1 = await agent.addProtectedToken(token1.address, { from: authorized })
                 const receipt2 = await agent.addProtectedToken(token2.address, { from: authorized })
-                const receipt3 = await agent.addProtectedToken(token3.address, { from: authorized })
 
                 assertAmountOfEvents(receipt1, 'AddProtectedToken')
                 assertAmountOfEvents(receipt2, 'AddProtectedToken')
-                assertAmountOfEvents(receipt3, 'AddProtectedToken')
-                assert.equal(await agent.protectedTokens(0), ETH)
+                assert.equal(await agent.protectedTokens(0), token1.address)
                 assert.equal(await agent.protectedTokens(1), token2.address)
-                assert.equal(await agent.protectedTokens(2), token3.address)
               })
             })
 
@@ -485,8 +483,8 @@ module.exports = (
                 const token7 = await TokenMock.new(agent.address, 1000)
                 const token8 = await TokenMock.new(agent.address, 1000)
                 const token9 = await TokenMock.new(agent.address, 1000)
+                const token10 = await TokenMock.new(agent.address, 1000)
 
-                await agent.addProtectedToken(ETH, { from: authorized })
                 await agent.addProtectedToken(token1.address, { from: authorized })
                 await agent.addProtectedToken(token2.address, { from: authorized })
                 await agent.addProtectedToken(token3.address, { from: authorized })
@@ -496,6 +494,7 @@ module.exports = (
                 await agent.addProtectedToken(token7.address, { from: authorized })
                 await agent.addProtectedToken(token8.address, { from: authorized })
                 await agent.addProtectedToken(token9.address, { from: authorized })
+                await agent.addProtectedToken(token10.address, { from: authorized })
               })
 
               it('it should revert', async () => {
@@ -516,7 +515,7 @@ module.exports = (
           })
         })
 
-        context('> but token is not ETH or ERC20', () => {
+        context('> but token is not ERC20', () => {
           it('it should revert', async () => {
             await assertRevert(() => agent.addProtectedToken(root, { from: authorized }))
           })
@@ -541,15 +540,16 @@ module.exports = (
       context('> sender has REMOVE_PROTECTED_TOKEN_ROLE', () => {
         context('> and token is actually protected', () => {
           it('it should remove protected token', async () => {
+            const token1 = await TokenMock.new(agent.address, 10000)
             const token2 = await TokenMock.new(agent.address, 10000)
             const token3 = await TokenMock.new(agent.address, 10000)
 
-            await agent.addProtectedToken(ETH, { from: authorized })
+            await agent.addProtectedToken(token1.address, { from: authorized })
             await agent.addProtectedToken(token2.address, { from: authorized })
             await agent.addProtectedToken(token3.address, { from: authorized })
 
             const receipt1 = await agent.removeProtectedToken(token3.address, { from: authorized })
-            const receipt2 = await agent.removeProtectedToken(ETH, { from: authorized })
+            const receipt2 = await agent.removeProtectedToken(token1.address, { from: authorized })
 
             assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
             assertAmountOfEvents(receipt2, 'RemoveProtectedToken')

From 6512c27bd3cc3b2805ad8de066ea08069c30aeab Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Wed, 7 Aug 2019 12:20:42 +0200
Subject: [PATCH 11/25] Rename _protectedTokens into protectedTokens_

---
 apps/agent/contracts/Agent.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index df71f2ce12..000625bddb 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -64,7 +64,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(getSig(_data)))) {
         uint256 protectedTokensLength = protectedTokens.length;
-        address[] memory _protectedTokens = new address[](protectedTokensLength);
+        address[] memory protectedTokens_ = new address[](protectedTokensLength);
         uint256[] memory balances = new uint256[](protectedTokensLength);
         bytes32 size;
         bytes32 ptr;
@@ -73,7 +73,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
             address token = protectedTokens[i];
             require(_target != token, ERROR_TARGET_PROTECTED);
             // we copy the protected tokens array to check whether the storage array has been modified during the underlying call
-            _protectedTokens[i] = token;
+            protectedTokens_[i] = token;
             // we copy the balances to check whether they have been modified during the underlying call
             balances[i] = balance(token);
         }
@@ -92,7 +92,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
             // and their balances have not been modified and return the call's return data
             require(protectedTokens.length == protectedTokensLength, ERROR_PROTECTED_TOKENS_MODIFIED);
             for (uint256 j = 0; j < protectedTokensLength; j++) {
-                require(protectedTokens[j] == _protectedTokens[j], ERROR_PROTECTED_TOKENS_MODIFIED);
+                require(protectedTokens[j] == protectedTokens_[j], ERROR_PROTECTED_TOKENS_MODIFIED);
                 require(balance(protectedTokens[j]) >= balances[j], ERROR_PROTECTED_BALANCE_LOWERED);
             }
 

From 4e8f3ffe56b897709979f1c640ce82de5d6f3faa Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Wed, 7 Aug 2019 12:57:11 +0200
Subject: [PATCH 12/25] Add new test cases

---
 .../contracts/test/mocks/ExecutionTarget.sol  | 10 +++-
 apps/agent/test/agent_shared.js               | 49 ++++++++++++++-----
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/apps/agent/contracts/test/mocks/ExecutionTarget.sol b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
index 723adda0eb..0531b1ef37 100644
--- a/apps/agent/contracts/test/mocks/ExecutionTarget.sol
+++ b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
@@ -1,7 +1,7 @@
 pragma solidity 0.4.24;
 
 import "@aragon/test-helpers/contracts/TokenMock.sol";
-
+import "../../Agent.sol";
 
 contract ExecutionTarget {
     uint public counter;
@@ -21,6 +21,14 @@ contract ExecutionTarget {
         TokenMock(_token).transferFrom(msg.sender, this, 1);
     }
 
+    function transferTokenTo(address _token) external {
+        TokenMock(_token).transfer(msg.sender, 1);
+    }
+
+    function removeProtectedToken(address _token) external {
+        Agent(msg.sender).removeProtectedToken(_token);
+    }
+
     function fail() external pure {
         revert(REASON);
     }
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 8e965529cd..7690e3a6fb 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -205,26 +205,49 @@ module.exports = (
             const data = target.contract.fail.getData()
             await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
           })
-        })
 
-        context('> but target is a protected ERC20', () => {
-          it('it should revert', async () => {
-            const approve = token1.contract.approve.getData(target.address, 10)
+          context('> but action affects a protected ERC20 balance', () => {
+            context('> action decreases protected ERC20 balance', () => {
+              it('it should revert', async () => {
+                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
+                const token11 = await TokenMock.new(agent.address, amount)
+                const approve = token11.contract.approve.getData(target.address, 10)
+                await agent.safeExecute(token11.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
+                const data = target.contract.transferTokenFrom.getData(token11.address)
+
+                await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+              })
+            })
 
-            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
+            context('> action increases protected ERC20 balance', () => {
+              it('it should execute action', async () => {
+                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
+                const token11 = await TokenMock.new(target.address, amount)
+                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
+                const data = target.contract.transferTokenTo.getData(token11.address)
+                await agent.safeExecute(target.address, data, { from: authorized })
+
+                assert.equal((await token11.balanceOf(agent.address)).toNumber(), 1)
+              })
+            })
+          })
+
+          context('> but action affects protected tokens list', () => {
+            it('it should revert', async () => {
+              await acl.grantPermission(target.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
+              const data = target.contract.removeProtectedToken.getData(token4.address)
+
+              await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+            })
           })
         })
 
-        context('> and target is not a protected ERC20 but action affects a protected ERC20 balance', () => {
+        context('> but target is a protected ERC20', () => {
           it('it should revert', async () => {
-            await agent.removeProtectedToken(token9.address, { from: authorized }) // leave a spot to add a new token
-            const token10 = await TokenMock.new(agent.address, amount)
-            const approve = token10.contract.approve.getData(target.address, 10)
-            await agent.safeExecute(token10.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
-            await agent.addProtectedToken(token10.address, { from: authorized }) // token10 is now protected
-            const data = target.contract.transferTokenFrom.getData(token10.address)
+            const approve = token1.contract.approve.getData(target.address, 10)
 
-            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
           })
         })
       })

From c069ff1a36dc3d8fc354fad0c2730b1baa5c8347 Mon Sep 17 00:00:00 2001
From: Olivier Sarrouy <olivier.sarrouy@gmail.com>
Date: Wed, 7 Aug 2019 13:09:44 +0200
Subject: [PATCH 13/25] Add additionnal ERC20 checks

---
 apps/agent/contracts/Agent.sol  | 8 +++++++-
 apps/agent/test/agent_shared.js | 6 +++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 000625bddb..f93a91316d 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -279,7 +279,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     }
 
     function isERC20(address _token) internal view returns (bool) {
-        return isContract(_token);
+        if (!isContract(_token)) {
+            return false;
+        }
+
+        balance(_token);
+
+        return true;
     }
 
     function protectedTokenIndex(address _token) internal view returns (uint256) {
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 7690e3a6fb..9e10c4eef5 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -539,9 +539,13 @@ module.exports = (
         })
 
         context('> but token is not ERC20', () => {
-          it('it should revert', async () => {
+          it('it should revert [token is not a contract]', async () => {
             await assertRevert(() => agent.addProtectedToken(root, { from: authorized }))
           })
+
+          it('it should revert [token is a contract but not an ERC20]', async () => {
+            await assertRevert(() => agent.addProtectedToken(daoFact.address, { from: authorized }))
+          })
         })
       })
 

From 63a0f836096232b8979ec2e4119962bcc0a7d963 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Thu, 8 Aug 2019 22:55:37 +0200
Subject: [PATCH 14/25] Agent: reorder mainnet environment to be more
 prominent, remove old rinkeby environment

---
 apps/agent/arapp.json | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/apps/agent/arapp.json b/apps/agent/arapp.json
index d2c2c0fefc..f6c476089b 100644
--- a/apps/agent/arapp.json
+++ b/apps/agent/arapp.json
@@ -6,6 +6,11 @@
       "appName": "agent.aragonpm.eth",
       "network": "rpc"
     },
+    "mainnet": {
+      "registry": "0x314159265dd8dbb310642f98f50c066173c1259b",
+      "appName": "agent.aragonpm.eth",
+      "network": "mainnet"
+    },
     "rinkeby": {
       "registry": "0x98Df287B6C145399Aaa709692c8D308357bC085D",
       "appName": "agent.aragonpm.eth",
@@ -16,20 +21,10 @@
       "appName": "agent.aragonpm.eth",
       "network": "rinkeby"
     },
-    "mainnet": {
-      "registry": "0x314159265dd8dbb310642f98f50c066173c1259b",
-      "appName": "agent.aragonpm.eth",
-      "network": "mainnet"
-    },
     "ropsten": {
       "registry": "0x6afe2cacee211ea9179992f89dc61ff25c61e923",
       "appName": "agent.aragonpm.eth",
       "network": "ropsten"
-    },
-    "rinkeby-old": {
-      "registry": "0xfbae32d1cde62858bc45f51efc8cc4fa1415447e",
-      "appName": "agent.aragonpm.eth",
-      "network": "rinkeby"
     }
   },
   "roles": [

From 99c793f6c8a5dcd5794aa271c255ce26c5acc5be Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Thu, 8 Aug 2019 22:57:50 +0200
Subject: [PATCH 15/25] Agent: cosmetic reordering of external functions and
 roles in arapp

---
 apps/agent/arapp.json          | 46 ++++++++---------
 apps/agent/contracts/Agent.sol | 92 +++++++++++++++++-----------------
 2 files changed, 69 insertions(+), 69 deletions(-)

diff --git a/apps/agent/arapp.json b/apps/agent/arapp.json
index f6c476089b..361bf834e3 100644
--- a/apps/agent/arapp.json
+++ b/apps/agent/arapp.json
@@ -28,6 +28,24 @@
     }
   },
   "roles": [
+    {
+      "name": "Transfer Agent's tokens",
+      "id": "TRANSFER_ROLE",
+      "params": [
+        "Token address",
+        "Receiver address",
+        "Token amount"
+      ]
+    },
+    {
+      "name": "Execute actions",
+      "id": "EXECUTE_ROLE",
+      "params": [
+        "Target address",
+        "ETH value",
+        "Signature"
+      ]
+    },
     {
       "name": "Execute safe actions",
       "id": "SAFE_EXECUTE_ROLE",
@@ -47,44 +65,26 @@
       "params": []
     },
     {
-      "name": "Execute actions",
-      "id": "EXECUTE_ROLE",
+      "name": "Add presigned hashes",
+      "id": "ADD_PRESIGNED_HASH_ROLE",
       "params": [
-        "Target address",
-        "ETH value",
-        "Signature"
+        "Hash"
       ]
     },
     {
-      "name": "Designate signer",
+      "name": "Set designated signer",
       "id": "DESIGNATE_SIGNER_ROLE",
       "params": [
         "Designated signer address"
       ]
     },
-    {
-      "name": "Presign hash",
-      "id": "ADD_PRESIGNED_HASH_ROLE",
-      "params": [
-        "Hash"
-      ]
-    },
     {
       "name": "Run EVM Script",
       "id": "RUN_SCRIPT_ROLE",
       "params": [
         "EVM Script"
       ]
-    },
-    {
-      "name": "Transfer Agent's tokens",
-      "id": "TRANSFER_ROLE",
-      "params": [
-        "Token address",
-        "Receiver address",
-        "Token amount"
-      ]
     }
   ],
   "path": "contracts/Agent.sol"
-}
\ No newline at end of file
+}
diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index f93a91316d..b1c9730cbb 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -15,22 +15,22 @@ import "@aragon/os/contracts/common/IForwarder.sol";
 
 contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     /* Hardcoded constants to save gas
-    bytes32 public constant SAFE_EXECUTE_ROLE = keccak256("SAFE_EXECUTE_ROLE");
     bytes32 public constant EXECUTE_ROLE = keccak256("EXECUTE_ROLE");
-    bytes32 public constant RUN_SCRIPT_ROLE = keccak256("RUN_SCRIPT_ROLE");
+    bytes32 public constant SAFE_EXECUTE_ROLE = keccak256("SAFE_EXECUTE_ROLE");
     bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = keccak256("ADD_PROTECTED_TOKEN_ROLE");
     bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = keccak256("REMOVE_PROTECTED_TOKEN_ROLE");
     bytes32 public constant ADD_PRESIGNED_HASH_ROLE = keccak256("ADD_PRESIGNED_HASH_ROLE");
     bytes32 public constant DESIGNATE_SIGNER_ROLE = keccak256("DESIGNATE_SIGNER_ROLE");
+    bytes32 public constant RUN_SCRIPT_ROLE = keccak256("RUN_SCRIPT_ROLE");
     */
 
-    bytes32 public constant SAFE_EXECUTE_ROLE = 0x0a1ad7b87f5846153c6d5a1f761d71c7d0cfd122384f56066cd33239b7933694;
     bytes32 public constant EXECUTE_ROLE = 0xcebf517aa4440d1d125e0355aae64401211d0848a23c02cc5d29a14822580ba4;
-    bytes32 public constant RUN_SCRIPT_ROLE = 0xb421f7ad7646747f3051c50c0b8e2377839296cd4973e27f63821d73e390338f;
+    bytes32 public constant SAFE_EXECUTE_ROLE = 0x0a1ad7b87f5846153c6d5a1f761d71c7d0cfd122384f56066cd33239b7933694;
     bytes32 public constant ADD_PROTECTED_TOKEN_ROLE = 0x6eb2a499556bfa2872f5aa15812b956cc4a71b4d64eb3553f7073c7e41415aaa;
     bytes32 public constant REMOVE_PROTECTED_TOKEN_ROLE = 0x71eee93d500f6f065e38b27d242a756466a00a52a1dbcd6b4260f01a8640402a;
     bytes32 public constant ADD_PRESIGNED_HASH_ROLE = 0x0b29780bb523a130b3b01f231ef49ed2fa2781645591a0b0a44ca98f15a5994c;
     bytes32 public constant DESIGNATE_SIGNER_ROLE = 0x23ce341656c3f14df6692eebd4757791e33662b7dcf9970c8308303da5472b7c;
+    bytes32 public constant RUN_SCRIPT_ROLE = 0xb421f7ad7646747f3051c50c0b8e2377839296cd4973e27f63821d73e390338f;
 
     uint256 public constant PROTECTED_TOKENS_CAP = 10;
 
@@ -56,6 +56,35 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     event PresignHash(address indexed sender, bytes32 indexed hash);
     event SetDesignatedSigner(address indexed sender, address indexed oldSigner, address indexed newSigner);
 
+    /**
+    * @notice Execute '`@radspec(_target, _data)`' on `_target``_ethValue == 0 ? '' : ' (Sending' + @tokenAmount(0x0000000000000000000000000000000000000000, _ethValue) + ')'`
+    * @param _target Address where the action is being executed
+    * @param _ethValue Amount of ETH from the contract that is sent with the action
+    * @param _data Calldata for the action
+    * @return Exits call frame forwarding the return data of the executed call (either error or success data)
+    */
+    function execute(address _target, uint256 _ethValue, bytes _data)
+        external // This function MUST always be external as the function performs a low level return, exiting the Agent app execution context
+        authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(getSig(_data)))) // bytes4 casted as uint256 sets the bytes as the LSBs
+    {
+        bool result = _target.call.value(_ethValue)(_data);
+
+        if (result) {
+            emit Execute(msg.sender, _target, _ethValue, _data);
+        }
+
+        assembly {
+            let size := returndatasize
+            let ptr := mload(0x40)
+            returndatacopy(ptr, 0, size)
+
+            // revert instead of invalid() bc if the underlying call failed with invalid() it already wasted gas.
+            // if the call returned error data, forward it
+            switch result case 0 { revert(ptr, size) }
+            default { return(ptr, size) }
+        }
+    }
+
     /**
     * @notice Execute '`@radspec(_target, _data)`' on `_target` ensuring that protected tokens can't be spent
     * @param _target Address where the action is being executed
@@ -109,35 +138,6 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         }
     }
 
-    /**
-    * @notice Execute '`@radspec(_target, _data)`' on `_target``_ethValue == 0 ? '' : ' (Sending' + @tokenAmount(0x0000000000000000000000000000000000000000, _ethValue) + ')'`
-    * @param _target Address where the action is being executed
-    * @param _ethValue Amount of ETH from the contract that is sent with the action
-    * @param _data Calldata for the action
-    * @return Exits call frame forwarding the return data of the executed call (either error or success data)
-    */
-    function execute(address _target, uint256 _ethValue, bytes _data)
-        external // This function MUST always be external as the function performs a low level return, exiting the Agent app execution context
-        authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(getSig(_data)))) // bytes4 casted as uint256 sets the bytes as the LSBs
-    {
-        bool result = _target.call.value(_ethValue)(_data);
-
-        if (result) {
-            emit Execute(msg.sender, _target, _ethValue, _data);
-        }
-
-        assembly {
-            let size := returndatasize
-            let ptr := mload(0x40)
-            returndatacopy(ptr, 0, size)
-
-            // revert instead of invalid() bc if the underlying call failed with invalid() it already wasted gas.
-            // if the call returned error data, forward it
-            switch result case 0 { revert(ptr, size) }
-            default { return(ptr, size) }
-        }
-    }
-
     /**
     * @notice Add `_token.symbol(): string` to the list of protected tokens
     * @param _token Address of the token to be protected
@@ -160,6 +160,19 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         _removeProtectedToken(_token);
     }
 
+    /**
+    * @notice Pre-sign hash `_hash`
+    * @param _hash Hash that will be considered signed regardless of the signature checked with 'isValidSignature()'
+    */
+    function presignHash(bytes32 _hash)
+        external
+        authP(ADD_PRESIGNED_HASH_ROLE, arr(_hash))
+    {
+        isPresigned[_hash] = true;
+
+        emit PresignHash(msg.sender, _hash);
+    }
+
     /**
     * @notice Set `_designatedSigner` as the designated signer of the app, which will be able to sign messages on behalf of the app
     * @param _designatedSigner Address that will be able to sign messages on behalf of the app
@@ -181,19 +194,6 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         emit SetDesignatedSigner(msg.sender, oldDesignatedSigner, _designatedSigner);
     }
 
-    /**
-    * @notice Pre-sign hash `_hash`
-    * @param _hash Hash that will be considered signed regardless of the signature checked with 'isValidSignature()'
-    */
-    function presignHash(bytes32 _hash)
-        external
-        authP(ADD_PRESIGNED_HASH_ROLE, arr(_hash))
-    {
-        isPresigned[_hash] = true;
-
-        emit PresignHash(msg.sender, _hash);
-    }
-
     function isForwarder() external pure returns (bool) {
         return true;
     }

From 2946020328cff9f03b5827d9c8f24fa0c6a9c6cf Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Thu, 8 Aug 2019 23:08:19 +0200
Subject: [PATCH 16/25] Agent: cosmetic reordering of errors for usage

---
 apps/agent/contracts/Agent.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index b1c9730cbb..69d658cbf8 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -36,13 +36,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
 
     bytes4 private constant ERC165_INTERFACE_ID = 0x01ffc9a7;
 
+    string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
+    string private constant ERROR_PROTECTED_TOKENS_MODIFIED = "AGENT_PROTECTED_TOKENS_MODIFIED";
+    string private constant ERROR_PROTECTED_BALANCE_LOWERED = "AGENT_PROTECTED_BALANCE_LOWERED";
     string private constant ERROR_TOKENS_CAP_REACHED = "AGENT_TOKENS_CAP_REACHED";
     string private constant ERROR_TOKEN_NOT_ERC20 = "AGENT_TOKEN_NOT_ERC20";
     string private constant ERROR_TOKEN_ALREADY_PROTECTED = "AGENT_TOKEN_ALREADY_PROTECTED";
     string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
-    string private constant ERROR_TARGET_PROTECTED = "AGENT_TARGET_PROTECTED";
-    string private constant ERROR_PROTECTED_TOKENS_MODIFIED = "AGENT_PROTECTED_TOKENS_MODIFIED";
-    string private constant ERROR_PROTECTED_BALANCE_LOWERED = "AGENT_PROTECTED_BALANCE_LOWERED";
     string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";
 
     mapping (bytes32 => bool) public isPresigned;

From a4a97b2b57790b32c4526ba957b36a61c74d06fb Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Thu, 8 Aug 2019 23:10:45 +0200
Subject: [PATCH 17/25] Agent: cosmetic reordering of functions and conforming
 to code style in other apps

---
 apps/agent/contracts/Agent.sol | 114 +++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 42 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 69d658cbf8..5f24e93638 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -65,7 +65,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function execute(address _target, uint256 _ethValue, bytes _data)
         external // This function MUST always be external as the function performs a low level return, exiting the Agent app execution context
-        authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(getSig(_data)))) // bytes4 casted as uint256 sets the bytes as the LSBs
+        authP(EXECUTE_ROLE, arr(_target, _ethValue, uint256(_getSig(_data)))) // bytes4 casted as uint256 sets the bytes as the LSBs
     {
         bool result = _target.call.value(_ethValue)(_data);
 
@@ -91,7 +91,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @param _data Calldata for the action
     * @return Exits call frame forwarding the return data of the executed call (either error or success data)
     */
-    function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(getSig(_data)))) {
+    function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(_getSig(_data)))) {
         uint256 protectedTokensLength = protectedTokens.length;
         address[] memory protectedTokens_ = new address[](protectedTokensLength);
         uint256[] memory balances = new uint256[](protectedTokensLength);
@@ -144,8 +144,8 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
         require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
-        require(isERC20(_token), ERROR_TOKEN_NOT_ERC20);
-        require(!tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
+        require(_isERC20(_token), ERROR_TOKEN_NOT_ERC20);
+        require(!_tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
 
         _addProtectedToken(_token);
     }
@@ -155,7 +155,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @param _token Address of the token to be unprotected
     */
     function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
-        require(tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
+        require(_tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
 
         _removeProtectedToken(_token);
     }
@@ -194,16 +194,17 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         emit SetDesignatedSigner(msg.sender, oldDesignatedSigner, _designatedSigner);
     }
 
+    // Forwarding fns
+
+    /**
+    * @notice Tells whether the Agent app is a forwarder or not
+    * @dev IForwarder interface conformance
+    * @return Always true
+    */
     function isForwarder() external pure returns (bool) {
         return true;
     }
 
-    function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
-        return
-            interfaceId == ERC1271_INTERFACE_ID ||
-            interfaceId == ERC165_INTERFACE_ID;
-    }
-
     /**
     * @notice Execute the script as the Agent app
     * @dev IForwarder interface conformance. Forwards any token holder action.
@@ -211,7 +212,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     */
     function forward(bytes _evmScript)
         public
-        authP(RUN_SCRIPT_ROLE, arr(getScriptACLParam(_evmScript)))
+        authP(RUN_SCRIPT_ROLE, arr(_getScriptACLParam(_evmScript)))
     {
         bytes memory input = ""; // no input
         address[] memory blacklist = new address[](0); // no addr blacklist, can interact with anything
@@ -219,10 +220,43 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         // We don't need to emit an event here as EVMScriptRunner will emit ScriptResult if successful
     }
 
-    function isValidSignature(bytes32 hash, bytes signature) public view returns (bytes4) {
+    /**
+    * @notice Tells whether `_sender` can forward actions or not
+    * @dev IForwarder interface conformance
+    * @param _sender Address of the account intending to forward an action
+    * @return True if the given address can run scripts, false otherwise
+    */
+    function canForward(address _sender, bytes _evmScript) public view returns (bool) {
+        uint256[] memory params = new uint256[](1);
+        params[0] = _getScriptACLParam(_evmScript);
+        return canPerform(_sender, RUN_SCRIPT_ROLE, params);
+    }
+
+    // ERC-165 conformance
+
+    /**
+     * @notice Tells whether this contract supports a given ERC-165 interface
+     * @param _interfaceId Interface bytes to check
+     * @return True if this contract supports the interface
+     */
+    function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
+        return
+            _interfaceId == ERC1271_INTERFACE_ID ||
+            _interfaceId == ERC165_INTERFACE_ID;
+    }
+
+    // ERC-1271 conformance
+
+    /**
+     * @notice Tells whether a signature is seen as valid by this contract through ERC-1271
+     * @param _hash Arbitrary length data signed on the behalf of address (this)
+     * @param _signature Signature byte array associated with _data
+     * @return The ERC-1271 magic value if the signature is valid
+     */
+    function isValidSignature(bytes32 _hash, bytes _signature) public view returns (bytes4) {
         // Short-circuit in case the hash was presigned. Optimization as performing calls
         // and ecrecover is more expensive than an SLOAD.
-        if (isPresigned[hash]) {
+        if (isPresigned[_hash]) {
             return returnIsValidSignatureMagicNumber(true);
         }
 
@@ -230,17 +264,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         if (designatedSigner == address(0)) {
             isValid = false;
         } else {
-            isValid = SignatureValidator.isValidSignature(hash, designatedSigner, signature);
+            isValid = SignatureValidator.isValidSignature(_hash, designatedSigner, _signature);
         }
 
         return returnIsValidSignatureMagicNumber(isValid);
     }
 
-    function canForward(address sender, bytes evmScript) public view returns (bool) {
-        uint256[] memory params = new uint256[](1);
-        params[0] = getScriptACLParam(evmScript);
-        return canPerform(sender, RUN_SCRIPT_ROLE, params);
-    }
+    // Internal fns
 
     function _addProtectedToken(address _token) internal {
         protectedTokens.push(_token);
@@ -249,26 +279,34 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     }
 
     function _removeProtectedToken(address _token) internal {
-        protectedTokens[protectedTokenIndex(_token)] = protectedTokens[protectedTokens.length - 1];
+        protectedTokens[_protectedTokenIndex(_token)] = protectedTokens[protectedTokens.length - 1];
         delete protectedTokens[protectedTokens.length - 1];
         protectedTokens.length --;
 
         emit RemoveProtectedToken(_token);
     }
 
-    function getScriptACLParam(bytes evmScript) internal pure returns (uint256) {
-        return uint256(keccak256(abi.encodePacked(evmScript)));
+    function _isERC20(address _token) internal view returns (bool) {
+        if (!isContract(_token)) {
+            return false;
+        }
+
+        balance(_token);
+
+        return true;
     }
 
-    function getSig(bytes data) internal pure returns (bytes4 sig) {
-        if (data.length < 4) {
-            return;
+    function _protectedTokenIndex(address _token) internal view returns (uint256) {
+        for (uint i = 0; i < protectedTokens.length; i++) {
+            if (protectedTokens[i] == _token) {
+              return i;
+            }
         }
 
-        assembly { sig := mload(add(data, 0x20)) }
+        revert(ERROR_TOKEN_NOT_PROTECTED);
     }
 
-    function tokenIsProtected(address _token) internal view returns (bool) {
+    function _tokenIsProtected(address _token) internal view returns (bool) {
         for (uint256 i = 0; i < protectedTokens.length; i++) {
             if (protectedTokens[i] == _token) {
                 return true;
@@ -278,23 +316,15 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         return false;
     }
 
-    function isERC20(address _token) internal view returns (bool) {
-        if (!isContract(_token)) {
-            return false;
-        }
-
-        balance(_token);
-
-        return true;
+    function _getScriptACLParam(bytes _evmScript) internal pure returns (uint256) {
+        return uint256(keccak256(abi.encodePacked(_evmScript)));
     }
 
-    function protectedTokenIndex(address _token) internal view returns (uint256) {
-        for (uint i = 0; i < protectedTokens.length; i++) {
-            if (protectedTokens[i] == _token) {
-              return i;
-            }
+    function _getSig(bytes _data) internal pure returns (bytes4 sig) {
+        if (_data.length < 4) {
+            return;
         }
 
-        revert(ERROR_TOKEN_NOT_PROTECTED);
+        assembly { sig := mload(add(_data, 0x20)) }
     }
 }

From 1f6c9f95b60d0e48fb3dd3d232ca038d5629b50c Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Thu, 8 Aug 2019 23:13:16 +0200
Subject: [PATCH 18/25] Agent: simplify forwarding permission check

---
 apps/agent/contracts/Agent.sol  | 13 ++++++-------
 apps/agent/test/agent_shared.js |  3 ++-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 5f24e93638..5df34de340 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -44,6 +44,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     string private constant ERROR_TOKEN_ALREADY_PROTECTED = "AGENT_TOKEN_ALREADY_PROTECTED";
     string private constant ERROR_TOKEN_NOT_PROTECTED = "AGENT_TOKEN_NOT_PROTECTED";
     string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";
+    string private constant ERROR_CAN_NOT_FORWARD = "AGENT_CAN_NOT_FORWARD";
 
     mapping (bytes32 => bool) public isPresigned;
     address public designatedSigner;
@@ -210,10 +211,9 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @dev IForwarder interface conformance. Forwards any token holder action.
     * @param _evmScript Script being executed
     */
-    function forward(bytes _evmScript)
-        public
-        authP(RUN_SCRIPT_ROLE, arr(_getScriptACLParam(_evmScript)))
-    {
+    function forward(bytes _evmScript) public {
+        require(canForward(msg.sender, _evmScript), ERROR_CAN_NOT_FORWARD);
+
         bytes memory input = ""; // no input
         address[] memory blacklist = new address[](0); // no addr blacklist, can interact with anything
         runScript(_evmScript, input, blacklist);
@@ -227,9 +227,8 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @return True if the given address can run scripts, false otherwise
     */
     function canForward(address _sender, bytes _evmScript) public view returns (bool) {
-        uint256[] memory params = new uint256[](1);
-        params[0] = _getScriptACLParam(_evmScript);
-        return canPerform(_sender, RUN_SCRIPT_ROLE, params);
+        // Note that `canPerform()` implicitly does an initialization check itself
+        return canPerform(_sender, RUN_SCRIPT_ROLE, arr(_getScriptACLParam(_evmScript)));
     }
 
     // ERC-165 conformance
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 9e10c4eef5..c818f32508 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -52,6 +52,7 @@ module.exports = (
 
       // Agent errors
       AGENT_DESIGNATED_TO_SELF: "AGENT_DESIGNATED_TO_SELF",
+      AGENT_CAN_NOT_FORWARD: "AGENT_CAN_NOT_FORWARD",
     })
 
     const root = accounts[0]
@@ -467,7 +468,7 @@ module.exports = (
         assert.isFalse(await agent.canForward(nonScriptRunner, script))
         assert.equal(await executionTarget.counter(), 0)
 
-        await assertRevert(agent.forward(script, { from: nonScriptRunner }), errors.APP_AUTH_FAILED)
+        await assertRevert(agent.forward(script, { from: nonScriptRunner }), errors.AGENT_CAN_NOT_FORWARD)
         assert.equal(await executionTarget.counter(), 0)
       })
     })

From 3ac568cf9eed17abdc438379ab66523df8195ff2 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 11:10:45 +0200
Subject: [PATCH 19/25] Agent: cosmetic comment clarifications

---
 apps/agent/contracts/Agent.sol | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 5df34de340..0e7f3cd514 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -92,7 +92,10 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @param _data Calldata for the action
     * @return Exits call frame forwarding the return data of the executed call (either error or success data)
     */
-    function safeExecute(address _target, bytes _data) external authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(_getSig(_data)))) {
+    function safeExecute(address _target, bytes _data)
+        external // This function MUST always be external as the function performs a low level return, exiting the Agent app execution context
+        authP(SAFE_EXECUTE_ROLE, arr(_target, uint256(_getSig(_data)))) // bytes4 casted as uint256 sets the bytes as the LSBs
+    {
         uint256 protectedTokensLength = protectedTokens.length;
         address[] memory protectedTokens_ = new address[](protectedTokensLength);
         uint256[] memory balances = new uint256[](protectedTokensLength);
@@ -132,7 +135,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
                 return(ptr, size)
             }
         } else {
-            // if the underlying call has failed, we revert and forward [possible] returned error data
+            // if the underlying call has failed, we revert and forward returned error data
             assembly {
                 revert(ptr, size)
             }
@@ -290,6 +293,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
             return false;
         }
 
+        // Throwaway sanity check to make sure the token's `balanceOf()` does not error (for now)
         balance(_token);
 
         return true;

From 14576ae693cbd8720eb73927f9924664d96cad38 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 11:11:03 +0200
Subject: [PATCH 20/25] Agent: small code optimizations

---
 apps/agent/contracts/Agent.sol | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 0e7f3cd514..547d04731e 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -75,14 +75,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         }
 
         assembly {
-            let size := returndatasize
             let ptr := mload(0x40)
-            returndatacopy(ptr, 0, size)
+            returndatacopy(ptr, 0, returndatasize)
 
             // revert instead of invalid() bc if the underlying call failed with invalid() it already wasted gas.
             // if the call returned error data, forward it
-            switch result case 0 { revert(ptr, size) }
-            default { return(ptr, size) }
+            switch result case 0 { revert(ptr, returndatasize) }
+            default { return(ptr, returndatasize) }
         }
     }
 
@@ -99,8 +98,6 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         uint256 protectedTokensLength = protectedTokens.length;
         address[] memory protectedTokens_ = new address[](protectedTokensLength);
         uint256[] memory balances = new uint256[](protectedTokensLength);
-        bytes32 size;
-        bytes32 ptr;
 
         for (uint256 i = 0; i < protectedTokensLength; i++) {
             address token = protectedTokens[i];
@@ -113,11 +110,13 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
 
         bool result = _target.call(_data);
 
+        bytes32 ptr;
+        uint256 size;
         assembly {
             size := returndatasize
             ptr := mload(0x40)
-            mstore(0x40, add(ptr, size))
-            returndatacopy(ptr, 0, size)
+            mstore(0x40, add(ptr, returndatasize))
+            returndatacopy(ptr, 0, returndatasize)
         }
 
         if (result) {
@@ -282,8 +281,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
 
     function _removeProtectedToken(address _token) internal {
         protectedTokens[_protectedTokenIndex(_token)] = protectedTokens[protectedTokens.length - 1];
-        delete protectedTokens[protectedTokens.length - 1];
-        protectedTokens.length --;
+        protectedTokens.length--;
 
         emit RemoveProtectedToken(_token);
     }

From 22392f72ffe7fe9f68ca00ebd87ee62bf542ef7e Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 11:11:24 +0200
Subject: [PATCH 21/25] Agent: add token as parameter to add and remove
 protected token roles

---
 apps/agent/arapp.json          | 8 ++++++--
 apps/agent/contracts/Agent.sol | 4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/apps/agent/arapp.json b/apps/agent/arapp.json
index 361bf834e3..aa6734555b 100644
--- a/apps/agent/arapp.json
+++ b/apps/agent/arapp.json
@@ -57,12 +57,16 @@
     {
       "name": "Add protected tokens",
       "id": "ADD_PROTECTED_TOKEN_ROLE",
-      "params": []
+      "params": [
+        "Token"
+      ]
     },
     {
       "name": "Remove protected tokens",
       "id": "REMOVE_PROTECTED_TOKEN_ROLE",
-      "params": []
+      "params": [
+        "Token"
+      ]
     },
     {
       "name": "Add presigned hashes",
diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 547d04731e..98869253f4 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -145,7 +145,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @notice Add `_token.symbol(): string` to the list of protected tokens
     * @param _token Address of the token to be protected
     */
-    function addProtectedToken(address _token) external auth(ADD_PROTECTED_TOKEN_ROLE) {
+    function addProtectedToken(address _token) external authP(ADD_PROTECTED_TOKEN_ROLE, arr(_token)) {
         require(protectedTokens.length < PROTECTED_TOKENS_CAP, ERROR_TOKENS_CAP_REACHED);
         require(_isERC20(_token), ERROR_TOKEN_NOT_ERC20);
         require(!_tokenIsProtected(_token), ERROR_TOKEN_ALREADY_PROTECTED);
@@ -157,7 +157,7 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     * @notice Remove `_token.symbol(): string` from the list of protected tokens
     * @param _token Address of the token to be unprotected
     */
-    function removeProtectedToken(address _token) external auth(REMOVE_PROTECTED_TOKEN_ROLE) {
+    function removeProtectedToken(address _token) external authP(REMOVE_PROTECTED_TOKEN_ROLE, arr(_token)) {
         require(_tokenIsProtected(_token), ERROR_TOKEN_NOT_PROTECTED);
 
         _removeProtectedToken(_token);

From aa05a8877f33bf1ffdffa4404124badf4a2cc180 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 11:36:52 +0200
Subject: [PATCH 22/25] Agent: add getter for length of protected tokens

---
 apps/agent/contracts/Agent.sol  |  6 +++++
 apps/agent/test/agent_shared.js | 40 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/apps/agent/contracts/Agent.sol b/apps/agent/contracts/Agent.sol
index 98869253f4..cd10e00f5c 100644
--- a/apps/agent/contracts/Agent.sol
+++ b/apps/agent/contracts/Agent.sol
@@ -271,6 +271,12 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         return returnIsValidSignatureMagicNumber(isValid);
     }
 
+    // Getters
+
+    function getProtectedTokensLength() public view isInitialized returns (uint256) {
+        return protectedTokens.length;
+    }
+
     // Internal fns
 
     function _addProtectedToken(address _token) internal {
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index c818f32508..5fc4c41817 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -581,6 +581,8 @@ module.exports = (
 
             assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
             assertAmountOfEvents(receipt2, 'RemoveProtectedToken')
+
+            assert.equal(await agent.getProtectedTokensLength(), 1)
             assert.equal(await agent.protectedTokens(0), token2.address)
             await assertRevert(() => agent.protectedTokens(1)) // this should try to overflow the length of the protectedTokens array and thus revert
           })
@@ -607,6 +609,44 @@ module.exports = (
       })
     })
 
+    context('> Accessing protected tokens', () => {
+      beforeEach(async () => {
+        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+      })
+
+      context('> when there are protected tokens', () => {
+        let token
+
+        beforeEach(async () => {
+          token = await TokenMock.new(agent.address, 10000)
+          await agent.addProtectedToken(token.address, { from: authorized })
+        })
+
+        it('has correct protected tokens length', async () => {
+          assert.equal(await agent.getProtectedTokensLength(), 1)
+        })
+
+        it('can access protected token', async () => {
+          const protectedTokenAddress = await agent.protectedTokens(0)
+          assert.equal(token.address, protectedTokenAddress)
+        })
+
+        it('cannot access non-existent protected tokens', async () => {
+          assertRevert(agent.protectedTokens(1))
+        })
+      })
+
+      context('> when there are no protected tokens', () => {
+        it('has correct protected tokens length', async () => {
+          assert.equal(await agent.getProtectedTokensLength(), 0)
+        })
+
+        it('cannot access non-existent protected tokens', async () => {
+          assertRevert(agent.protectedTokens(0))
+        })
+      })
+    })
+
     context('> Signing messages', () => {
       const [_, nobody, presigner, signerDesignator] = accounts
       const HASH = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing

From eb03d9cb8b2e517eb080c5fe03dd7d1bcfe33acf Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 11:38:46 +0200
Subject: [PATCH 23/25] Agent: cosmetic reorganization of tests

---
 apps/agent/test/agent_shared.js | 340 ++++++++++++++++----------------
 1 file changed, 172 insertions(+), 168 deletions(-)

diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 5fc4c41817..637fb48c69 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -101,167 +101,6 @@ module.exports = (
       await agent.initialize()
     })
 
-    context('> Safe executing actions', () => {
-      const noData = '0x'
-      const amount = 1000
-      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9, token10
-
-      beforeEach(async () => {
-        target = await ExecutionTarget.new()
-        token1 = await TokenMock.new(agent.address, amount)
-        token2 = await TokenMock.new(agent.address, amount)
-        token3 = await TokenMock.new(agent.address, amount)
-        token4 = await TokenMock.new(agent.address, amount)
-        token5 = await TokenMock.new(agent.address, amount)
-        token6 = await TokenMock.new(agent.address, amount)
-        token7 = await TokenMock.new(agent.address, amount)
-        token8 = await TokenMock.new(agent.address, amount)
-        token9 = await TokenMock.new(agent.address, amount)
-        token10 = await TokenMock.new(agent.address, amount)
-
-        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
-        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
-        await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
-
-        await agent.addProtectedToken(token1.address, { from: authorized })
-        await agent.addProtectedToken(token2.address, { from: authorized })
-        await agent.addProtectedToken(token3.address, { from: authorized })
-        await agent.addProtectedToken(token4.address, { from: authorized })
-        await agent.addProtectedToken(token5.address, { from: authorized })
-        await agent.addProtectedToken(token6.address, { from: authorized })
-        await agent.addProtectedToken(token7.address, { from: authorized })
-        await agent.addProtectedToken(token8.address, { from: authorized })
-        await agent.addProtectedToken(token9.address, { from: authorized })
-        await agent.addProtectedToken(token10.address, { from: authorized })
-
-        assert.equal(await target.counter(), 0)
-        assert.equal(await token1.balanceOf(agent.address), amount)
-        assert.equal(await token2.balanceOf(agent.address), amount)
-      })
-
-      context('> sender has SAFE_EXECUTE_ROLE', () => {
-        context('> and target is not a protected ERC20', () => {
-          it('it can execute actions', async () => {
-            const N = 1102
-            const data = target.contract.setCounter.getData(N)
-            const receipt = await agent.safeExecute(target.address, data, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await target.counter(), N)
-          })
-
-          it('it can execute actions without data', async () => {
-            const receipt = await agent.safeExecute(target.address, noData, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await target.counter(), 1) // fallback just runs ExecutionTarget.execute()
-          })
-
-          it('it can execute cheap fallback actions', async () => {
-            const cheapFallbackTarget = await DestinationMock.new(false)
-            const receipt = await agent.safeExecute(cheapFallbackTarget.address, noData, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-          })
-
-          it('it can execute expensive fallback actions', async () => {
-            const expensiveFallbackTarget = await DestinationMock.new(true)
-            assert.equal(await expensiveFallbackTarget.counter(), 0)
-            const receipt = await agent.safeExecute(expensiveFallbackTarget.address, noData, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await expensiveFallbackTarget.counter(), 1) // fallback increments counter
-          })
-
-          it('it can execute with data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const randomData = '0x12345678'
-            const receipt = await agent.safeExecute(nonContract, randomData, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-          })
-
-          it('it can execute without data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const receipt = await agent.safeExecute(nonContract, noData, { from: authorized })
-
-            assertAmountOfEvents(receipt, 'SafeExecute')
-          })
-
-          it('it can forward success return data', async () => {
-            const { to, data } = encodeFunctionCall(target, 'execute')
-
-            // We make a call to easily get what data could be gotten inside the EVM
-            // Contract -> agent.safeExecute -> Target.func (would allow Contract to have access to this data)
-            const call = encodeFunctionCall(agent, 'safeExecute', to, data, { from: authorized })
-            const returnData = await web3Call(call)
-
-            // ExecutionTarget.execute() increments the counter by 1
-            assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
-          })
-
-          it('it should revert if executed action reverts', async () => {
-            // TODO: Check revert data was correctly forwarded
-            // ganache currently doesn't support fetching this data
-            const data = target.contract.fail.getData()
-            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
-          })
-
-          context('> but action affects a protected ERC20 balance', () => {
-            context('> action decreases protected ERC20 balance', () => {
-              it('it should revert', async () => {
-                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
-                const token11 = await TokenMock.new(agent.address, amount)
-                const approve = token11.contract.approve.getData(target.address, 10)
-                await agent.safeExecute(token11.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
-                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
-                const data = target.contract.transferTokenFrom.getData(token11.address)
-
-                await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
-              })
-            })
-
-            context('> action increases protected ERC20 balance', () => {
-              it('it should execute action', async () => {
-                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
-                const token11 = await TokenMock.new(target.address, amount)
-                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
-                const data = target.contract.transferTokenTo.getData(token11.address)
-                await agent.safeExecute(target.address, data, { from: authorized })
-
-                assert.equal((await token11.balanceOf(agent.address)).toNumber(), 1)
-              })
-            })
-          })
-
-          context('> but action affects protected tokens list', () => {
-            it('it should revert', async () => {
-              await acl.grantPermission(target.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
-              const data = target.contract.removeProtectedToken.getData(token4.address)
-
-              await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
-            })
-          })
-        })
-
-        context('> but target is a protected ERC20', () => {
-          it('it should revert', async () => {
-            const approve = token1.contract.approve.getData(target.address, 10)
-
-            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
-          })
-        })
-      })
-
-      context('> sender does not have SAFE_EXECUTE_ROLE', () => {
-        it('it should revert', async () => {
-          const data = target.contract.execute.getData()
-
-          await assertRevert(() => agent.safeExecute(target.address, data, { from: unauthorized }))
-        })
-      })
-    })
-
     context('> Executing actions', () => {
       const [_, nonExecutor, executor] = accounts
       let executionTarget
@@ -440,6 +279,167 @@ module.exports = (
       }
     })
 
+    context('> Safe executing actions', () => {
+      const noData = '0x'
+      const amount = 1000
+      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9, token10
+
+      beforeEach(async () => {
+        target = await ExecutionTarget.new()
+        token1 = await TokenMock.new(agent.address, amount)
+        token2 = await TokenMock.new(agent.address, amount)
+        token3 = await TokenMock.new(agent.address, amount)
+        token4 = await TokenMock.new(agent.address, amount)
+        token5 = await TokenMock.new(agent.address, amount)
+        token6 = await TokenMock.new(agent.address, amount)
+        token7 = await TokenMock.new(agent.address, amount)
+        token8 = await TokenMock.new(agent.address, amount)
+        token9 = await TokenMock.new(agent.address, amount)
+        token10 = await TokenMock.new(agent.address, amount)
+
+        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
+
+        await agent.addProtectedToken(token1.address, { from: authorized })
+        await agent.addProtectedToken(token2.address, { from: authorized })
+        await agent.addProtectedToken(token3.address, { from: authorized })
+        await agent.addProtectedToken(token4.address, { from: authorized })
+        await agent.addProtectedToken(token5.address, { from: authorized })
+        await agent.addProtectedToken(token6.address, { from: authorized })
+        await agent.addProtectedToken(token7.address, { from: authorized })
+        await agent.addProtectedToken(token8.address, { from: authorized })
+        await agent.addProtectedToken(token9.address, { from: authorized })
+        await agent.addProtectedToken(token10.address, { from: authorized })
+
+        assert.equal(await target.counter(), 0)
+        assert.equal(await token1.balanceOf(agent.address), amount)
+        assert.equal(await token2.balanceOf(agent.address), amount)
+      })
+
+      context('> sender has SAFE_EXECUTE_ROLE', () => {
+        context('> and target is not a protected ERC20', () => {
+          it('it can execute actions', async () => {
+            const N = 1102
+            const data = target.contract.setCounter.getData(N)
+            const receipt = await agent.safeExecute(target.address, data, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await target.counter(), N)
+          })
+
+          it('it can execute actions without data', async () => {
+            const receipt = await agent.safeExecute(target.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await target.counter(), 1) // fallback just runs ExecutionTarget.execute()
+          })
+
+          it('it can execute cheap fallback actions', async () => {
+            const cheapFallbackTarget = await DestinationMock.new(false)
+            const receipt = await agent.safeExecute(cheapFallbackTarget.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can execute expensive fallback actions', async () => {
+            const expensiveFallbackTarget = await DestinationMock.new(true)
+            assert.equal(await expensiveFallbackTarget.counter(), 0)
+            const receipt = await agent.safeExecute(expensiveFallbackTarget.address, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+            assert.equal(await expensiveFallbackTarget.counter(), 1) // fallback increments counter
+          })
+
+          it('it can execute with data when target is not a contract', async () => {
+            const nonContract = accounts[8] // random account
+            const randomData = '0x12345678'
+            const receipt = await agent.safeExecute(nonContract, randomData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can execute without data when target is not a contract', async () => {
+            const nonContract = accounts[8] // random account
+            const receipt = await agent.safeExecute(nonContract, noData, { from: authorized })
+
+            assertAmountOfEvents(receipt, 'SafeExecute')
+          })
+
+          it('it can forward success return data', async () => {
+            const { to, data } = encodeFunctionCall(target, 'execute')
+
+            // We make a call to easily get what data could be gotten inside the EVM
+            // Contract -> agent.safeExecute -> Target.func (would allow Contract to have access to this data)
+            const call = encodeFunctionCall(agent, 'safeExecute', to, data, { from: authorized })
+            const returnData = await web3Call(call)
+
+            // ExecutionTarget.execute() increments the counter by 1
+            assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
+          })
+
+          it('it should revert if executed action reverts', async () => {
+            // TODO: Check revert data was correctly forwarded
+            // ganache currently doesn't support fetching this data
+            const data = target.contract.fail.getData()
+            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+          })
+
+          context('> but action affects a protected ERC20 balance', () => {
+            context('> action decreases protected ERC20 balance', () => {
+              it('it should revert', async () => {
+                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
+                const token11 = await TokenMock.new(agent.address, amount)
+                const approve = token11.contract.approve.getData(target.address, 10)
+                await agent.safeExecute(token11.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
+                const data = target.contract.transferTokenFrom.getData(token11.address)
+
+                await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+              })
+            })
+
+            context('> action increases protected ERC20 balance', () => {
+              it('it should execute action', async () => {
+                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
+                const token11 = await TokenMock.new(target.address, amount)
+                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
+                const data = target.contract.transferTokenTo.getData(token11.address)
+                await agent.safeExecute(target.address, data, { from: authorized })
+
+                assert.equal((await token11.balanceOf(agent.address)).toNumber(), 1)
+              })
+            })
+          })
+
+          context('> but action affects protected tokens list', () => {
+            it('it should revert', async () => {
+              await acl.grantPermission(target.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
+              const data = target.contract.removeProtectedToken.getData(token4.address)
+
+              await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+            })
+          })
+        })
+
+        context('> but target is a protected ERC20', () => {
+          it('it should revert', async () => {
+            const approve = token1.contract.approve.getData(target.address, 10)
+
+            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
+          })
+        })
+      })
+
+      context('> sender does not have SAFE_EXECUTE_ROLE', () => {
+        it('it should revert', async () => {
+          const data = target.contract.execute.getData()
+
+          await assertRevert(() => agent.safeExecute(target.address, data, { from: unauthorized }))
+        })
+      })
+    })
+
     context('> Running scripts', () => {
       let executionTarget, script
       const [_, nonScriptRunner, scriptRunner] = accounts
@@ -567,24 +567,28 @@ module.exports = (
 
       context('> sender has REMOVE_PROTECTED_TOKEN_ROLE', () => {
         context('> and token is actually protected', () => {
-          it('it should remove protected token', async () => {
-            const token1 = await TokenMock.new(agent.address, 10000)
-            const token2 = await TokenMock.new(agent.address, 10000)
-            const token3 = await TokenMock.new(agent.address, 10000)
+          let token1, token2, token3
+
+          beforeEach(async () => {
+            token1 = await TokenMock.new(agent.address, 10000)
+            token2 = await TokenMock.new(agent.address, 10000)
+            token3 = await TokenMock.new(agent.address, 10000)
 
             await agent.addProtectedToken(token1.address, { from: authorized })
             await agent.addProtectedToken(token2.address, { from: authorized })
             await agent.addProtectedToken(token3.address, { from: authorized })
+          })
 
-            const receipt1 = await agent.removeProtectedToken(token3.address, { from: authorized })
-            const receipt2 = await agent.removeProtectedToken(token1.address, { from: authorized })
+          it('it should remove protected token', async () => {
+            const receipt1 = await agent.removeProtectedToken(token1.address, { from: authorized })
+            const receipt2 = await agent.removeProtectedToken(token3.address, { from: authorized })
 
             assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
             assertAmountOfEvents(receipt2, 'RemoveProtectedToken')
 
             assert.equal(await agent.getProtectedTokensLength(), 1)
             assert.equal(await agent.protectedTokens(0), token2.address)
-            await assertRevert(() => agent.protectedTokens(1)) // this should try to overflow the length of the protectedTokens array and thus revert
+            await assertRevert(() => agent.protectedTokens(1)) // this should overflow the length of the protectedTokens array and thus revert
           })
         })
 

From 9310cb60d88858c32480766fe4a7caf3bdb9f662 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 12:02:06 +0200
Subject: [PATCH 24/25] Agent: improve tests with revert strings, more checks

---
 .../contracts/test/mocks/ExecutionTarget.sol  |  14 ---
 .../mocks/TokenInteractionExecutionTarget.sol |  18 +++
 apps/agent/test/agent_shared.js               | 108 ++++++++++--------
 3 files changed, 79 insertions(+), 61 deletions(-)
 create mode 100644 apps/agent/contracts/test/mocks/TokenInteractionExecutionTarget.sol

diff --git a/apps/agent/contracts/test/mocks/ExecutionTarget.sol b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
index 0531b1ef37..fd04b8611c 100644
--- a/apps/agent/contracts/test/mocks/ExecutionTarget.sol
+++ b/apps/agent/contracts/test/mocks/ExecutionTarget.sol
@@ -1,7 +1,5 @@
 pragma solidity 0.4.24;
 
-import "@aragon/test-helpers/contracts/TokenMock.sol";
-import "../../Agent.sol";
 
 contract ExecutionTarget {
     uint public counter;
@@ -17,18 +15,6 @@ contract ExecutionTarget {
         counter = x;
     }
 
-    function transferTokenFrom(address _token) external {
-        TokenMock(_token).transferFrom(msg.sender, this, 1);
-    }
-
-    function transferTokenTo(address _token) external {
-        TokenMock(_token).transfer(msg.sender, 1);
-    }
-
-    function removeProtectedToken(address _token) external {
-        Agent(msg.sender).removeProtectedToken(_token);
-    }
-
     function fail() external pure {
         revert(REASON);
     }
diff --git a/apps/agent/contracts/test/mocks/TokenInteractionExecutionTarget.sol b/apps/agent/contracts/test/mocks/TokenInteractionExecutionTarget.sol
new file mode 100644
index 0000000000..b5d6ee9adb
--- /dev/null
+++ b/apps/agent/contracts/test/mocks/TokenInteractionExecutionTarget.sol
@@ -0,0 +1,18 @@
+pragma solidity 0.4.24;
+
+import "@aragon/test-helpers/contracts/TokenMock.sol";
+import "../../Agent.sol";
+
+contract TokenInteractionExecutionTarget {
+    function transferTokenFrom(address _token) external {
+        TokenMock(_token).transferFrom(msg.sender, this, 1);
+    }
+
+    function transferTokenTo(address _token) external {
+        TokenMock(_token).transfer(msg.sender, 1);
+    }
+
+    function removeProtectedToken(address _token) external {
+        Agent(msg.sender).removeProtectedToken(_token);
+    }
+}
diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index 637fb48c69..a3333f7578 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -29,6 +29,7 @@ module.exports = (
   const DesignatedSigner = artifacts.require('DesignatedSigner')
   const DestinationMock = artifacts.require('DestinationMock')
   const EtherTokenConstantMock = artifacts.require('EtherTokenConstantMock')
+  const TokenInteractionExecutionTarget = artifacts.require('TokenInteractionExecutionTarget')
   const TokenMock = artifacts.require('TokenMock')
 
   const NO_SIG = '0x'
@@ -49,10 +50,18 @@ module.exports = (
       INIT_ALREADY_INITIALIZED: 'INIT_ALREADY_INITIALIZED',
       INIT_NOT_INITIALIZED: 'INIT_NOT_INITIALIZED',
       RECOVER_DISALLOWED: 'RECOVER_DISALLOWED',
+      SAFE_ERC_20_BALANCE_REVERTED: 'SAFE_ERC_20_BALANCE_REVERTED',
 
       // Agent errors
-      AGENT_DESIGNATED_TO_SELF: "AGENT_DESIGNATED_TO_SELF",
-      AGENT_CAN_NOT_FORWARD: "AGENT_CAN_NOT_FORWARD",
+      AGENT_TARGET_PROTECTED: 'AGENT_TARGET_PROTECTED',
+      AGENT_PROTECTED_TOKENS_MODIFIED: 'AGENT_PROTECTED_TOKENS_MODIFIED',
+      AGENT_PROTECTED_BALANCE_LOWERED: 'AGENT_PROTECTED_BALANCE_LOWERED',
+      AGENT_TOKENS_CAP_REACHED: 'AGENT_TOKENS_CAP_REACHED',
+      AGENT_TOKEN_ALREADY_PROTECTED: 'AGENT_TOKEN_ALREADY_PROTECTED',
+      AGENT_TOKEN_NOT_ERC20: 'AGENT_TOKEN_NOT_ERC20',
+      AGENT_TOKEN_NOT_PROTECTED: 'AGENT_TOKEN_NOT_PROTECTED',
+      AGENT_DESIGNATED_TO_SELF: 'AGENT_DESIGNATED_TO_SELF',
+      AGENT_CAN_NOT_FORWARD: 'AGENT_CAN_NOT_FORWARD',
     })
 
     const root = accounts[0]
@@ -282,20 +291,12 @@ module.exports = (
     context('> Safe executing actions', () => {
       const noData = '0x'
       const amount = 1000
-      let target, token1, token2, token3, token4, token5, token6, token7, token8, token9, token10
+      let target, token1, token2
 
       beforeEach(async () => {
         target = await ExecutionTarget.new()
         token1 = await TokenMock.new(agent.address, amount)
         token2 = await TokenMock.new(agent.address, amount)
-        token3 = await TokenMock.new(agent.address, amount)
-        token4 = await TokenMock.new(agent.address, amount)
-        token5 = await TokenMock.new(agent.address, amount)
-        token6 = await TokenMock.new(agent.address, amount)
-        token7 = await TokenMock.new(agent.address, amount)
-        token8 = await TokenMock.new(agent.address, amount)
-        token9 = await TokenMock.new(agent.address, amount)
-        token10 = await TokenMock.new(agent.address, amount)
 
         await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
         await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
@@ -303,14 +304,6 @@ module.exports = (
 
         await agent.addProtectedToken(token1.address, { from: authorized })
         await agent.addProtectedToken(token2.address, { from: authorized })
-        await agent.addProtectedToken(token3.address, { from: authorized })
-        await agent.addProtectedToken(token4.address, { from: authorized })
-        await agent.addProtectedToken(token5.address, { from: authorized })
-        await agent.addProtectedToken(token6.address, { from: authorized })
-        await agent.addProtectedToken(token7.address, { from: authorized })
-        await agent.addProtectedToken(token8.address, { from: authorized })
-        await agent.addProtectedToken(token9.address, { from: authorized })
-        await agent.addProtectedToken(token10.address, { from: authorized })
 
         assert.equal(await target.counter(), 0)
         assert.equal(await token1.balanceOf(agent.address), amount)
@@ -382,42 +375,62 @@ module.exports = (
             // TODO: Check revert data was correctly forwarded
             // ganache currently doesn't support fetching this data
             const data = target.contract.fail.getData()
-            await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+            await assertRevert(agent.safeExecute(target.address, data, { from: authorized }))
           })
 
           context('> but action affects a protected ERC20 balance', () => {
+            let tokenInteractionTarget
+
+            beforeEach(async () => {
+              tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+            })
+
             context('> action decreases protected ERC20 balance', () => {
               it('it should revert', async () => {
-                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
-                const token11 = await TokenMock.new(agent.address, amount)
-                const approve = token11.contract.approve.getData(target.address, 10)
-                await agent.safeExecute(token11.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
-                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
-                const data = target.contract.transferTokenFrom.getData(token11.address)
-
-                await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+                const newToken = await TokenMock.new(agent.address, amount)
+                const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
+
+                const approve = newToken.contract.approve.getData(tokenInteractionTarget.address, 10)
+                await agent.safeExecute(newToken.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+
+                await agent.addProtectedToken(newToken.address, { from: authorized }) // new token is now protected (must do this after execution)
+                const data = tokenInteractionTarget.contract.transferTokenFrom.getData(newToken.address)
+
+                await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_BALANCE_LOWERED)
+                const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
+
+                assert.equal(initialBalance, newBalance)
               })
             })
 
             context('> action increases protected ERC20 balance', () => {
               it('it should execute action', async () => {
-                await agent.removeProtectedToken(token10.address, { from: authorized }) // leave a spot to add a new token
-                const token11 = await TokenMock.new(target.address, amount)
-                await agent.addProtectedToken(token11.address, { from: authorized }) // token11 is now protected
-                const data = target.contract.transferTokenTo.getData(token11.address)
-                await agent.safeExecute(target.address, data, { from: authorized })
+                const newToken = await TokenMock.new(tokenInteractionTarget.address, amount)
+                const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
+                await agent.addProtectedToken(newToken.address, { from: authorized }) // newToken is now protected
+
+                const data = tokenInteractionTarget.contract.transferTokenTo.getData(newToken.address)
+                await agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized })
+                const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
 
-                assert.equal((await token11.balanceOf(agent.address)).toNumber(), 1)
+                assert.equal(newBalance, 1)
+                assert.notEqual(initialBalance, newBalance)
               })
             })
           })
 
           context('> but action affects protected tokens list', () => {
+            let tokenInteractionTarget
+
+            beforeEach(async () => {
+              tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+            })
+
             it('it should revert', async () => {
-              await acl.grantPermission(target.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
-              const data = target.contract.removeProtectedToken.getData(token4.address)
+              await acl.grantPermission(tokenInteractionTarget.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
+              const data = tokenInteractionTarget.contract.removeProtectedToken.getData(token2.address)
 
-              await assertRevert(() => agent.safeExecute(target.address, data, { from: authorized }))
+              await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_TOKENS_MODIFIED)
             })
           })
         })
@@ -426,7 +439,7 @@ module.exports = (
           it('it should revert', async () => {
             const approve = token1.contract.approve.getData(target.address, 10)
 
-            await assertRevert(() => agent.safeExecute(token1.address, approve, { from: authorized }))
+            await assertRevert(agent.safeExecute(token1.address, approve, { from: authorized }), errors.AGENT_TARGET_PROTECTED)
           })
         })
       })
@@ -435,7 +448,7 @@ module.exports = (
         it('it should revert', async () => {
           const data = target.contract.execute.getData()
 
-          await assertRevert(() => agent.safeExecute(target.address, data, { from: unauthorized }))
+          await assertRevert(agent.safeExecute(target.address, data, { from: unauthorized }), errors.APP_AUTH_FAILED)
         })
       })
     })
@@ -524,7 +537,7 @@ module.exports = (
               it('it should revert', async () => {
                 const token10 = await TokenMock.new(agent.address, 10000)
 
-                await assertRevert(() => agent.addProtectedToken(token10.address, { from: authorized }))
+                await assertRevert(agent.addProtectedToken(token10.address, { from: authorized }), errors.AGENT_TOKENS_CAP_REACHED)
               })
             })
           })
@@ -534,18 +547,19 @@ module.exports = (
               const token = await TokenMock.new(agent.address, 10000)
               await agent.addProtectedToken(token.address, { from: authorized })
 
-              await assertRevert(() => agent.addProtectedToken(token.address, { from: authorized }))
+              await assertRevert(agent.addProtectedToken(token.address, { from: authorized }), errors.AGENT_TOKEN_ALREADY_PROTECTED)
             })
           })
         })
 
         context('> but token is not ERC20', () => {
           it('it should revert [token is not a contract]', async () => {
-            await assertRevert(() => agent.addProtectedToken(root, { from: authorized }))
+            await assertRevert(agent.addProtectedToken(root, { from: authorized }), errors.AGENT_TOKEN_NOT_ERC20)
           })
 
           it('it should revert [token is a contract but not an ERC20]', async () => {
-            await assertRevert(() => agent.addProtectedToken(daoFact.address, { from: authorized }))
+            // The balanceOf check reverts here, so it's a SafeERC20 error
+            await assertRevert(agent.addProtectedToken(daoFact.address, { from: authorized }), errors.SAFE_ERC_20_BALANCE_REVERTED)
           })
         })
       })
@@ -554,7 +568,7 @@ module.exports = (
         it('it should revert', async () => {
           const token = await TokenMock.new(agent.address, 10000)
 
-          await assertRevert(() => agent.addProtectedToken(token.address, { from: unauthorized }))
+          await assertRevert(agent.addProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
         })
       })
     })
@@ -588,7 +602,7 @@ module.exports = (
 
             assert.equal(await agent.getProtectedTokensLength(), 1)
             assert.equal(await agent.protectedTokens(0), token2.address)
-            await assertRevert(() => agent.protectedTokens(1)) // this should overflow the length of the protectedTokens array and thus revert
+            await assertRevert(agent.protectedTokens(1)) // this should overflow the length of the protectedTokens array and thus revert
           })
         })
 
@@ -598,7 +612,7 @@ module.exports = (
             const token2 = await TokenMock.new(agent.address, 10000)
             await agent.addProtectedToken(token1.address, { from: authorized })
 
-            await assertRevert(() => agent.removeProtectedToken(token2.address, { from: authorized }))
+            await assertRevert(agent.removeProtectedToken(token2.address, { from: authorized }), errors.AGENT_TOKEN_NOT_PROTECTED)
           })
         })
       })
@@ -608,7 +622,7 @@ module.exports = (
           const token = await TokenMock.new(agent.address, 10000)
           await agent.addProtectedToken(token.address, { from: authorized })
 
-          await assertRevert(() => agent.removeProtectedToken(token.address, { from: unauthorized }))
+          await assertRevert(agent.removeProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
         })
       })
     })

From 1e770457980ff27a3a2496d8f3cdefde6f07ae66 Mon Sep 17 00:00:00 2001
From: Brett Sun <qisheng.brett.sun@gmail.com>
Date: Fri, 9 Aug 2019 12:26:46 +0200
Subject: [PATCH 25/25] Agent: add uninitalized tests

---
 apps/agent/test/agent_shared.js | 1299 ++++++++++++++++---------------
 1 file changed, 678 insertions(+), 621 deletions(-)

diff --git a/apps/agent/test/agent_shared.js b/apps/agent/test/agent_shared.js
index a3333f7578..9878d46219 100644
--- a/apps/agent/test/agent_shared.js
+++ b/apps/agent/test/agent_shared.js
@@ -33,6 +33,7 @@ module.exports = (
   const TokenMock = artifacts.require('TokenMock')
 
   const NO_SIG = '0x'
+  const NO_DATA = '0x'
   const ERC165_SUPPORT_INVALID_ID = '0xffffffff'
   const ERC165_SUPPORT_INTERFACE_ID = '0x01ffc9a7'
 
@@ -83,11 +84,11 @@ module.exports = (
       APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
       SAFE_EXECUTE_ROLE = await agentBase.SAFE_EXECUTE_ROLE()
       EXECUTE_ROLE = await agentBase.EXECUTE_ROLE()
-      RUN_SCRIPT_ROLE = await agentBase.RUN_SCRIPT_ROLE()
       ADD_PROTECTED_TOKEN_ROLE = await agentBase.ADD_PROTECTED_TOKEN_ROLE()
       REMOVE_PROTECTED_TOKEN_ROLE = await agentBase.REMOVE_PROTECTED_TOKEN_ROLE()
       ADD_PRESIGNED_HASH_ROLE = await agentBase.ADD_PRESIGNED_HASH_ROLE()
       DESIGNATE_SIGNER_ROLE = await agentBase.DESIGNATE_SIGNER_ROLE()
+      RUN_SCRIPT_ROLE = await agentBase.RUN_SCRIPT_ROLE()
       ERC1271_INTERFACE_ID = await agentBase.ERC1271_INTERFACE_ID()
 
       const ethConstant = await EtherTokenConstantMock.new()
@@ -106,837 +107,893 @@ module.exports = (
 
       const agentReceipt = await dao.newAppInstance(agentAppId, agentBase.address, '0x', false)
       agent = AgentLike.at(getNewProxyAddress(agentReceipt))
-
-      await agent.initialize()
     })
 
-    context('> Executing actions', () => {
-      const [_, nonExecutor, executor] = accounts
-      let executionTarget
-
-      for (const depositAmount of [0, 3]) {
-        context(depositAmount ? '> With ETH' : '> Without ETH', () => {
-          beforeEach(async () => {
-            await acl.createPermission(executor, agent.address, EXECUTE_ROLE, root, { from: root })
-
-            executionTarget = await ExecutionTarget.new()
-            assert.equal(await executionTarget.counter(), 0, 'expected starting counter of execution target to be be 0')
-            assert.equal((await getBalance(executionTarget.address)).toString(), 0, 'expected starting balance of execution target to be be 0')
-
-            if (depositAmount) {
-              await agent.deposit(ETH, depositAmount, { value: depositAmount })
-            }
-            assert.equal((await getBalance(agent.address)).toString(), depositAmount, `expected starting balance of agent to be ${depositAmount}`)
-          })
+    context('> Uninitialized', () => {
+      it('cannot initialize base app', async () => {
+        const newAgent = await AgentLike.new()
+        assert.isTrue(await newAgent.isPetrified())
+        await assertRevert(newAgent.initialize(), errors.INIT_ALREADY_INITIALIZED)
+      })
 
-          it('can execute actions', async () => {
-            const N = 1102
+      it('can be initialized', async () => {
+        await agent.initialize()
+        assert.isTrue(await agent.hasInitialized())
+      })
 
-            const data = executionTarget.contract.setCounter.getData(N)
-            const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: executor })
+      it('cannot execute actions', async () => {
+        await acl.createPermission(root, agent.address, EXECUTE_ROLE, root, { from: root })
+        await assertRevert(agent.execute(accounts[8], 0, NO_DATA), errors.APP_AUTH_FAILED)
+      })
 
-            assertAmountOfEvents(receipt, 'Execute')
-            assert.equal(await executionTarget.counter(), N, `expected counter to be ${N}`)
-            assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+      it('cannot safe execute actions', async () => {
+        await acl.createPermission(root, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
+        await assertRevert(agent.safeExecute(accounts[8], NO_DATA), errors.APP_AUTH_FAILED)
+      })
 
-          it('can execute actions without data', async () => {
-            const noData = '0x'
-            const receipt = await agent.execute(executionTarget.address, depositAmount, noData, { from: executor })
+      it('cannot add protected tokens', async () => {
+        await acl.createPermission(root, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await assertRevert(agent.addProtectedToken(accounts[8]), errors.APP_AUTH_FAILED)
+      })
 
-            assertAmountOfEvents(receipt, 'Execute')
-            // Fallback just runs ExecutionTarget.execute()
-            assert.equal(await executionTarget.counter(), 1, 'expected counter to be 1')
-            assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+      it('cannot remove protected tokens', async () => {
+        await acl.createPermission(root, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
+        await assertRevert(agent.removeProtectedToken(accounts[8]), errors.APP_AUTH_FAILED)
+      })
 
-          it('can execute cheap fallback actions', async () => {
-            const cheapFallbackTarget = await DestinationMock.new(false)
-            const noData = '0x'
-            const receipt = await agent.execute(cheapFallbackTarget.address, depositAmount, noData, { from: executor })
+      it('cannot add presigned hashes', async () => {
+        const hash = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing
 
-            assertAmountOfEvents(receipt, 'Execute')
-            assert.equal((await getBalance(cheapFallbackTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+        await acl.createPermission(root, agent.address, ADD_PRESIGNED_HASH_ROLE, root, { from: root })
+        await assertRevert(agent.presignHash(hash), errors.APP_AUTH_FAILED)
+      })
 
-          it('can execute expensive fallback actions', async () => {
-            const expensiveFallbackTarget = await DestinationMock.new(true)
-            assert.equal(await expensiveFallbackTarget.counter(), 0)
+      it('cannot set designated signers', async () => {
+        await acl.createPermission(root, agent.address, DESIGNATE_SIGNER_ROLE, root, { from: root })
+        await assertRevert(agent.setDesignatedSigner(accounts[8]), errors.APP_AUTH_FAILED)
+      })
 
-            const noData = '0x'
-            const receipt = await agent.execute(expensiveFallbackTarget.address, depositAmount, noData, { from: executor })
+      it('cannot forward actions', async () => {
+        const action = { to: agent.address, calldata: agent.contract.presignHash.getData(accounts[8]) }
+        const script = encodeCallScript([action])
 
-            assertAmountOfEvents(receipt, 'Execute')
-            // Fallback increments counter
-            assert.equal(await expensiveFallbackTarget.counter(), 1)
-            assert.equal((await getBalance(expensiveFallbackTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+        await acl.createPermission(root, agent.address, RUN_SCRIPT_ROLE, root, { from: root })
+        await assertRevert(agent.forward(script), errors.AGENT_CAN_NOT_FORWARD)
+      })
+    })
 
-          it('can execute with data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const nonContractBalance = await getBalance(nonContract)
-            const randomData = '0x12345678'
+    context('> Initialized', () => {
+      beforeEach(async () => {
+        await agent.initialize()
+      })
 
-            const receipt = await agent.execute(nonContract, depositAmount, randomData, { from: executor })
+      it('fails on reinitialization', async () => {
+        await assertRevert(agent.initialize(), errors.INIT_ALREADY_INITIALIZED)
+      })
 
-            assertAmountOfEvents(receipt, 'Execute')
-            assert.equal((await getBalance(nonContract)).toString(), nonContractBalance.add(depositAmount).toString(), 'expected ending balance of non-contract to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+      context('> Executing actions', () => {
+        const [_, nonExecutor, executor] = accounts
+        let executionTarget
 
-          it('can execute without data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const nonContractBalance = await getBalance(nonContract)
-            const noData = '0x'
+        for (const depositAmount of [0, 3]) {
+          context(depositAmount ? '> With ETH' : '> Without ETH', () => {
+            beforeEach(async () => {
+              await acl.createPermission(executor, agent.address, EXECUTE_ROLE, root, { from: root })
 
-            const receipt = await agent.execute(nonContract, depositAmount, noData, { from: executor })
+              executionTarget = await ExecutionTarget.new()
+              assert.equal(await executionTarget.counter(), 0, 'expected starting counter of execution target to be be 0')
+              assert.equal((await getBalance(executionTarget.address)).toString(), 0, 'expected starting balance of execution target to be be 0')
 
-            assertAmountOfEvents(receipt, 'Execute')
-            assert.equal((await getBalance(nonContract)).toString(), nonContractBalance.add(depositAmount).toString(), 'expected ending balance of non-contract to be correct')
-            assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
-          })
+              if (depositAmount) {
+                await agent.deposit(ETH, depositAmount, { value: depositAmount })
+              }
+              assert.equal((await getBalance(agent.address)).toString(), depositAmount, `expected starting balance of agent to be ${depositAmount}`)
+            })
 
-          it('fails to execute without permissions', async () => {
-            const data = executionTarget.contract.execute.getData()
+            it('can execute actions', async () => {
+              const N = 1102
 
-            await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: nonExecutor }), errors.APP_AUTH_FAILED)
-          })
+              const data = executionTarget.contract.setCounter.getData(N)
+              const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: executor })
 
-          it('fails to execute actions with more ETH than the agent owns', async () => {
-            const data = executionTarget.contract.execute.getData()
+              assertAmountOfEvents(receipt, 'Execute')
+              assert.equal(await executionTarget.counter(), N, `expected counter to be ${N}`)
+              assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+            })
 
-            await assertRevert(agent.execute(executionTarget.address, depositAmount + 1, data, { from: executor }))
-          })
+            it('can execute actions without data', async () => {
+              const receipt = await agent.execute(executionTarget.address, depositAmount, NO_DATA, { from: executor })
 
-          it('execution forwards success return data', async () => {
-            const { to, data } = encodeFunctionCall(executionTarget, 'execute')
+              assertAmountOfEvents(receipt, 'Execute')
+              // Fallback just runs ExecutionTarget.execute()
+              assert.equal(await executionTarget.counter(), 1, 'expected counter to be 1')
+              assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+            })
 
-            // We make a call to easily get what data could be gotten inside the EVM
-            // Contract -> agent.execute -> Target.func (would allow Contract to have access to this data)
-            const call = encodeFunctionCall(agent, 'execute', to, depositAmount, data, { from: executor })
-            const returnData = await web3Call(call)
+            it('can execute cheap fallback actions', async () => {
+              const cheapFallbackTarget = await DestinationMock.new(false)
+              const receipt = await agent.execute(cheapFallbackTarget.address, depositAmount, NO_DATA, { from: executor })
 
-            // ExecutionTarget.execute() increments the counter by 1
-            assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
-          })
+              assertAmountOfEvents(receipt, 'Execute')
+              assert.equal((await getBalance(cheapFallbackTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+            })
 
-          it('it reverts if executed action reverts', async () => {
-            // TODO: Check revert data was correctly forwarded
-            // ganache currently doesn't support fetching this data
+            it('can execute expensive fallback actions', async () => {
+              const expensiveFallbackTarget = await DestinationMock.new(true)
+              assert.equal(await expensiveFallbackTarget.counter(), 0)
 
-            const data = executionTarget.contract.fail.getData()
-            await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: executor }))
-          })
+              const receipt = await agent.execute(expensiveFallbackTarget.address, depositAmount, NO_DATA, { from: executor })
 
-          context('depending on the sig ACL param', () => {
-            const [granteeEqualToSig, granteeUnequalToSig] = accounts.slice(6) // random slice from accounts
+              assertAmountOfEvents(receipt, 'Execute')
+              // Fallback increments counter
+              assert.equal(await expensiveFallbackTarget.counter(), 1)
+              assert.equal((await getBalance(expensiveFallbackTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+            })
 
-            beforeEach(async () => {
-              const sig = executionTarget.contract.setCounter.getData(1).slice(2, 10)
-              const argId = '0x02' // arg 2
-              const equalOp = '01'
-              const nonEqualOp = '02'
-              const value = `${'00'.repeat(30 - 4)}${sig}`
+            it('can execute with data when target is not a contract', async () => {
+              const nonContract = accounts[8] // random account
+              const nonContractBalance = await getBalance(nonContract)
+              const randomData = '0x12345678'
 
-              const equalParam = new web3.BigNumber(`${argId}${equalOp}${value}`)
-              const nonEqualParam = new web3.BigNumber(`${argId}${nonEqualOp}${value}`)
+              const receipt = await agent.execute(nonContract, depositAmount, randomData, { from: executor })
 
-              await acl.grantPermissionP(granteeEqualToSig, agent.address, EXECUTE_ROLE, [equalParam], { from: root })
-              await acl.grantPermissionP(granteeUnequalToSig, agent.address, EXECUTE_ROLE, [nonEqualParam], { from: root })
+              assertAmountOfEvents(receipt, 'Execute')
+              assert.equal((await getBalance(nonContract)).toString(), nonContractBalance.add(depositAmount).toString(), 'expected ending balance of non-contract to be correct')
+              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
             })
 
-            it('equal param: can execute if the signature matches', async () => {
-              const N = 1102
+            it('can execute without data when target is not a contract', async () => {
+              const nonContract = accounts[8] // random account
+              const nonContractBalance = await getBalance(nonContract)
 
-              const data = executionTarget.contract.setCounter.getData(N)
-              const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: granteeEqualToSig })
+              const receipt = await agent.execute(nonContract, depositAmount, NO_DATA, { from: executor })
 
               assertAmountOfEvents(receipt, 'Execute')
-              assert.equal(await executionTarget.counter(), N, `expected counter to be ${N}`)
-              assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+              assert.equal((await getBalance(nonContract)).toString(), nonContractBalance.add(depositAmount).toString(), 'expected ending balance of non-contract to be correct')
               assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
             })
 
-            it('not equal param: can execute if the signature doesn\'t match', async () => {
+            it('fails to execute without permissions', async () => {
               const data = executionTarget.contract.execute.getData()
-              const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: granteeUnequalToSig })
 
-              assertAmountOfEvents(receipt, 'Execute')
-              assert.equal(await executionTarget.counter(), 1, `expected counter to be ${1}`)
-              assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
-              assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+              await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: nonExecutor }), errors.APP_AUTH_FAILED)
             })
 
-            it('equal param: fails to execute if signature doesn\'t match', async () => {
+            it('fails to execute actions with more ETH than the agent owns', async () => {
               const data = executionTarget.contract.execute.getData()
 
-              await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: granteeEqualToSig }), errors.APP_AUTH_FAILED)
+              await assertRevert(agent.execute(executionTarget.address, depositAmount + 1, data, { from: executor }))
             })
 
-            it('not equal param: fails to execute if the signature matches', async () => {
-              const N = 1102
+            it('execution forwards success return data', async () => {
+              const { to, data } = encodeFunctionCall(executionTarget, 'execute')
 
-              const data = executionTarget.contract.setCounter.getData(N)
-              await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: granteeUnequalToSig }), errors.APP_AUTH_FAILED)
+              // We make a call to easily get what data could be gotten inside the EVM
+              // Contract -> agent.execute -> Target.func (would allow Contract to have access to this data)
+              const call = encodeFunctionCall(agent, 'execute', to, depositAmount, data, { from: executor })
+              const returnData = await web3Call(call)
+
+              // ExecutionTarget.execute() increments the counter by 1
+              assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
             })
-          })
-        })
-      }
-    })
 
-    context('> Safe executing actions', () => {
-      const noData = '0x'
-      const amount = 1000
-      let target, token1, token2
+            it('it reverts if executed action reverts', async () => {
+              // TODO: Check revert data was correctly forwarded
+              // ganache currently doesn't support fetching this data
 
-      beforeEach(async () => {
-        target = await ExecutionTarget.new()
-        token1 = await TokenMock.new(agent.address, amount)
-        token2 = await TokenMock.new(agent.address, amount)
+              const data = executionTarget.contract.fail.getData()
+              await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: executor }))
+            })
 
-        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
-        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
-        await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
+            context('depending on the sig ACL param', () => {
+              const [granteeEqualToSig, granteeUnequalToSig] = accounts.slice(6) // random slice from accounts
 
-        await agent.addProtectedToken(token1.address, { from: authorized })
-        await agent.addProtectedToken(token2.address, { from: authorized })
+              beforeEach(async () => {
+                const sig = executionTarget.contract.setCounter.getData(1).slice(2, 10)
+                const argId = '0x02' // arg 2
+                const equalOp = '01'
+                const nonEqualOp = '02'
+                const value = `${'00'.repeat(30 - 4)}${sig}`
 
-        assert.equal(await target.counter(), 0)
-        assert.equal(await token1.balanceOf(agent.address), amount)
-        assert.equal(await token2.balanceOf(agent.address), amount)
-      })
+                const equalParam = new web3.BigNumber(`${argId}${equalOp}${value}`)
+                const nonEqualParam = new web3.BigNumber(`${argId}${nonEqualOp}${value}`)
 
-      context('> sender has SAFE_EXECUTE_ROLE', () => {
-        context('> and target is not a protected ERC20', () => {
-          it('it can execute actions', async () => {
-            const N = 1102
-            const data = target.contract.setCounter.getData(N)
-            const receipt = await agent.safeExecute(target.address, data, { from: authorized })
+                await acl.grantPermissionP(granteeEqualToSig, agent.address, EXECUTE_ROLE, [equalParam], { from: root })
+                await acl.grantPermissionP(granteeUnequalToSig, agent.address, EXECUTE_ROLE, [nonEqualParam], { from: root })
+              })
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await target.counter(), N)
-          })
+              it('equal param: can execute if the signature matches', async () => {
+                const N = 1102
 
-          it('it can execute actions without data', async () => {
-            const receipt = await agent.safeExecute(target.address, noData, { from: authorized })
+                const data = executionTarget.contract.setCounter.getData(N)
+                const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: granteeEqualToSig })
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await target.counter(), 1) // fallback just runs ExecutionTarget.execute()
-          })
+                assertAmountOfEvents(receipt, 'Execute')
+                assert.equal(await executionTarget.counter(), N, `expected counter to be ${N}`)
+                assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+                assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+              })
 
-          it('it can execute cheap fallback actions', async () => {
-            const cheapFallbackTarget = await DestinationMock.new(false)
-            const receipt = await agent.safeExecute(cheapFallbackTarget.address, noData, { from: authorized })
+              it('not equal param: can execute if the signature doesn\'t match', async () => {
+                const data = executionTarget.contract.execute.getData()
+                const receipt = await agent.execute(executionTarget.address, depositAmount, data, { from: granteeUnequalToSig })
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
-          })
+                assertAmountOfEvents(receipt, 'Execute')
+                assert.equal(await executionTarget.counter(), 1, `expected counter to be ${1}`)
+                assert.equal((await getBalance(executionTarget.address)).toString(), depositAmount, 'expected ending balance of execution target to be correct')
+                assert.equal((await getBalance(agent.address)).toString(), 0, 'expected ending balance of agent at end to be 0')
+              })
 
-          it('it can execute expensive fallback actions', async () => {
-            const expensiveFallbackTarget = await DestinationMock.new(true)
-            assert.equal(await expensiveFallbackTarget.counter(), 0)
-            const receipt = await agent.safeExecute(expensiveFallbackTarget.address, noData, { from: authorized })
+              it('equal param: fails to execute if signature doesn\'t match', async () => {
+                const data = executionTarget.contract.execute.getData()
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
-            assert.equal(await expensiveFallbackTarget.counter(), 1) // fallback increments counter
-          })
+                await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: granteeEqualToSig }), errors.APP_AUTH_FAILED)
+              })
 
-          it('it can execute with data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const randomData = '0x12345678'
-            const receipt = await agent.safeExecute(nonContract, randomData, { from: authorized })
+              it('not equal param: fails to execute if the signature matches', async () => {
+                const N = 1102
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
+                const data = executionTarget.contract.setCounter.getData(N)
+                await assertRevert(agent.execute(executionTarget.address, depositAmount, data, { from: granteeUnequalToSig }), errors.APP_AUTH_FAILED)
+              })
+            })
           })
+        }
+      })
 
-          it('it can execute without data when target is not a contract', async () => {
-            const nonContract = accounts[8] // random account
-            const receipt = await agent.safeExecute(nonContract, noData, { from: authorized })
+      context('> Safe executing actions', () => {
+        const amount = 1000
+        let target, token1, token2
 
-            assertAmountOfEvents(receipt, 'SafeExecute')
-          })
+        beforeEach(async () => {
+          target = await ExecutionTarget.new()
+          token1 = await TokenMock.new(agent.address, amount)
+          token2 = await TokenMock.new(agent.address, amount)
 
-          it('it can forward success return data', async () => {
-            const { to, data } = encodeFunctionCall(target, 'execute')
+          await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+          await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
+          await acl.createPermission(authorized, agent.address, SAFE_EXECUTE_ROLE, root, { from: root })
 
-            // We make a call to easily get what data could be gotten inside the EVM
-            // Contract -> agent.safeExecute -> Target.func (would allow Contract to have access to this data)
-            const call = encodeFunctionCall(agent, 'safeExecute', to, data, { from: authorized })
-            const returnData = await web3Call(call)
+          await agent.addProtectedToken(token1.address, { from: authorized })
+          await agent.addProtectedToken(token2.address, { from: authorized })
 
-            // ExecutionTarget.execute() increments the counter by 1
-            assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
-          })
+          assert.equal(await target.counter(), 0)
+          assert.equal(await token1.balanceOf(agent.address), amount)
+          assert.equal(await token2.balanceOf(agent.address), amount)
+        })
 
-          it('it should revert if executed action reverts', async () => {
-            // TODO: Check revert data was correctly forwarded
-            // ganache currently doesn't support fetching this data
-            const data = target.contract.fail.getData()
-            await assertRevert(agent.safeExecute(target.address, data, { from: authorized }))
-          })
+        context('> sender has SAFE_EXECUTE_ROLE', () => {
+          context('> and target is not a protected ERC20', () => {
+            it('it can execute actions', async () => {
+              const N = 1102
+              const data = target.contract.setCounter.getData(N)
+              const receipt = await agent.safeExecute(target.address, data, { from: authorized })
+
+              assertAmountOfEvents(receipt, 'SafeExecute')
+              assert.equal(await target.counter(), N)
+            })
 
-          context('> but action affects a protected ERC20 balance', () => {
-            let tokenInteractionTarget
+            it('it can execute actions without data', async () => {
+              const receipt = await agent.safeExecute(target.address, NO_DATA, { from: authorized })
 
-            beforeEach(async () => {
-              tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+              assertAmountOfEvents(receipt, 'SafeExecute')
+              assert.equal(await target.counter(), 1) // fallback just runs ExecutionTarget.execute()
             })
 
-            context('> action decreases protected ERC20 balance', () => {
-              it('it should revert', async () => {
-                const newToken = await TokenMock.new(agent.address, amount)
-                const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
+            it('it can execute cheap fallback actions', async () => {
+              const cheapFallbackTarget = await DestinationMock.new(false)
+              const receipt = await agent.safeExecute(cheapFallbackTarget.address, NO_DATA, { from: authorized })
 
-                const approve = newToken.contract.approve.getData(tokenInteractionTarget.address, 10)
-                await agent.safeExecute(newToken.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+              assertAmountOfEvents(receipt, 'SafeExecute')
+            })
 
-                await agent.addProtectedToken(newToken.address, { from: authorized }) // new token is now protected (must do this after execution)
-                const data = tokenInteractionTarget.contract.transferTokenFrom.getData(newToken.address)
+            it('it can execute expensive fallback actions', async () => {
+              const expensiveFallbackTarget = await DestinationMock.new(true)
+              assert.equal(await expensiveFallbackTarget.counter(), 0)
+              const receipt = await agent.safeExecute(expensiveFallbackTarget.address, NO_DATA, { from: authorized })
 
-                await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_BALANCE_LOWERED)
-                const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
+              assertAmountOfEvents(receipt, 'SafeExecute')
+              assert.equal(await expensiveFallbackTarget.counter(), 1) // fallback increments counter
+            })
 
-                assert.equal(initialBalance, newBalance)
-              })
+            it('it can execute with data when target is not a contract', async () => {
+              const nonContract = accounts[8] // random account
+              const randomData = '0x12345678'
+              const receipt = await agent.safeExecute(nonContract, randomData, { from: authorized })
+
+              assertAmountOfEvents(receipt, 'SafeExecute')
+            })
+
+            it('it can execute without data when target is not a contract', async () => {
+              const nonContract = accounts[8] // random account
+              const receipt = await agent.safeExecute(nonContract, NO_DATA, { from: authorized })
+
+              assertAmountOfEvents(receipt, 'SafeExecute')
+            })
+
+            it('it can forward success return data', async () => {
+              const { to, data } = encodeFunctionCall(target, 'execute')
+
+              // We make a call to easily get what data could be gotten inside the EVM
+              // Contract -> agent.safeExecute -> Target.func (would allow Contract to have access to this data)
+              const call = encodeFunctionCall(agent, 'safeExecute', to, data, { from: authorized })
+              const returnData = await web3Call(call)
+
+              // ExecutionTarget.execute() increments the counter by 1
+              assert.equal(ethABI.decodeParameter('uint256', returnData), 1)
             })
 
-            context('> action increases protected ERC20 balance', () => {
-              it('it should execute action', async () => {
-                const newToken = await TokenMock.new(tokenInteractionTarget.address, amount)
-                const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
-                await agent.addProtectedToken(newToken.address, { from: authorized }) // newToken is now protected
+            it('it should revert if executed action reverts', async () => {
+              // TODO: Check revert data was correctly forwarded
+              // ganache currently doesn't support fetching this data
+              const data = target.contract.fail.getData()
+              await assertRevert(agent.safeExecute(target.address, data, { from: authorized }))
+            })
+
+            context('> but action affects a protected ERC20 balance', () => {
+              let tokenInteractionTarget
+
+              beforeEach(async () => {
+                tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+              })
+
+              context('> action decreases protected ERC20 balance', () => {
+                it('it should revert', async () => {
+                  const newToken = await TokenMock.new(agent.address, amount)
+                  const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
+
+                  const approve = newToken.contract.approve.getData(tokenInteractionTarget.address, 10)
+                  await agent.safeExecute(newToken.address, approve, { from: authorized }) // target is now allowed to transfer on behalf of agent
+
+                  await agent.addProtectedToken(newToken.address, { from: authorized }) // new token is now protected (must do this after execution)
+                  const data = tokenInteractionTarget.contract.transferTokenFrom.getData(newToken.address)
 
-                const data = tokenInteractionTarget.contract.transferTokenTo.getData(newToken.address)
-                await agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized })
-                const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
+                  await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_BALANCE_LOWERED)
+                  const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
 
-                assert.equal(newBalance, 1)
-                assert.notEqual(initialBalance, newBalance)
+                  assert.equal(initialBalance, newBalance)
+                })
+              })
+
+              context('> action increases protected ERC20 balance', () => {
+                it('it should execute action', async () => {
+                  const newToken = await TokenMock.new(tokenInteractionTarget.address, amount)
+                  const initialBalance = (await newToken.balanceOf(agent.address)).toNumber()
+                  await agent.addProtectedToken(newToken.address, { from: authorized }) // newToken is now protected
+
+                  const data = tokenInteractionTarget.contract.transferTokenTo.getData(newToken.address)
+                  await agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized })
+                  const newBalance = (await newToken.balanceOf(agent.address)).toNumber()
+
+                  assert.equal(newBalance, 1)
+                  assert.notEqual(initialBalance, newBalance)
+                })
               })
             })
-          })
 
-          context('> but action affects protected tokens list', () => {
-            let tokenInteractionTarget
+            context('> but action affects protected tokens list', () => {
+              let tokenInteractionTarget
 
-            beforeEach(async () => {
-              tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+              beforeEach(async () => {
+                tokenInteractionTarget = await TokenInteractionExecutionTarget.new()
+              })
+
+              it('it should revert', async () => {
+                await acl.grantPermission(tokenInteractionTarget.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
+                const data = tokenInteractionTarget.contract.removeProtectedToken.getData(token2.address)
+
+                await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_TOKENS_MODIFIED)
+              })
             })
+          })
 
+          context('> but target is a protected ERC20', () => {
             it('it should revert', async () => {
-              await acl.grantPermission(tokenInteractionTarget.address, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, { from: root }) // grant target permission to remove protected tokens
-              const data = tokenInteractionTarget.contract.removeProtectedToken.getData(token2.address)
+              const approve = token1.contract.approve.getData(target.address, 10)
 
-              await assertRevert(agent.safeExecute(tokenInteractionTarget.address, data, { from: authorized }), errors.AGENT_PROTECTED_TOKENS_MODIFIED)
+              await assertRevert(agent.safeExecute(token1.address, approve, { from: authorized }), errors.AGENT_TARGET_PROTECTED)
             })
           })
         })
 
-        context('> but target is a protected ERC20', () => {
+        context('> sender does not have SAFE_EXECUTE_ROLE', () => {
           it('it should revert', async () => {
-            const approve = token1.contract.approve.getData(target.address, 10)
+            const data = target.contract.execute.getData()
 
-            await assertRevert(agent.safeExecute(token1.address, approve, { from: authorized }), errors.AGENT_TARGET_PROTECTED)
+            await assertRevert(agent.safeExecute(target.address, data, { from: unauthorized }), errors.APP_AUTH_FAILED)
           })
         })
       })
 
-      context('> sender does not have SAFE_EXECUTE_ROLE', () => {
-        it('it should revert', async () => {
-          const data = target.contract.execute.getData()
-
-          await assertRevert(agent.safeExecute(target.address, data, { from: unauthorized }), errors.APP_AUTH_FAILED)
-        })
-      })
-    })
-
-    context('> Running scripts', () => {
-      let executionTarget, script
-      const [_, nonScriptRunner, scriptRunner] = accounts
+      context('> Running scripts', () => {
+        let executionTarget, script
+        const [_, nonScriptRunner, scriptRunner] = accounts
 
-      beforeEach(async () => {
-        executionTarget = await ExecutionTarget.new()
-        // prepare script
-        const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() }
-        script = encodeCallScript([action, action]) // perform action twice
+        beforeEach(async () => {
+          executionTarget = await ExecutionTarget.new()
+          // prepare script
+          const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() }
+          script = encodeCallScript([action, action]) // perform action twice
 
-        await acl.createPermission(scriptRunner, agent.address, RUN_SCRIPT_ROLE, root, { from: root })
-      })
+          await acl.createPermission(scriptRunner, agent.address, RUN_SCRIPT_ROLE, root, { from: root })
+        })
 
-      it('runs script', async () => {
-        assert.isTrue(await agent.canForward(scriptRunner, script))
-        assert.equal(await executionTarget.counter(), 0)
+        it('runs script', async () => {
+          assert.isTrue(await agent.canForward(scriptRunner, script))
+          assert.equal(await executionTarget.counter(), 0)
 
-        const receipt = await agent.forward(script, { from: scriptRunner })
+          const receipt = await agent.forward(script, { from: scriptRunner })
 
-        // Should execute ExecutionTarget.execute() twice
-        assert.equal(await executionTarget.counter(), 2)
-        assertAmountOfEvents(receipt, 'ScriptResult')
-      })
+          // Should execute ExecutionTarget.execute() twice
+          assert.equal(await executionTarget.counter(), 2)
+          assertAmountOfEvents(receipt, 'ScriptResult')
+        })
 
-      it('fails to run script without permissions', async () => {
-        assert.isFalse(await agent.canForward(nonScriptRunner, script))
-        assert.equal(await executionTarget.counter(), 0)
+        it('fails to run script without permissions', async () => {
+          assert.isFalse(await agent.canForward(nonScriptRunner, script))
+          assert.equal(await executionTarget.counter(), 0)
 
-        await assertRevert(agent.forward(script, { from: nonScriptRunner }), errors.AGENT_CAN_NOT_FORWARD)
-        assert.equal(await executionTarget.counter(), 0)
+          await assertRevert(agent.forward(script, { from: nonScriptRunner }), errors.AGENT_CAN_NOT_FORWARD)
+          assert.equal(await executionTarget.counter(), 0)
+        })
       })
-    })
 
-    context('> Adding protected tokens', () => {
-      beforeEach(async () => {
-        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
-      })
+      context('> Adding protected tokens', () => {
+        beforeEach(async () => {
+          await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+        })
 
-      context('> sender has ADD_PROTECTED_TOKEN_ROLE', () => {
-        context('> and token is ERC20', () => {
-          context('> and token is not already protected', () => {
-            context('> and protected tokens cap has not yet been reached', () => {
-              it('it should add protected token', async () => {
-                const token1 = await TokenMock.new(agent.address, 10000)
-                const token2 = await TokenMock.new(agent.address, 10000)
-
-                const receipt1 = await agent.addProtectedToken(token1.address, { from: authorized })
-                const receipt2 = await agent.addProtectedToken(token2.address, { from: authorized })
-
-                assertAmountOfEvents(receipt1, 'AddProtectedToken')
-                assertAmountOfEvents(receipt2, 'AddProtectedToken')
-                assert.equal(await agent.protectedTokens(0), token1.address)
-                assert.equal(await agent.protectedTokens(1), token2.address)
+        context('> sender has ADD_PROTECTED_TOKEN_ROLE', () => {
+          context('> and token is ERC20', () => {
+            context('> and token is not already protected', () => {
+              context('> and protected tokens cap has not yet been reached', () => {
+                it('it should add protected token', async () => {
+                  const token1 = await TokenMock.new(agent.address, 10000)
+                  const token2 = await TokenMock.new(agent.address, 10000)
+
+                  const receipt1 = await agent.addProtectedToken(token1.address, { from: authorized })
+                  const receipt2 = await agent.addProtectedToken(token2.address, { from: authorized })
+
+                  assertAmountOfEvents(receipt1, 'AddProtectedToken')
+                  assertAmountOfEvents(receipt2, 'AddProtectedToken')
+                  assert.equal(await agent.protectedTokens(0), token1.address)
+                  assert.equal(await agent.protectedTokens(1), token2.address)
+                })
               })
-            })
 
-            context('> but protected tokens cap has been reached', () => {
-              beforeEach(async () => {
-                const token1 = await TokenMock.new(agent.address, 1000)
-                const token2 = await TokenMock.new(agent.address, 1000)
-                const token3 = await TokenMock.new(agent.address, 1000)
-                const token4 = await TokenMock.new(agent.address, 1000)
-                const token5 = await TokenMock.new(agent.address, 1000)
-                const token6 = await TokenMock.new(agent.address, 1000)
-                const token7 = await TokenMock.new(agent.address, 1000)
-                const token8 = await TokenMock.new(agent.address, 1000)
-                const token9 = await TokenMock.new(agent.address, 1000)
-                const token10 = await TokenMock.new(agent.address, 1000)
-
-                await agent.addProtectedToken(token1.address, { from: authorized })
-                await agent.addProtectedToken(token2.address, { from: authorized })
-                await agent.addProtectedToken(token3.address, { from: authorized })
-                await agent.addProtectedToken(token4.address, { from: authorized })
-                await agent.addProtectedToken(token5.address, { from: authorized })
-                await agent.addProtectedToken(token6.address, { from: authorized })
-                await agent.addProtectedToken(token7.address, { from: authorized })
-                await agent.addProtectedToken(token8.address, { from: authorized })
-                await agent.addProtectedToken(token9.address, { from: authorized })
-                await agent.addProtectedToken(token10.address, { from: authorized })
+              context('> but protected tokens cap has been reached', () => {
+                beforeEach(async () => {
+                  const token1 = await TokenMock.new(agent.address, 1000)
+                  const token2 = await TokenMock.new(agent.address, 1000)
+                  const token3 = await TokenMock.new(agent.address, 1000)
+                  const token4 = await TokenMock.new(agent.address, 1000)
+                  const token5 = await TokenMock.new(agent.address, 1000)
+                  const token6 = await TokenMock.new(agent.address, 1000)
+                  const token7 = await TokenMock.new(agent.address, 1000)
+                  const token8 = await TokenMock.new(agent.address, 1000)
+                  const token9 = await TokenMock.new(agent.address, 1000)
+                  const token10 = await TokenMock.new(agent.address, 1000)
+
+                  await agent.addProtectedToken(token1.address, { from: authorized })
+                  await agent.addProtectedToken(token2.address, { from: authorized })
+                  await agent.addProtectedToken(token3.address, { from: authorized })
+                  await agent.addProtectedToken(token4.address, { from: authorized })
+                  await agent.addProtectedToken(token5.address, { from: authorized })
+                  await agent.addProtectedToken(token6.address, { from: authorized })
+                  await agent.addProtectedToken(token7.address, { from: authorized })
+                  await agent.addProtectedToken(token8.address, { from: authorized })
+                  await agent.addProtectedToken(token9.address, { from: authorized })
+                  await agent.addProtectedToken(token10.address, { from: authorized })
+                })
+
+                it('it should revert', async () => {
+                  const token10 = await TokenMock.new(agent.address, 10000)
+
+                  await assertRevert(agent.addProtectedToken(token10.address, { from: authorized }), errors.AGENT_TOKENS_CAP_REACHED)
+                })
               })
+            })
 
+            context('> but token is already protected', () => {
               it('it should revert', async () => {
-                const token10 = await TokenMock.new(agent.address, 10000)
+                const token = await TokenMock.new(agent.address, 10000)
+                await agent.addProtectedToken(token.address, { from: authorized })
 
-                await assertRevert(agent.addProtectedToken(token10.address, { from: authorized }), errors.AGENT_TOKENS_CAP_REACHED)
+                await assertRevert(agent.addProtectedToken(token.address, { from: authorized }), errors.AGENT_TOKEN_ALREADY_PROTECTED)
               })
             })
           })
 
-          context('> but token is already protected', () => {
-            it('it should revert', async () => {
-              const token = await TokenMock.new(agent.address, 10000)
-              await agent.addProtectedToken(token.address, { from: authorized })
+          context('> but token is not ERC20', () => {
+            it('it should revert [token is not a contract]', async () => {
+              await assertRevert(agent.addProtectedToken(root, { from: authorized }), errors.AGENT_TOKEN_NOT_ERC20)
+            })
 
-              await assertRevert(agent.addProtectedToken(token.address, { from: authorized }), errors.AGENT_TOKEN_ALREADY_PROTECTED)
+            it('it should revert [token is a contract but not an ERC20]', async () => {
+              // The balanceOf check reverts here, so it's a SafeERC20 error
+              await assertRevert(agent.addProtectedToken(daoFact.address, { from: authorized }), errors.SAFE_ERC_20_BALANCE_REVERTED)
             })
           })
         })
 
-        context('> but token is not ERC20', () => {
-          it('it should revert [token is not a contract]', async () => {
-            await assertRevert(agent.addProtectedToken(root, { from: authorized }), errors.AGENT_TOKEN_NOT_ERC20)
-          })
+        context('> sender does not have ADD_PROTECTED_TOKEN_ROLE', () => {
+          it('it should revert', async () => {
+            const token = await TokenMock.new(agent.address, 10000)
 
-          it('it should revert [token is a contract but not an ERC20]', async () => {
-            // The balanceOf check reverts here, so it's a SafeERC20 error
-            await assertRevert(agent.addProtectedToken(daoFact.address, { from: authorized }), errors.SAFE_ERC_20_BALANCE_REVERTED)
+            await assertRevert(agent.addProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
           })
         })
       })
 
-      context('> sender does not have ADD_PROTECTED_TOKEN_ROLE', () => {
-        it('it should revert', async () => {
-          const token = await TokenMock.new(agent.address, 10000)
-
-          await assertRevert(agent.addProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
+      context('> Removing protected tokens', () => {
+        beforeEach(async () => {
+          await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
+          await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
         })
-      })
-    })
 
-    context('> Removing protected tokens', () => {
-      beforeEach(async () => {
-        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
-        await acl.createPermission(authorized, agent.address, REMOVE_PROTECTED_TOKEN_ROLE, root, { from: root })
-      })
+        context('> sender has REMOVE_PROTECTED_TOKEN_ROLE', () => {
+          context('> and token is actually protected', () => {
+            let token1, token2, token3
+
+            beforeEach(async () => {
+              token1 = await TokenMock.new(agent.address, 10000)
+              token2 = await TokenMock.new(agent.address, 10000)
+              token3 = await TokenMock.new(agent.address, 10000)
 
-      context('> sender has REMOVE_PROTECTED_TOKEN_ROLE', () => {
-        context('> and token is actually protected', () => {
-          let token1, token2, token3
+              await agent.addProtectedToken(token1.address, { from: authorized })
+              await agent.addProtectedToken(token2.address, { from: authorized })
+              await agent.addProtectedToken(token3.address, { from: authorized })
+            })
 
-          beforeEach(async () => {
-            token1 = await TokenMock.new(agent.address, 10000)
-            token2 = await TokenMock.new(agent.address, 10000)
-            token3 = await TokenMock.new(agent.address, 10000)
+            it('it should remove protected token', async () => {
+              const receipt1 = await agent.removeProtectedToken(token1.address, { from: authorized })
+              const receipt2 = await agent.removeProtectedToken(token3.address, { from: authorized })
 
-            await agent.addProtectedToken(token1.address, { from: authorized })
-            await agent.addProtectedToken(token2.address, { from: authorized })
-            await agent.addProtectedToken(token3.address, { from: authorized })
-          })
+              assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
+              assertAmountOfEvents(receipt2, 'RemoveProtectedToken')
 
-          it('it should remove protected token', async () => {
-            const receipt1 = await agent.removeProtectedToken(token1.address, { from: authorized })
-            const receipt2 = await agent.removeProtectedToken(token3.address, { from: authorized })
+              assert.equal(await agent.getProtectedTokensLength(), 1)
+              assert.equal(await agent.protectedTokens(0), token2.address)
+              await assertRevert(agent.protectedTokens(1)) // this should overflow the length of the protectedTokens array and thus revert
+            })
+          })
 
-            assertAmountOfEvents(receipt1, 'RemoveProtectedToken')
-            assertAmountOfEvents(receipt2, 'RemoveProtectedToken')
+          context('> but token is not actually protected', () => {
+            it('it should revert', async () => {
+              const token1 = await TokenMock.new(agent.address, 10000)
+              const token2 = await TokenMock.new(agent.address, 10000)
+              await agent.addProtectedToken(token1.address, { from: authorized })
 
-            assert.equal(await agent.getProtectedTokensLength(), 1)
-            assert.equal(await agent.protectedTokens(0), token2.address)
-            await assertRevert(agent.protectedTokens(1)) // this should overflow the length of the protectedTokens array and thus revert
+              await assertRevert(agent.removeProtectedToken(token2.address, { from: authorized }), errors.AGENT_TOKEN_NOT_PROTECTED)
+            })
           })
         })
 
-        context('> but token is not actually protected', () => {
+        context('> sender does not have REMOVE_PROTECTED_TOKEN_ROLE', () => {
           it('it should revert', async () => {
-            const token1 = await TokenMock.new(agent.address, 10000)
-            const token2 = await TokenMock.new(agent.address, 10000)
-            await agent.addProtectedToken(token1.address, { from: authorized })
+            const token = await TokenMock.new(agent.address, 10000)
+            await agent.addProtectedToken(token.address, { from: authorized })
 
-            await assertRevert(agent.removeProtectedToken(token2.address, { from: authorized }), errors.AGENT_TOKEN_NOT_PROTECTED)
+            await assertRevert(agent.removeProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
           })
         })
       })
 
-      context('> sender does not have REMOVE_PROTECTED_TOKEN_ROLE', () => {
-        it('it should revert', async () => {
-          const token = await TokenMock.new(agent.address, 10000)
-          await agent.addProtectedToken(token.address, { from: authorized })
-
-          await assertRevert(agent.removeProtectedToken(token.address, { from: unauthorized }), errors.APP_AUTH_FAILED)
+      context('> Accessing protected tokens', () => {
+        beforeEach(async () => {
+          await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
         })
-      })
-    })
-
-    context('> Accessing protected tokens', () => {
-      beforeEach(async () => {
-        await acl.createPermission(authorized, agent.address, ADD_PROTECTED_TOKEN_ROLE, root, { from: root })
-      })
 
-      context('> when there are protected tokens', () => {
-        let token
+        context('> when there are protected tokens', () => {
+          let token
 
-        beforeEach(async () => {
-          token = await TokenMock.new(agent.address, 10000)
-          await agent.addProtectedToken(token.address, { from: authorized })
-        })
+          beforeEach(async () => {
+            token = await TokenMock.new(agent.address, 10000)
+            await agent.addProtectedToken(token.address, { from: authorized })
+          })
 
-        it('has correct protected tokens length', async () => {
-          assert.equal(await agent.getProtectedTokensLength(), 1)
-        })
+          it('has correct protected tokens length', async () => {
+            assert.equal(await agent.getProtectedTokensLength(), 1)
+          })
 
-        it('can access protected token', async () => {
-          const protectedTokenAddress = await agent.protectedTokens(0)
-          assert.equal(token.address, protectedTokenAddress)
-        })
+          it('can access protected token', async () => {
+            const protectedTokenAddress = await agent.protectedTokens(0)
+            assert.equal(token.address, protectedTokenAddress)
+          })
 
-        it('cannot access non-existent protected tokens', async () => {
-          assertRevert(agent.protectedTokens(1))
+          it('cannot access non-existent protected tokens', async () => {
+            assertRevert(agent.protectedTokens(1))
+          })
         })
-      })
 
-      context('> when there are no protected tokens', () => {
-        it('has correct protected tokens length', async () => {
-          assert.equal(await agent.getProtectedTokensLength(), 0)
-        })
+        context('> when there are no protected tokens', () => {
+          it('has correct protected tokens length', async () => {
+            assert.equal(await agent.getProtectedTokensLength(), 0)
+          })
 
-        it('cannot access non-existent protected tokens', async () => {
-          assertRevert(agent.protectedTokens(0))
+          it('cannot access non-existent protected tokens', async () => {
+            assertRevert(agent.protectedTokens(0))
+          })
         })
       })
-    })
 
-    context('> Signing messages', () => {
-      const [_, nobody, presigner, signerDesignator] = accounts
-      const HASH = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing
+      context('> Signing messages', () => {
+        const [_, nobody, presigner, signerDesignator] = accounts
+        const HASH = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing
 
-      const SIGNATURE_MODES = {
-        Invalid: '0x00',
-        EIP712: '0x01',
-        EthSign: '0x02',
-        ERC1271: '0x03',
-        NMode: '0x04',
-      }
+        const SIGNATURE_MODES = {
+          Invalid: '0x00',
+          EIP712: '0x01',
+          EthSign: '0x02',
+          ERC1271: '0x03',
+          NMode: '0x04',
+        }
 
-      const ERC1271_RETURN_VALID_SIGNATURE = '0x20c13b0b'
-      const ERC1271_RETURN_INVALID_SIGNATURE = '0x00000000'
+        const ERC1271_RETURN_VALID_SIGNATURE = '0x20c13b0b'
+        const ERC1271_RETURN_INVALID_SIGNATURE = '0x00000000'
 
-      const assertIsValidSignature = (isValid, erc1271Return) => {
-        const expectedReturn =
-          isValid
-            ? ERC1271_RETURN_VALID_SIGNATURE
-            : ERC1271_RETURN_INVALID_SIGNATURE
+        const assertIsValidSignature = (isValid, erc1271Return) => {
+          const expectedReturn =
+            isValid
+              ? ERC1271_RETURN_VALID_SIGNATURE
+              : ERC1271_RETURN_INVALID_SIGNATURE
 
-        assert.equal(erc1271Return, expectedReturn, `Expected signature to be ${isValid ? '' : 'in'}valid (returned ${erc1271Return})`)
-      }
+          assert.equal(erc1271Return, expectedReturn, `Expected signature to be ${isValid ? '' : 'in'}valid (returned ${erc1271Return})`)
+        }
 
-      beforeEach(async () => {
-        await acl.createPermission(presigner, agent.address, ADD_PRESIGNED_HASH_ROLE, root, { from: root })
-        await acl.createPermission(signerDesignator, agent.address, DESIGNATE_SIGNER_ROLE, root, { from: root })
-      })
+        beforeEach(async () => {
+          await acl.createPermission(presigner, agent.address, ADD_PRESIGNED_HASH_ROLE, root, { from: root })
+          await acl.createPermission(signerDesignator, agent.address, DESIGNATE_SIGNER_ROLE, root, { from: root })
+        })
 
-      it('complies with ERC165', async () => {
-        assert.isTrue(await agent.supportsInterface(ERC165_SUPPORT_INTERFACE_ID))
-        assert.isFalse(await agent.supportsInterface(ERC165_SUPPORT_INVALID_ID))
-      })
+        it('complies with ERC165', async () => {
+          assert.isTrue(await agent.supportsInterface(ERC165_SUPPORT_INTERFACE_ID))
+          assert.isFalse(await agent.supportsInterface(ERC165_SUPPORT_INVALID_ID))
+        })
 
-      it('supports ERC1271 interface', async () => {
-        assert.isTrue(await agent.supportsInterface(ERC1271_INTERFACE_ID))
-      })
+        it('supports ERC1271 interface', async () => {
+          assert.isTrue(await agent.supportsInterface(ERC1271_INTERFACE_ID))
+        })
 
-      it('doesn\'t support any other interface', async () => {
-        assert.isFalse(await agent.supportsInterface('0x12345678'))
-        assert.isFalse(await agent.supportsInterface('0x'))
-      })
+        it('doesn\'t support any other interface', async () => {
+          assert.isFalse(await agent.supportsInterface('0x12345678'))
+          assert.isFalse(await agent.supportsInterface('0x'))
+        })
 
-      it('isValidSignature returns false if there is not designated signer and hash isn\'t presigned', async () => {
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
-      })
+        it('isValidSignature returns false if there is not designated signer and hash isn\'t presigned', async () => {
+          assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        })
 
-      it('presigns a hash', async () => {
-        await agent.presignHash(HASH, { from: presigner })
+        it('presigns a hash', async () => {
+          await agent.presignHash(HASH, { from: presigner })
 
-        assertIsValidSignature(true, await agent.isValidSignature(HASH, NO_SIG))
-      })
+          assertIsValidSignature(true, await agent.isValidSignature(HASH, NO_SIG))
+        })
 
-      it('fails to presign a hash if not authorized', async () => {
-        await assertRevert(agent.presignHash(HASH, { from: nobody }), errors.APP_AUTH_FAILED)
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
-      })
+        it('fails to presign a hash if not authorized', async () => {
+          await assertRevert(agent.presignHash(HASH, { from: nobody }), errors.APP_AUTH_FAILED)
+          assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        })
 
-      context('> Designated signer', () => {
-        const ethSign = async (hash, signer) => {
-          const packedSig = await web3Sign(signer, hash)
+        context('> Designated signer', () => {
+          const ethSign = async (hash, signer) => {
+            const packedSig = await web3Sign(signer, hash)
 
-          return {
-            r: ethUtil.toBuffer('0x' + packedSig.substring(2, 66)),
-            s: ethUtil.toBuffer('0x' + packedSig.substring(66, 130)),
-            v: parseInt(packedSig.substring(130, 132), 16) + 27,
-            mode: ethUtil.toBuffer(SIGNATURE_MODES.EthSign)
+            return {
+              r: ethUtil.toBuffer('0x' + packedSig.substring(2, 66)),
+              s: ethUtil.toBuffer('0x' + packedSig.substring(66, 130)),
+              v: parseInt(packedSig.substring(130, 132), 16) + 27,
+              mode: ethUtil.toBuffer(SIGNATURE_MODES.EthSign)
+            }
           }
-        }
 
-        const eip712Sign = async (hash, key) => ({
-          mode: ethUtil.toBuffer(SIGNATURE_MODES.EIP712),
-          ...ethUtil.ecsign(
-            Buffer.from(hash.slice(2), 'hex'),
-            Buffer.from(key, 'hex')
-          )
-        })
+          const eip712Sign = async (hash, key) => ({
+            mode: ethUtil.toBuffer(SIGNATURE_MODES.EIP712),
+            ...ethUtil.ecsign(
+              Buffer.from(hash.slice(2), 'hex'),
+              Buffer.from(key, 'hex')
+            )
+          })
 
-        const signFunctionGenerator = (signFunction, signatureModifier) => (
-          async (hash, signerOrKey, useLegacySig = false, useInvalidV = false) => {
-            const sig = await signFunction(hash, signerOrKey)
-            const v =
-              useInvalidV
-                ? ethUtil.toBuffer(2) // force set an invalid v
-                : ethUtil.toBuffer(sig.v - (useLegacySig ? 0 : 27))
+          const signFunctionGenerator = (signFunction, signatureModifier) => (
+            async (hash, signerOrKey, useLegacySig = false, useInvalidV = false) => {
+              const sig = await signFunction(hash, signerOrKey)
+              const v =
+                useInvalidV
+                  ? ethUtil.toBuffer(2) // force set an invalid v
+                  : ethUtil.toBuffer(sig.v - (useLegacySig ? 0 : 27))
 
-            const signature = '0x' + Buffer.concat([sig.mode, sig.r, sig.s, v]).toString('hex')
-            return signatureModifier(signature)
-          }
-        )
+              const signature = '0x' + Buffer.concat([sig.mode, sig.r, sig.s, v]).toString('hex')
+              return signatureModifier(signature)
+            }
+          )
 
-        const addERC1271ModePrefix = (signature) =>
-          `${SIGNATURE_MODES.ERC1271}${signature.slice(2)}`
+          const addERC1271ModePrefix = (signature) =>
+            `${SIGNATURE_MODES.ERC1271}${signature.slice(2)}`
 
-        const createChildAgentGenerator = (designatedSigner) =>
-          async () => {
-            const agentReceipt = await dao.newAppInstance(agentAppId, agentBase.address, '0x', false)
-            const childAgent = AgentLike.at(getNewProxyAddress(agentReceipt))
+          const createChildAgentGenerator = (designatedSigner) =>
+            async () => {
+              const agentReceipt = await dao.newAppInstance(agentAppId, agentBase.address, '0x', false)
+              const childAgent = AgentLike.at(getNewProxyAddress(agentReceipt))
 
-            await childAgent.initialize()
-            await acl.createPermission(signerDesignator, childAgent.address, DESIGNATE_SIGNER_ROLE, root, { from: root })
-            await childAgent.setDesignatedSigner(designatedSigner, { from: signerDesignator })
+              await childAgent.initialize()
+              await acl.createPermission(signerDesignator, childAgent.address, DESIGNATE_SIGNER_ROLE, root, { from: root })
+              await childAgent.setDesignatedSigner(designatedSigner, { from: signerDesignator })
 
-            return childAgent.address
-          }
+              return childAgent.address
+            }
 
-        const directSignatureTests = [
-          {
-            name: 'EIP712',
-            signFunction: eip712Sign,
-            getSigner: () => '0x93070b307c373D7f9344859E909e3EEeF6E4Fd5a',
-            signerOrKey: '11bc31e7fef59610dfd6f95d2f78d2396c7b5477e4a9a54d72d9c1b76930e5c1',
-            notSignerOrKey: '7224b5bc510e01f75b10e3b6d6c903861ca91adb95a26406d1603e2d28a29e7f',
-          },
-          {
-            name: 'EthSign',
-            signFunction: ethSign,
-            getSigner: () => accounts[7],
-            signerOrKey: accounts[7],
-            notSignerOrKey: accounts[8]
-          },
-        ]
-
-        const wrappedSignatureTests = directSignatureTests.map(signatureTest => ({
-          ...signatureTest,
-          name: `ERC1271 -> ${signatureTest.name}`,
-          signatureModifier: addERC1271ModePrefix,
-          getSigner: createChildAgentGenerator(signatureTest.getSigner()),
-        }))
-
-        const signatureTests = directSignatureTests.concat(wrappedSignatureTests)
-
-        for (const {
-          name,
-          signFunction,
-          getSigner,
-          signerOrKey,
-          notSignerOrKey,
-          signatureModifier = sig => sig // defaults to identity function (returns input)
-        } of signatureTests) {
-          const sign = signFunctionGenerator(signFunction, signatureModifier)
-
-          context(`> Signature mode: ${name}`, () => {
-            beforeEach(async () => {
-              const signer = await getSigner()
-              await agent.setDesignatedSigner(signer, { from: signerDesignator })
-            })
+          const directSignatureTests = [
+            {
+              name: 'EIP712',
+              signFunction: eip712Sign,
+              getSigner: () => '0x93070b307c373D7f9344859E909e3EEeF6E4Fd5a',
+              signerOrKey: '11bc31e7fef59610dfd6f95d2f78d2396c7b5477e4a9a54d72d9c1b76930e5c1',
+              notSignerOrKey: '7224b5bc510e01f75b10e3b6d6c903861ca91adb95a26406d1603e2d28a29e7f',
+            },
+            {
+              name: 'EthSign',
+              signFunction: ethSign,
+              getSigner: () => accounts[7],
+              signerOrKey: accounts[7],
+              notSignerOrKey: accounts[8]
+            },
+          ]
+
+          const wrappedSignatureTests = directSignatureTests.map(signatureTest => ({
+            ...signatureTest,
+            name: `ERC1271 -> ${signatureTest.name}`,
+            signatureModifier: addERC1271ModePrefix,
+            getSigner: createChildAgentGenerator(signatureTest.getSigner()),
+          }))
+
+          const signatureTests = directSignatureTests.concat(wrappedSignatureTests)
+
+          for (const {
+            name,
+            signFunction,
+            getSigner,
+            signerOrKey,
+            notSignerOrKey,
+            signatureModifier = sig => sig // defaults to identity function (returns input)
+          } of signatureTests) {
+            const sign = signFunctionGenerator(signFunction, signatureModifier)
+
+            context(`> Signature mode: ${name}`, () => {
+              beforeEach(async () => {
+                const signer = await getSigner()
+                await agent.setDesignatedSigner(signer, { from: signerDesignator })
+              })
 
-            it('isValidSignature returns true to a valid signature', async () => {
-              const signature = await sign(HASH, signerOrKey)
-              assertIsValidSignature(true, await agent.isValidSignature(HASH, signature))
-            })
+              it('isValidSignature returns true to a valid signature', async () => {
+                const signature = await sign(HASH, signerOrKey)
+                assertIsValidSignature(true, await agent.isValidSignature(HASH, signature))
+              })
 
-            it('isValidSignature returns true to a valid signature with legacy version', async () => {
-              const legacyVersionSignature = await sign(HASH, signerOrKey, true)
-              assertIsValidSignature(true, await agent.isValidSignature(HASH, legacyVersionSignature))
-            })
+              it('isValidSignature returns true to a valid signature with legacy version', async () => {
+                const legacyVersionSignature = await sign(HASH, signerOrKey, true)
+                assertIsValidSignature(true, await agent.isValidSignature(HASH, legacyVersionSignature))
+              })
 
-            it('isValidSignature returns false to an invalid signature', async () => {
-              const badSignature = (await sign(HASH, signerOrKey)).slice(0, -2) // drop last byte
-              assertIsValidSignature(false, await agent.isValidSignature(HASH, badSignature))
-            })
+              it('isValidSignature returns false to an invalid signature', async () => {
+                const badSignature = (await sign(HASH, signerOrKey)).slice(0, -2) // drop last byte
+                assertIsValidSignature(false, await agent.isValidSignature(HASH, badSignature))
+              })
 
-            it('isValidSignature returns false to a signature with an invalid v', async () => {
-              const invalidVersionSignature = await sign(HASH, signerOrKey, false, true)
-              assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidVersionSignature))
-            })
+              it('isValidSignature returns false to a signature with an invalid v', async () => {
+                const invalidVersionSignature = await sign(HASH, signerOrKey, false, true)
+                assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidVersionSignature))
+              })
 
-            it('isValidSignature returns false to an unauthorized signer', async () => {
-              const otherSignature = await sign(HASH, notSignerOrKey)
-              assertIsValidSignature(false, await agent.isValidSignature(HASH, otherSignature))
+              it('isValidSignature returns false to an unauthorized signer', async () => {
+                const otherSignature = await sign(HASH, notSignerOrKey)
+                assertIsValidSignature(false, await agent.isValidSignature(HASH, otherSignature))
+              })
             })
-          })
-        }
+          }
 
-        context('> Signature mode: ERC1271', () => {
-          const ERC1271_SIG = SIGNATURE_MODES.ERC1271
+          context('> Signature mode: ERC1271', () => {
+            const ERC1271_SIG = SIGNATURE_MODES.ERC1271
 
-          const setDesignatedSignerContract = async (...params) => {
-            const designatedSigner = await DesignatedSigner.new(...params)
-            return agent.setDesignatedSigner(designatedSigner.address, { from: signerDesignator })
-          }
+            const setDesignatedSignerContract = async (...params) => {
+              const designatedSigner = await DesignatedSigner.new(...params)
+              return agent.setDesignatedSigner(designatedSigner.address, { from: signerDesignator })
+            }
 
-          it('isValidSignature returns true if designated signer returns true', async () => {
-            // true  - ERC165 interface compliant
-            // true  - any sigs are valid
-            // false - doesn't revert on checking sig
-            // false - doesn't modify state on checking sig
-            await setDesignatedSignerContract(true, true, false, false)
+            it('isValidSignature returns true if designated signer returns true', async () => {
+              // true  - ERC165 interface compliant
+              // true  - any sigs are valid
+              // false - doesn't revert on checking sig
+              // false - doesn't modify state on checking sig
+              await setDesignatedSignerContract(true, true, false, false)
 
-            assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
-          })
+              assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
+            })
 
-          it('isValidSignature returns false if designated signer returns false', async () => {
-            // true  - ERC165 interface compliant
-            // false - sigs are invalid
-            // false - doesn't revert on checking sig
-            // false - doesn't modify state on checking sig
-            await setDesignatedSignerContract(true, false, false, false)
+            it('isValidSignature returns false if designated signer returns false', async () => {
+              // true  - ERC165 interface compliant
+              // false - sigs are invalid
+              // false - doesn't revert on checking sig
+              // false - doesn't modify state on checking sig
+              await setDesignatedSignerContract(true, false, false, false)
 
-            // Signature fails check
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
-          })
+              // Signature fails check
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
+            })
 
-          it('isValidSignature returns true even if the designated signer doesnt support the interface', async () => {
-            // false - not ERC165 interface compliant
-            // true  - any sigs are valid
-            // false - doesn't revert on checking sig
-            // false - doesn't modify state on checking sig
-            await setDesignatedSignerContract(false, true, false, false)
+            it('isValidSignature returns true even if the designated signer doesnt support the interface', async () => {
+              // false - not ERC165 interface compliant
+              // true  - any sigs are valid
+              // false - doesn't revert on checking sig
+              // false - doesn't modify state on checking sig
+              await setDesignatedSignerContract(false, true, false, false)
 
-            assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
-          })
+              assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
+            })
 
-          it('isValidSignature returns false if designated signer reverts', async () => {
-            // true  - ERC165 interface compliant
-            // true  - any sigs are valid
-            // true  - reverts on checking sig
-            // false - doesn't modify state on checking sig
-            await setDesignatedSignerContract(true, true, true, false)
+            it('isValidSignature returns false if designated signer reverts', async () => {
+              // true  - ERC165 interface compliant
+              // true  - any sigs are valid
+              // true  - reverts on checking sig
+              // false - doesn't modify state on checking sig
+              await setDesignatedSignerContract(true, true, true, false)
 
-            // Reverts on checking
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
-          })
+              // Reverts on checking
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
+            })
 
-          it('isValidSignature returns false if designated signer attempts to modify state', async () => {
-            // true  - ERC165 interface compliant
-            // true  - any sigs are valid
-            // false - doesn't revert on checking sig
-            // true  - modifies state on checking sig
-            await setDesignatedSignerContract(true, true, false, true)
+            it('isValidSignature returns false if designated signer attempts to modify state', async () => {
+              // true  - ERC165 interface compliant
+              // true  - any sigs are valid
+              // false - doesn't revert on checking sig
+              // true  - modifies state on checking sig
+              await setDesignatedSignerContract(true, true, false, true)
 
-            // Checking costs too much gas
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
+              // Checking costs too much gas
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
+            })
           })
-        })
 
-        context(`> Signature mode: invalid modes`, () => {
-          const randomAccount = accounts[9]
+          context(`> Signature mode: invalid modes`, () => {
+            const randomAccount = accounts[9]
 
-          beforeEach(async () => {
-            await agent.setDesignatedSigner(randomAccount, { from: signerDesignator })
-          })
+            beforeEach(async () => {
+              await agent.setDesignatedSigner(randomAccount, { from: signerDesignator })
+            })
 
-          it('isValidSignature returns false to an empty signature', async () => {
-            const emptySig = '0x'
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, emptySig))
-          })
+            it('isValidSignature returns false to an empty signature', async () => {
+              const emptySig = '0x'
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, emptySig))
+            })
 
-          it('isValidSignature returns false to an invalid mode signature', async () => {
-            const invalidSignature = SIGNATURE_MODES.Invalid
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
-          })
+            it('isValidSignature returns false to an invalid mode signature', async () => {
+              const invalidSignature = SIGNATURE_MODES.Invalid
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
+            })
 
-          it('isValidSignature returns false to an unspecified mode signature', async () => {
-            const unspecifiedSignature = SIGNATURE_MODES.NMode
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, unspecifiedSignature))
-          })
+            it('isValidSignature returns false to an unspecified mode signature', async () => {
+              const unspecifiedSignature = SIGNATURE_MODES.NMode
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, unspecifiedSignature))
+            })
 
-          it('isValidSignature returns true to an invalid signature iff the hash was presigned', async () => {
-            const invalidSignature = SIGNATURE_MODES.Invalid
-            assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
+            it('isValidSignature returns true to an invalid signature iff the hash was presigned', async () => {
+              const invalidSignature = SIGNATURE_MODES.Invalid
+              assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
 
-            // Now presign it
-            await agent.presignHash(HASH, { from: presigner })
-            assertIsValidSignature(true, await agent.isValidSignature(HASH, invalidSignature))
+              // Now presign it
+              await agent.presignHash(HASH, { from: presigner })
+              assertIsValidSignature(true, await agent.isValidSignature(HASH, invalidSignature))
+            })
           })
-        })
 
-        context('> Signature mode: self', () => {
-          it('cannot set itself as the designated signer', async () => {
-            await assertRevert(agent.setDesignatedSigner(agent.address, { from: signerDesignator }), errors.AGENT_DESIGNATED_TO_SELF)
+          context('> Signature mode: self', () => {
+            it('cannot set itself as the designated signer', async () => {
+              await assertRevert(agent.setDesignatedSigner(agent.address, { from: signerDesignator }), errors.AGENT_DESIGNATED_TO_SELF)
+            })
           })
         })
       })