From 25be5a7c5faf8b8bf39a4585f356d48f362c40ce Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 18 May 2018 00:07:00 -0700 Subject: [PATCH 1/3] Revert "Require minimum return data from EVMScript executors to prevent self-destructs" This reverts commit dede447f426c51a45cc907d0da5071002e3c0eb6. --- contracts/evmscript/EVMScriptRunner.sol | 8 +- contracts/evmscript/executors/CallsScript.sol | 5 -- .../evmscript/executors/DelegateScript.sol | 10 +-- .../executors/DeployDelegateScript.sol | 2 +- test/evm_script.js | 76 +------------------ test/mocks/Delegator.sol | 13 +--- test/mocks/MockDyingScriptExecutor.sol | 12 --- 7 files changed, 11 insertions(+), 115 deletions(-) delete mode 100644 test/mocks/MockDyingScriptExecutor.sol diff --git a/contracts/evmscript/EVMScriptRunner.sol b/contracts/evmscript/EVMScriptRunner.sol index 028232523..5cb4b3ed6 100644 --- a/contracts/evmscript/EVMScriptRunner.sol +++ b/contracts/evmscript/EVMScriptRunner.sol @@ -20,11 +20,7 @@ contract EVMScriptRunner is AppStorage, EVMScriptRegistryConstants { require(executorAddr.delegatecall(sig, calldataArgs)); - bytes memory ret = returnedDataDecoded(); - - require(ret.length > 0); - - return ret; + return returnedDataDecoded(); } function getExecutor(bytes _script) public view returns (IEVMScriptExecutor) { @@ -61,4 +57,4 @@ contract EVMScriptRunner is AppStorage, EVMScriptRegistryConstants { require(kernel == preKernel); require(appId == preAppId); } -} +} \ No newline at end of file diff --git a/contracts/evmscript/executors/CallsScript.sol b/contracts/evmscript/executors/CallsScript.sol index dcbd3d978..f2f62fd19 100644 --- a/contracts/evmscript/executors/CallsScript.sol +++ b/contracts/evmscript/executors/CallsScript.sol @@ -47,10 +47,5 @@ contract CallsScript is IEVMScriptExecutor { switch success case 0 { revert(0, 0) } } } - - bytes memory ret = new bytes(1); - ret[0] = 0x01; - - return ret; } } diff --git a/contracts/evmscript/executors/DelegateScript.sol b/contracts/evmscript/executors/DelegateScript.sol index 960da2220..5e22cc05d 100644 --- a/contracts/evmscript/executors/DelegateScript.sol +++ b/contracts/evmscript/executors/DelegateScript.sol @@ -6,7 +6,7 @@ import "../../common/IsContract.sol"; interface DelegateScriptTarget { - function exec() public returns (bool); + function exec() public; } @@ -36,11 +36,7 @@ contract DelegateScript is IEVMScriptExecutor, IsContract { function delegate(address _addr, bytes memory _input) internal returns (bytes memory output) { require(isContract(_addr)); require(_addr.delegatecall(_input.length > 0 ? _input : defaultInput())); - bytes memory ret = returnedData(); - - require(ret.length > 0); - - return ret; + return returnedData(); } function defaultInput() internal pure returns (bytes) { @@ -60,4 +56,4 @@ contract DelegateScript is IEVMScriptExecutor, IsContract { } return ret; } -} +} \ No newline at end of file diff --git a/contracts/evmscript/executors/DeployDelegateScript.sol b/contracts/evmscript/executors/DeployDelegateScript.sol index 3f5b6c014..d2847709a 100644 --- a/contracts/evmscript/executors/DeployDelegateScript.sol +++ b/contracts/evmscript/executors/DeployDelegateScript.sol @@ -43,4 +43,4 @@ contract DeployDelegateScript is DelegateScript { case 1 { revert(0, 0) } // throw if contract failed to deploy } } -} +} \ No newline at end of file diff --git a/test/evm_script.js b/test/evm_script.js index 0c2357f44..5f4b25cb5 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -7,9 +7,7 @@ const ExecutionTarget = artifacts.require('ExecutionTarget') const Executor = artifacts.require('Executor') const Delegator = artifacts.require('Delegator') const FailingDelegator = artifacts.require('FailingDelegator') -const DyingDelegator = artifacts.require('DyingDelegator') const FailingDeployment = artifacts.require('FailingDeployment') -const MockDyingScriptExecutor = artifacts.require('MockDyingScriptExecutor') const Kernel = artifacts.require('Kernel') const ACL = artifacts.require('ACL') @@ -73,8 +71,6 @@ contract('EVM Script', accounts => { const receipt = await dao.newAppInstance(executorAppId, baseExecutor.address, { from: boss }) executor = Executor.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) executionTarget = await ExecutionTarget.new() - - await acl.grantPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), { from: boss }) }) it('fails to execute if spec ID is 0', async () => { @@ -90,6 +86,7 @@ contract('EVM Script', accounts => { }) it('can disable executors', async () => { + await acl.grantPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), { from: boss }) await reg.disableScriptExecutor(1, { from: boss }) return assertRevert(async () => { await executor.execute(encodeCallScript([])) @@ -208,14 +205,6 @@ contract('EVM Script', accounts => { }) }) - it('fails if underlying call selfdestructs', async () => { - const dyingDelegator = await DyingDelegator.new() - return assertRevert(async () => { - // extra gas to avoid oog - await executor.executeWithBan(encodeDelegate(dyingDelegator.address), [], { gas: 2e6 }) - }) - }) - it('fails if calling to non-contract', async () => { return assertRevert(async () => { await executor.execute(encodeDelegate(accounts[0])) // addr is too small @@ -290,68 +279,5 @@ contract('EVM Script', accounts => { }) }) }) - - context('adding mock dying executor', () => { - let dyingExecutor = null - - beforeEach(async () => { - dyingExecutor = await MockDyingScriptExecutor.new() - await reg.addScriptExecutor(dyingExecutor.address, { from: boss }) - }) - - it('registers new executor', async () => { - assert.equal(await reg.getScriptExecutor('0x00000004'), dyingExecutor.address) - }) - - it('executes mock executor', async () => { - await executor.execute('0x00000004') - }) - - it('fails when executor selfdestructs', async () => { - return assertRevert(async () => { - // passing some input makes executor to selfdestruct - await executor.executeWithIO('0x00000004', '0x0001', []) - }) - }) - }) - - context('script overflow', async () => { - const encodeCallScriptBad = actions => { - return actions.reduce((script, { to, calldata }) => { - const addr = rawEncode(['address'], [to]).toString('hex') - // length should be (calldata.length - 2) / 2 instead of calldata.length - // as this one is bigger, it would overflow and therefore must revert - const length = rawEncode(['uint256'], [calldata.length]).toString('hex') - - // Remove 12 first 0s of padding for addr and 28 0s for uint32 - return script + addr.slice(24) + length.slice(56) + calldata.slice(2) - }, '0x00000001') // spec 1 - } - - it('fails if data length is too big', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScriptBad([action]) - - return assertRevert(async () => { - await executor.execute(script) - }) - }) - }) - }) - - context('isContract tests', async () => { - let delegateScript - - before(async () => { - delegateScript = await getContract('DelegateScriptWrapper').new() - }) - - it('fails if address is 0', async () => { - assert.isFalse(await delegateScript.isContractWrapper('0x0'), "should return false") - }) - - it('fails if dst is not a contract', async () => { - assert.isFalse(await delegateScript.isContractWrapper('0x1234'), "should return false") - }) }) }) diff --git a/test/mocks/Delegator.sol b/test/mocks/Delegator.sol index d2cce16bb..469436215 100644 --- a/test/mocks/Delegator.sol +++ b/test/mocks/Delegator.sol @@ -5,7 +5,7 @@ import "./Executor.sol"; contract Delegator is ExecutorStorage, DelegateScriptTarget { - function exec() public returns (bool) { + function exec() public { randomNumber += 1234; } @@ -14,12 +14,7 @@ contract Delegator is ExecutorStorage, DelegateScriptTarget { contract FailingDelegator is DelegateScriptTarget { - function exec() public returns (bool) { revert(); } -} - - -contract DyingDelegator is DelegateScriptTarget { - function exec() public returns (bool) { selfdestruct(0); } + function exec() public { revert(); } } @@ -29,14 +24,14 @@ contract FailingDeployment { contract ProtectionModifierKernel is ExecutorStorage, DelegateScriptTarget { - function exec() public returns (bool) { + function exec() public { kernel = IKernel(0x1234); } } contract ProtectionModifierAppId is ExecutorStorage, DelegateScriptTarget { - function exec() public returns (bool) { + function exec() public { appId = bytes32(123456); } } diff --git a/test/mocks/MockDyingScriptExecutor.sol b/test/mocks/MockDyingScriptExecutor.sol deleted file mode 100644 index 2f339b9f2..000000000 --- a/test/mocks/MockDyingScriptExecutor.sol +++ /dev/null @@ -1,12 +0,0 @@ -pragma solidity 0.4.18; - -import "../../contracts/evmscript/IEVMScriptExecutor.sol"; - - -contract MockDyingScriptExecutor is IEVMScriptExecutor { - function execScript(bytes script, bytes input, address[] blacklist) external returns (bytes) { - if (input.length > 0) selfdestruct(address(0)); - - return new bytes(1); - } -} From 102a9d0ee62d75d4ab07fc6148a290affe4b880d Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 24 May 2018 23:19:37 -0300 Subject: [PATCH 2/3] Remove DelegateScript and DeployDelegateScript executors --- contracts/common/DelegateProxy.sol | 21 +-- .../evmscript/executors/DelegateScript.sol | 59 --------- .../executors/DeployDelegateScript.sol | 46 ------- .../factory/EVMScriptRegistryFactory.sol | 8 -- docs/aragonOS.md | 17 --- test/TestDelegateProxy.sol | 35 +---- test/evm_script.js | 120 +----------------- test/helpers/evmScript.js | 3 - test/mocks/DelegateScriptWrapper.sol | 10 -- test/mocks/Delegator.sol | 37 ------ 10 files changed, 5 insertions(+), 351 deletions(-) delete mode 100644 contracts/evmscript/executors/DelegateScript.sol delete mode 100644 contracts/evmscript/executors/DeployDelegateScript.sol delete mode 100644 test/mocks/DelegateScriptWrapper.sol delete mode 100644 test/mocks/Delegator.sol diff --git a/contracts/common/DelegateProxy.sol b/contracts/common/DelegateProxy.sol index 0000cbc82..080c9bbfc 100644 --- a/contracts/common/DelegateProxy.sol +++ b/contracts/common/DelegateProxy.sol @@ -13,29 +13,12 @@ contract DelegateProxy is ERCProxy, IsContract { * @param _calldata Calldata for the delegatecall */ function delegatedFwd(address _dst, bytes _calldata) internal { - delegatedFwd(_dst, _calldata, 0); - } - - /** - * @dev Performs a delegatecall and returns whatever the delegatecall returned (entire context execution will return!) - * @param _dst Destination address to perform the delegatecall - * @param _calldata Calldata for the delegatecall - * @param _minReturnSize Minimum size the call needs to return, if less than that it will revert - */ - function delegatedFwd(address _dst, bytes _calldata, uint256 _minReturnSize) internal { require(isContract(_dst)); - uint256 size; - uint256 result; uint256 fwd_gas_limit = FWD_GAS_LIMIT; assembly { - result := delegatecall(sub(gas, fwd_gas_limit), _dst, add(_calldata, 0x20), mload(_calldata), 0, 0) - size := returndatasize - } - - require(size >= _minReturnSize); - - assembly { + let result := delegatecall(sub(gas, fwd_gas_limit), _dst, add(_calldata, 0x20), mload(_calldata), 0, 0) + let size := returndatasize let ptr := mload(0x40) returndatacopy(ptr, 0, size) diff --git a/contracts/evmscript/executors/DelegateScript.sol b/contracts/evmscript/executors/DelegateScript.sol deleted file mode 100644 index 5e22cc05d..000000000 --- a/contracts/evmscript/executors/DelegateScript.sol +++ /dev/null @@ -1,59 +0,0 @@ -pragma solidity 0.4.18; - -import "../ScriptHelpers.sol"; -import "../IEVMScriptExecutor.sol"; -import "../../common/IsContract.sol"; - - -interface DelegateScriptTarget { - function exec() public; -} - - -contract DelegateScript is IEVMScriptExecutor, IsContract { - using ScriptHelpers for *; - - uint256 constant internal SCRIPT_START_LOCATION = 4; - - /** - * @notice Executes script by delegatecall into a contract - * @param _script [ specId (uint32) ][ contract address (20 bytes) ] - * @param _input ABI encoded call to be made to contract (if empty executes default exec() function) - * @param _blacklist If any address is passed, will revert. - * @return Call return data - */ - function execScript(bytes _script, bytes _input, address[] _blacklist) external returns (bytes) { - require(_blacklist.length == 0); // dont have ability to control bans, so fail. - - // Script should be spec id + address (20 bytes) - require(_script.length == SCRIPT_START_LOCATION + 20); - return delegate(_script.addressAt(SCRIPT_START_LOCATION), _input); - } - - /** - * @dev Delegatecall to contract with input data - */ - function delegate(address _addr, bytes memory _input) internal returns (bytes memory output) { - require(isContract(_addr)); - require(_addr.delegatecall(_input.length > 0 ? _input : defaultInput())); - return returnedData(); - } - - function defaultInput() internal pure returns (bytes) { - return DelegateScriptTarget(0).exec.selector.toBytes(); - } - - /** - * @dev copies and returns last's call data - */ - function returnedData() internal pure returns (bytes ret) { - assembly { - let size := returndatasize - ret := mload(0x40) // free mem ptr get - mstore(0x40, add(ret, add(size, 0x20))) // free mem ptr set - mstore(ret, size) // set array length - returndatacopy(add(ret, 0x20), 0, size) // copy return data - } - return ret; - } -} \ No newline at end of file diff --git a/contracts/evmscript/executors/DeployDelegateScript.sol b/contracts/evmscript/executors/DeployDelegateScript.sol deleted file mode 100644 index d2847709a..000000000 --- a/contracts/evmscript/executors/DeployDelegateScript.sol +++ /dev/null @@ -1,46 +0,0 @@ -pragma solidity 0.4.18; - -import "./DelegateScript.sol"; - -// Inspired by: https://github.com/dapphub/ds-proxy/blob/master/src/proxy.sol - - -contract DeployDelegateScript is DelegateScript { - uint256 constant internal SCRIPT_START_LOCATION = 4; - - uint256[2**254] private cacheStorageOffset; - mapping (bytes32 => address) cache; - - /** - * @notice Executes script by delegatecall into a deployed contract (exec() function) - * @param _script [ specId (uint32) ][ contractInitcode (bytecode) ] - * @param _input ABI encoded call to be made to contract (if empty executes default exec() function) - * @param _blacklist If any address is passed, will revert. - * @return Call return data - */ - function execScript(bytes _script, bytes _input, address[] _blacklist) external returns (bytes) { - require(_blacklist.length == 0); // dont have ability to control bans, so fail. - - bytes32 id = keccak256(_script); - address deployed = cache[id]; - if (deployed == address(0)) { - deployed = deploy(_script); - cache[id] = deployed; - } - - return DelegateScript.delegate(deployed, _input); - } - - /** - * @dev Deploys contract byte code to network - */ - function deploy(bytes _script) internal returns (address addr) { - assembly { - // 0x24 = 0x20 (length) + 0x04 (spec id uint32) - // Length of code is 4 bytes less than total script size - addr := create(0, add(_script, 0x24), sub(mload(_script), 0x04)) - switch iszero(extcodesize(addr)) - case 1 { revert(0, 0) } // throw if contract failed to deploy - } - } -} \ No newline at end of file diff --git a/contracts/factory/EVMScriptRegistryFactory.sol b/contracts/factory/EVMScriptRegistryFactory.sol index a0584bb15..f500fb701 100644 --- a/contracts/factory/EVMScriptRegistryFactory.sol +++ b/contracts/factory/EVMScriptRegistryFactory.sol @@ -3,8 +3,6 @@ pragma solidity 0.4.18; import "../evmscript/EVMScriptRegistry.sol"; import "../evmscript/executors/CallsScript.sol"; -import "../evmscript/executors/DelegateScript.sol"; -import "../evmscript/executors/DeployDelegateScript.sol"; import "./AppProxyFactory.sol"; import "../kernel/Kernel.sol"; @@ -14,14 +12,10 @@ import "../acl/ACL.sol"; contract EVMScriptRegistryFactory is AppProxyFactory, EVMScriptRegistryConstants { address public baseReg; address public baseCalls; - address public baseDel; - address public baseDeployDel; function EVMScriptRegistryFactory() public { baseReg = address(new EVMScriptRegistry()); baseCalls = address(new CallsScript()); - baseDel = address(new DelegateScript()); - baseDeployDel = address(new DeployDelegateScript()); } function newEVMScriptRegistry(Kernel _dao, address _root) public returns (EVMScriptRegistry reg) { @@ -33,8 +27,6 @@ contract EVMScriptRegistryFactory is AppProxyFactory, EVMScriptRegistryConstants acl.createPermission(this, reg, reg.REGISTRY_MANAGER_ROLE(), this); reg.addScriptExecutor(baseCalls); // spec 1 = CallsScript - reg.addScriptExecutor(baseDel); // spec 2 = DelegateScript - reg.addScriptExecutor(baseDeployDel); // spec 3 = DeployDelegateScript acl.revokePermission(this, reg, reg.REGISTRY_MANAGER_ROLE()); acl.setPermissionManager(_root, reg, reg.REGISTRY_MANAGER_ROLE()); diff --git a/docs/aragonOS.md b/docs/aragonOS.md index de7b9fedc..c88c3157a 100644 --- a/docs/aragonOS.md +++ b/docs/aragonOS.md @@ -436,23 +436,6 @@ A simple way to concatenate multiple calls. It cancels the operation if any of t - **Output:** None. - **Blacklist:** Entire script reverts if a call to one of the addresses in the blacklist is performed. -##### 5.2.1.1 DelegateScript -`delegatecalls` into a given contract, which basically allows for any arbitrary computation within the EVM in the caller’s context. - -- **Script body:** Address of the contract to make the call to. -- **Input:** `calldata` for the `delegatecall` that will be performed. -- **Output:** raw return data of the call. -- **Blacklist:** impossible to enforce. If there are any addresses in the blacklist the script will revert as it is not possible to check whether a particular address will be called. - -##### 5.2.1.3 DeployDelegateScript - -Is a superset of the DelegateScript, but it takes a contract’s initcode bytecode as its script body instead of just an address. On execution, it deploys the contract to the blockchain and executes it with a `delegatecall`. - -- **Script body:**: initcode for contract being created. -- **Input:** `calldata` for the `delegatecall` that will be performed after contract creation. -- **Output:** raw return data of the call. -- **Blacklist:** impossible to enforce. If there are any addresses in the blacklist the script will revert as it is not possible to check whether a particular address will be called. - ### 5.3 Making an app a Forwarder diff --git a/test/TestDelegateProxy.sol b/test/TestDelegateProxy.sol index 21bf16411..e1bb2c5b8 100644 --- a/test/TestDelegateProxy.sol +++ b/test/TestDelegateProxy.sol @@ -8,7 +8,6 @@ import "../contracts/evmscript/ScriptHelpers.sol"; contract Target { - function returnSomething() public constant returns (bool) { return true; } function dontReturn() public {} function fail() public { revert(); } function die() public { selfdestruct(0); } @@ -39,34 +38,13 @@ contract TestDelegateProxy is DelegateProxy { throwProxy = new ThrowProxy(address(this)); } - function testMinReturn0WithoutReturn() public { - delegatedFwd(target, target.dontReturn.selector.toBytes(), 0); - } - - function testMinReturn0WithReturn() public { - delegatedFwd(target, target.returnSomething.selector.toBytes(), 0); - } - - function testMinReturn32WithReturn() public { - delegatedFwd(target, target.returnSomething.selector.toBytes(), 32); - } - - function testFailsIfReturnLessThanMin() public { - TestDelegateProxy(throwProxy).revertIfReturnLessThanMin(); - throwProxy.assertThrows("should have reverted if return data was less than min"); - } - - function revertIfReturnLessThanMin() public { - delegatedFwd(target, target.dontReturn.selector.toBytes(), 32); - } - function testFailIfNoContract() public { TestDelegateProxy(throwProxy).noContract(); throwProxy.assertThrows("should have reverted if target is not a contract"); } function noContract() public { - delegatedFwd(address(0x1234), target.dontReturn.selector.toBytes(), 0); + delegatedFwd(address(0x1234), target.dontReturn.selector.toBytes()); } function testFailIfReverts() public { @@ -89,17 +67,6 @@ contract TestDelegateProxy is DelegateProxy { Assert.isFalse(result, "should return false"); } - /* TODO: this test doesn't work with ganache. To be restablished when we use geth for tests - function testSelfdestructIsRevertedWithMinReturn() public { - TestDelegateProxy(throwProxy).revertIfReturnLessThanMinAndDie(); - throwProxy.assertThrows("should have reverted to stop selfdestruct"); - } - - function revertIfReturnLessThanMinAndDie() public { - delegatedFwd(target, target.die.selector.toBytes(), 32); - } - */ - // keep as last test as it will kill this contract function testDieIfMinReturn0() public { delegatedFwd(target, target.die.selector.toBytes()); diff --git a/test/evm_script.js b/test/evm_script.js index 5f4b25cb5..431ed0120 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -1,13 +1,10 @@ const { rawEncode } = require('ethereumjs-abi') const { assertRevert } = require('./helpers/assertThrow') -const { encodeCallScript, encodeDelegate, encodeDeploy } = require('./helpers/evmScript') +const { encodeCallScript } = require('./helpers/evmScript') const ExecutionTarget = artifacts.require('ExecutionTarget') const Executor = artifacts.require('Executor') -const Delegator = artifacts.require('Delegator') -const FailingDelegator = artifacts.require('FailingDelegator') -const FailingDeployment = artifacts.require('FailingDeployment') const Kernel = artifacts.require('Kernel') const ACL = artifacts.require('ACL') @@ -55,9 +52,7 @@ contract('EVM Script', accounts => { assert.equal(await reg.getScriptExecutor('0x00000000'), zeroAddr) assert.notEqual(await reg.getScriptExecutor('0x00000001'), zeroAddr) - assert.notEqual(await reg.getScriptExecutor('0x00000002'), zeroAddr) - assert.notEqual(await reg.getScriptExecutor('0x00000003'), zeroAddr) - assert.equal(await reg.getScriptExecutor('0x00000004'), zeroAddr) + assert.equal(await reg.getScriptExecutor('0x00000002'), zeroAddr) }) it('fails if reinitializing registry', async () => { @@ -168,116 +163,5 @@ contract('EVM Script', accounts => { await executor.execute(encodeCallScript([])) }) }) - - const delegatorResultNumber = 1234 - - context('spec ID 2', () => { - let delegator = {} - before(async () => { - delegator = await Delegator.new() - }) - - it('can execute delegated action', async () => { - await executor.executeWithBan(encodeDelegate(delegator.address), []) - - assert.equal(await executor.randomNumber(), delegatorResultNumber, 'should have executed correctly') - }) - - it('can execute action with input and output', async () => { - const value = 101 - const input = delegator.contract.execReturnValue.getData(value) - const output = await executor.executeWithIO.call(encodeDelegate(delegator.address), input, []) - - assert.equal(new web3.BigNumber(output), value, 'return value should be correct') - }) - - it('fails to execute if it has blacklist addresses', async () => { - return assertRevert(async () => { - await executor.executeWithBan(encodeDelegate(delegator.address), ['0x12']) - }) - }) - - it('fails if underlying call fails', async () => { - const failingDelegator = await FailingDelegator.new() - return assertRevert(async () => { - // extra gas to avoid oog - await executor.executeWithBan(encodeDelegate(failingDelegator.address), [], { gas: 2e6 }) - }) - }) - - it('fails if calling to non-contract', async () => { - return assertRevert(async () => { - await executor.execute(encodeDelegate(accounts[0])) // addr is too small - }) - }) - - it('fails if payload is too small', async () => { - return assertRevert(async () => { - await executor.execute(encodeDelegate('0x1234')) // addr is too small - }) - }) - }) - - context('spec ID 3', () => { - it('can deploy and execute', async () => { - await executor.execute(encodeDeploy(Delegator)) - - assert.equal(await executor.randomNumber(), delegatorResultNumber, 'should have executed correctly') - }) - - it('can deploy action with input and output', async () => { - const value = 102 - const delegator = await Delegator.new() - const input = delegator.contract.execReturnValue.getData(value) - const output = await executor.executeWithIO.call(encodeDeploy(Delegator), input, []) - - assert.equal(new web3.BigNumber(output), value, 'return value should be correct') - }) - - it('caches deployed contract and reuses it', async () => { - const r1 = await executor.execute(encodeDeploy(Delegator)) - const r2 = await executor.execute(encodeDeploy(Delegator)) - - assert.isBelow(r2.receipt.gasUsed, r1.receipt.gasUsed / 2, 'should have used less than half the gas because of cache') - assert.equal(await executor.randomNumber(), delegatorResultNumber * 2, 'should have executed correctly twice') - }) - - it('fails if deployment fails', async () => { - return assertRevert(async () => { - await executor.execute(encodeDeploy(FailingDeployment)) - }) - }) - - it('fails if deployed contract doesnt have exec function', async () => { - return assertRevert(async () => { - // random contract without exec() func - await executor.execute(encodeDeploy(ExecutionTarget)) - }) - }) - - it('fails if exec function fails', async () => { - return assertRevert(async () => { - await executor.execute(encodeDeploy(FailingDelegator)) - }) - }) - - it('fails to execute if it has blacklist addresses', async () => { - return assertRevert(async () => { - await executor.executeWithBan(encodeDeploy(Delegator), ['0x1234']) - }) - }) - - it('fails if execution modifies kernel', async () => { - return assertRevert(async () => { - await executor.execute(encodeDeploy(artifacts.require('ProtectionModifierKernel'))) - }) - }) - - it('fails if execution modifies app id', async () => { - return assertRevert(async () => { - await executor.execute(encodeDeploy(artifacts.require('ProtectionModifierAppId'))) - }) - }) - }) }) }) diff --git a/test/helpers/evmScript.js b/test/helpers/evmScript.js index f8492252d..999718519 100644 --- a/test/helpers/evmScript.js +++ b/test/helpers/evmScript.js @@ -13,7 +13,4 @@ module.exports = { return script + addr.slice(24) + length.slice(56) + calldata.slice(2) }, '0x00000001') // spec 1 }, - - encodeDelegate: addr => '0x00000002' + addr.slice(2), // remove 0x from addr - encodeDeploy: contract => '0x00000003' + contract.binary.slice(2), } diff --git a/test/mocks/DelegateScriptWrapper.sol b/test/mocks/DelegateScriptWrapper.sol deleted file mode 100644 index 07a7d4e3d..000000000 --- a/test/mocks/DelegateScriptWrapper.sol +++ /dev/null @@ -1,10 +0,0 @@ -pragma solidity 0.4.18; - -import "../../contracts/evmscript/executors/DelegateScript.sol"; - - -contract DelegateScriptWrapper is DelegateScript { - function isContractWrapper(address _target) public constant returns (bool) { - return isContract(_target); - } -} diff --git a/test/mocks/Delegator.sol b/test/mocks/Delegator.sol deleted file mode 100644 index 469436215..000000000 --- a/test/mocks/Delegator.sol +++ /dev/null @@ -1,37 +0,0 @@ -pragma solidity 0.4.18; - -import "../../contracts/evmscript/executors/DelegateScript.sol"; -import "./Executor.sol"; - - -contract Delegator is ExecutorStorage, DelegateScriptTarget { - function exec() public { - randomNumber += 1234; - } - - function execReturnValue(uint i) public constant returns (uint) { return i; } -} - - -contract FailingDelegator is DelegateScriptTarget { - function exec() public { revert(); } -} - - -contract FailingDeployment { - function FailingDeployment() public { revert(); } -} - - -contract ProtectionModifierKernel is ExecutorStorage, DelegateScriptTarget { - function exec() public { - kernel = IKernel(0x1234); - } -} - - -contract ProtectionModifierAppId is ExecutorStorage, DelegateScriptTarget { - function exec() public { - appId = bytes32(123456); - } -} From 505021df374e01aa68227798c04ca7ab0ed97d2e Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 25 May 2018 16:10:07 -0300 Subject: [PATCH 3/3] Address PR comments --- test/evm_script.js | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/test/evm_script.js b/test/evm_script.js index 431ed0120..fbd8605b8 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -47,7 +47,7 @@ contract('EVM Script', accounts => { await dao.setApp(APP_BASE_NAMESPACE, executorAppId, baseExecutor.address, { from: boss }) }) - it('registered just 3 script executors', async () => { + it('factory registered just 1 script executor', async () => { const zeroAddr = '0x0000000000000000000000000000000000000000' assert.equal(await reg.getScriptExecutor('0x00000000'), zeroAddr) @@ -162,6 +162,29 @@ contract('EVM Script', accounts => { it('can execute empty script', async () => { await executor.execute(encodeCallScript([])) }) + + context('script overflow', async () => { + const encodeCallScriptBad = actions => { + return actions.reduce((script, { to, calldata }) => { + const addr = rawEncode(['address'], [to]).toString('hex') + // length should be (calldata.length - 2) / 2 instead of calldata.length + // as this one is bigger, it would overflow and therefore must revert + const length = rawEncode(['uint256'], [calldata.length]).toString('hex') + + // Remove 12 first 0s of padding for addr and 28 0s for uint32 + return script + addr.slice(24) + length.slice(56) + calldata.slice(2) + }, '0x00000001') // spec 1 + } + + it('fails if data length is too big', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptBad([action]) + + return assertRevert(async () => { + await executor.execute(script) + }) + }) + }) }) }) })