Testing improvements

contracts/test/mocks/evmscript/EVMScriptExecutorMalicious.sol

@@ -0,0 +1,24 @@
+pragma solidity ^0.4.24;
+
+import "../../../common/UnstructuredStorage.sol";
+import "../../../evmscript/executors/BaseEVMScriptExecutor.sol";
+import "../../../apps/AppStorage.sol";
+import "../../../kernel/IKernel.sol";
+
+
+contract EVMScriptExecutorMalicious is BaseEVMScriptExecutor, AppStorage {
+    bytes32 internal constant EXECUTOR_TYPE = keccak256("MALICIOUS_SCRIPT");
+
+    function execScript(bytes script, bytes, address[]) external isInitialized returns (bytes) {
+        // Use the script bytes variable to toggle between calling setKernel() and setAppId()
+        if (script[5] == 0x0) {
+            setKernel(IKernel(address(1)));
+        } else {
+            setAppId(bytes32(1));
+        }
+    }
+
+    function executorType() external pure returns (bytes32) {
+        return EXECUTOR_TYPE;
+    }
+}

contracts/test/mocks/lib/token/TokenRevertViewMethods.sol

@@ -0,0 +1,47 @@
+// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol
+
+pragma solidity 0.4.24;
+
+import "../../../../lib/math/SafeMath.sol";
+
+
+contract TokenRevertViewMethods {
+    using SafeMath for uint256;
+    mapping (address => uint256) private balances;
+    mapping (address => mapping (address => uint256)) private allowed;
+    uint256 private totalSupply_;
+    bool private allowTransfer_;
+
+    // Allow us to set the inital balance for an account on construction
+    constructor(address initialAccount, uint256 initialBalance) public {
+        balances[initialAccount] = initialBalance;
+        totalSupply_ = initialBalance;
+        allowTransfer_ = true;
+    }
+
+    function totalSupply() public view returns (uint256) {
+        require(false, "MOCK_ERROR");
+        return totalSupply_;
+    }
+
+    /**
+    * @dev Gets the balance of the specified address.
+    * @param _owner The address to query the the balance of.
+    * @return An uint256 representing the amount owned by the passed address.
+    */
+    function balanceOf(address _owner) public view returns (uint256) {
+        require(false, "MOCK_ERROR");
+        return balances[_owner];
+    }
+
+    /**
+    * @dev Function to check the amount of tokens that an owner allowed to a spender.
+    * @param _owner address The address which owns the funds.
+    * @param _spender address The address which will spend the funds.
+    * @return A uint256 specifying the amount of tokens still available for the spender.
+    */
+    function allowance(address _owner, address _spender) public view returns (uint256) {
+        require(false, "MOCK_ERROR");
+        return allowed[_owner][_spender];
+    }
+}

package.json

@@ -41,7 +41,7 @@
     "ethereumjs-abi": "^0.6.5",
     "ganache-cli": "^6.4.2",
     "mocha-lcov-reporter": "^1.3.0",
-    "solidity-coverage": "0.6.2",
+    "solidity-coverage": "^0.6.7",
     "solium": "^1.2.3",
     "truffle": "4.1.14",
     "truffle-bytecode-manager": "^1.1.1",

test/contracts/common/safe_erc20.js

@@ -1,10 +1,13 @@
 const { getEventArgument } = require('../../helpers/events')
+const { assertRevert } = require('../../helpers/assertThrow')
+const reverts = require('../../helpers/revertStrings')
 
 // Mocks
 const SafeERC20Mock = artifacts.require('SafeERC20Mock')
 const TokenMock = artifacts.require('TokenMock')
 const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock')
 const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock')
+const TokenRevertViewMethods = artifacts.require('TokenRevertViewMethods')
 
 const assertMockResult = (receipt, result) => assert.equal(getEventArgument(receipt, 'Result', 'result'), result, `result does not match`)
 
@@ -128,4 +131,33 @@ contract('SafeERC20', ([owner, receiver]) => {
       })
     })
   }
+
+  context('> Reverting view methods', () => {
+    let tokenMock
+
+    beforeEach(async () => {
+      tokenMock = await TokenRevertViewMethods.new(owner, initialBalance)
+    })
+
+    it('allowance', async () => {
+      await assertRevert(
+        safeERC20Mock.allowance(tokenMock.address, owner, owner),
+        reverts.SAFE_ERC_20_ALLOWANCE_REVERTED
+      )
+    })
+
+    it('balanceOf', async () => {
+      await assertRevert(
+        safeERC20Mock.balanceOf(tokenMock.address, owner),
+        reverts.SAFE_ERC_20_BALANCE_REVERTED
+      )
+    })
+
+    it('totalSupply', async () => {
+      await assertRevert(
+        safeERC20Mock.totalSupply(tokenMock.address),
+        reverts.SAFE_ERC_20_ALLOWANCE_REVERTED
+      )
+    })
+  })
 })

test/contracts/evmscript/evm_script.js

@@ -19,6 +19,7 @@ const ExecutionTarget = artifacts.require('ExecutionTarget')
 const EVMScriptExecutorMock = artifacts.require('EVMScriptExecutorMock')
 const EVMScriptExecutorNoReturnMock = artifacts.require('EVMScriptExecutorNoReturnMock')
 const EVMScriptExecutorRevertMock = artifacts.require('EVMScriptExecutorRevertMock')
+const EVMScriptExecutorMalicious = artifacts.require('EVMScriptExecutorMalicious')
 const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock')
 
 const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
@@ -359,6 +360,39 @@ contract('EVM Script', ([_, boss]) => {
       })
     })
 
+    context('> State changing script', () => {
+      beforeEach(async () => {
+        // Deploy malicious executor
+        const scriptExecutorMalicious = await EVMScriptExecutorMalicious.new()
+
+        // Install mock reverting executor onto registry
+        await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss })
+        const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMalicious.address, { from: boss })
+
+        // Sanity check it's at spec ID 1
+        const executorSpecId = getEventArgument(receipt, 'EnableExecutor', 'executorId')
+        const [, executorEnabled] = await evmScriptReg.executors(executorSpecId)
+        assert.equal(executorSpecId, 1, 'EVMScriptExecutorMalicious should be installed as spec ID 1')
+        assert.isTrue(executorEnabled, "malicious executor should be enabled")
+      })
+
+      it('fails if the kernel address changes', async () => {
+        // EVMScriptExecutorMalicious will call setKernel() 
+        // if the address is zero
+        const action = { to: ZERO_ADDR, calldata: "0x0" }
+        const script = encodeCallScript([action])
+        await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMRUN_PROTECTED_STATE_MODIFIED)        
+      })
+
+      it('fails if appId changes', async () => {
+        // EVMScriptExecutorMalicious will call setAppId()
+        // if the address is NOT zero
+        const action = { to: "0x"+"ff".repeat(20), calldata: "0x0" }
+        const script = encodeCallScript([action])
+        await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMRUN_PROTECTED_STATE_MODIFIED)        
+      })
+    })
+
     context('> CallsScript', () => {
       let callsScriptBase, executionTarget
 

test/contracts/factory/app_proxy_factory.js

@@ -0,0 +1,80 @@
+const { hash } = require('eth-ens-namehash')
+const { assertEvent } = require('../../helpers/assertEvent')(web3)
+const { getEventArgument } = require('../../helpers/events')
+const web3EthAbi = require('web3-eth-abi');
+
+const ACL = artifacts.require('ACL')
+const Kernel = artifacts.require('Kernel')
+const KernelProxy = artifacts.require('KernelProxy')
+const AppStorage = artifacts.require('AppStorage')
+const AppProxyFactory = artifacts.require('AppProxyFactory')
+const AppStub = artifacts.require('AppStub')
+
+// Mocks
+const APP_ID = hash('stub.aragonpm.test')
+
+contract('App Proxy Factory', accounts => {
+  let aclBase, kernel, kernelBase, appBase, appProxyFactory
+  let APP_BASES_NAMESPACE
+
+  const permissionsRoot = accounts[0]
+
+  before(async () => {
+    aclBase = await ACL.new()
+    appBase = await AppStub.new()
+
+    // Setup constants
+    kernelBase = await Kernel.new(true)
+    APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE()
+    kernel = Kernel.at((await KernelProxy.new(kernelBase.address)).address)
+    await kernel.initialize(aclBase.address, permissionsRoot)
+    acl = ACL.at(await kernel.acl())
+
+    // Set up app management permissions
+    const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
+    await acl.createPermission(permissionsRoot, kernel.address, APP_MANAGER_ROLE, permissionsRoot)
+
+    // Set app
+    await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase.address)
+
+    // Deploy test instance
+    appProxyFactory = await AppProxyFactory.new()
+  })
+
+  it('deploys a new App Proxy without initialization payload', async () => {
+    const receipt = await appProxyFactory.newAppProxy(kernel.address, APP_ID)
+
+    assertEvent(receipt, 'NewAppProxy', { isUpgradeable: true, appId: APP_ID })
+
+    const appProxy = AppStorage.at(getEventArgument(receipt, 'NewAppProxy', 'proxy'))
+    assert.equal(await appProxy.kernel(), kernel.address, 'should have set the correct kernel')
+    assert.equal(await appProxy.appId(), APP_ID, 'should have set the correct appId')
+  })
+
+  it('deploys a new App Proxy Pinned without initialization payload', async () => {
+    // Manually sending the transaction to overcome truffle@4.x limitations with overloaded functions
+    const receipt = await appProxyFactory.sendTransaction({
+      from: permissionsRoot,
+      data: web3EthAbi.encodeFunctionCall({
+        "constant": false,
+        "inputs": [
+          { "name": "_kernel", "type": "address" },
+          { "name": "_appId", "type": "bytes32" }
+        ],
+        "name": "newAppProxyPinned",
+        "outputs": [
+          { "name": "", "type": "address" }
+        ],
+        "payable": false,
+        "stateMutability": "nonpayable",
+        "type": "function"
+    }, [kernel.address, APP_ID])
+    })
+
+    assertEvent(receipt, 'NewAppProxy', { isUpgradeable: false, appId: APP_ID })
+
+    const appProxy = AppStorage.at(getEventArgument(receipt, 'NewAppProxy', 'proxy'))
+    assert.equal(await appProxy.kernel(), kernel.address, 'should have set the correct kernel')
+    assert.equal(await appProxy.appId(), APP_ID, 'should have set the correct appId')
+  })
+})