diff --git a/contracts/test/mocks/evmscript/EVMScriptExecutorMalicious.sol b/contracts/test/mocks/evmscript/EVMScriptExecutorMalicious.sol new file mode 100644 index 000000000..7d3646ae3 --- /dev/null +++ b/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; + } +} diff --git a/contracts/test/mocks/lib/token/TokenRevertViewMethods.sol b/contracts/test/mocks/lib/token/TokenRevertViewMethods.sol new file mode 100644 index 000000000..8919486b5 --- /dev/null +++ b/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]; + } +} diff --git a/package.json b/package.json index 2c02b0ef1..fe7fcd5d3 100644 --- a/package.json +++ b/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", diff --git a/test/contracts/common/safe_erc20.js b/test/contracts/common/safe_erc20.js index 2b8d58505..f3940aec2 100644 --- a/test/contracts/common/safe_erc20.js +++ b/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 + ) + }) + }) }) diff --git a/test/contracts/evmscript/evm_script.js b/test/contracts/evmscript/evm_script.js index 3b36ae593..a2dfa47ee 100644 --- a/test/contracts/evmscript/evm_script.js +++ b/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 diff --git a/test/contracts/factory/app_proxy_factory.js b/test/contracts/factory/app_proxy_factory.js new file mode 100644 index 000000000..a700ae7e9 --- /dev/null +++ b/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') + }) +})