From 446653d01a7b3f2344a32da244ef432edd38d0dc Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 5 Apr 2019 13:37:56 +0200 Subject: [PATCH] EVMScripts: check byte lengths, avoid returning unallocated memory, and forward error data (#496) --- contracts/evmscript/EVMScriptRegistry.sol | 4 + contracts/evmscript/EVMScriptRunner.sol | 54 +- contracts/evmscript/executors/CallsScript.sol | 48 +- contracts/test/mocks/AppStubScriptRunner.sol | 57 ++ .../test/mocks/EVMScriptExecutorMock.sol | 7 +- .../mocks/EVMScriptExecutorRevertMock.sol | 17 + contracts/test/mocks/ExecutionTarget.sol | 9 +- .../test/mocks/MockScriptExecutorApp.sol | 33 - test/evm_script.js | 716 +++++++++++------- test/evm_script_factory.js | 125 +++ test/helpers/assertThrow.js | 48 +- test/helpers/evmScript.js | 15 +- test/helpers/revertStrings.js | 74 ++ 13 files changed, 845 insertions(+), 362 deletions(-) create mode 100644 contracts/test/mocks/AppStubScriptRunner.sol create mode 100644 contracts/test/mocks/EVMScriptExecutorRevertMock.sol delete mode 100644 contracts/test/mocks/MockScriptExecutorApp.sol create mode 100644 test/evm_script_factory.js create mode 100644 test/helpers/revertStrings.js diff --git a/contracts/evmscript/EVMScriptRegistry.sol b/contracts/evmscript/EVMScriptRegistry.sol index 4f7f308fe..7eb8dd3d6 100644 --- a/contracts/evmscript/EVMScriptRegistry.sol +++ b/contracts/evmscript/EVMScriptRegistry.sol @@ -19,9 +19,12 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar // WARN: Manager can censor all votes and the like happening in an org bytes32 public constant REGISTRY_MANAGER_ROLE = 0xf7a450ef335e1892cb42c8ca72e7242359d7711924b75db5717410da3f614aa3; + uint256 internal constant SCRIPT_START_LOCATION = 4; + string private constant ERROR_INEXISTENT_EXECUTOR = "EVMREG_INEXISTENT_EXECUTOR"; string private constant ERROR_EXECUTOR_ENABLED = "EVMREG_EXECUTOR_ENABLED"; string private constant ERROR_EXECUTOR_DISABLED = "EVMREG_EXECUTOR_DISABLED"; + string private constant ERROR_SCRIPT_LENGTH_TOO_SHORT = "EVMREG_SCRIPT_LENGTH_TOO_SHORT"; struct ExecutorEntry { IEVMScriptExecutor executor; @@ -96,6 +99,7 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar * @param _script EVMScript being inspected */ function getScriptExecutor(bytes _script) public view returns (IEVMScriptExecutor) { + require(_script.length >= SCRIPT_START_LOCATION, ERROR_SCRIPT_LENGTH_TOO_SHORT); uint256 id = _script.getSpecId(); // Note that we don't need to check for an executor's existence in this case, as only diff --git a/contracts/evmscript/EVMScriptRunner.sol b/contracts/evmscript/EVMScriptRunner.sol index 44c20459f..28970632c 100644 --- a/contracts/evmscript/EVMScriptRunner.sol +++ b/contracts/evmscript/EVMScriptRunner.sol @@ -14,7 +14,6 @@ import "../common/Initializable.sol"; contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstants, KernelNamespaceConstants { string private constant ERROR_EXECUTOR_UNAVAILABLE = "EVMRUN_EXECUTOR_UNAVAILABLE"; - string private constant ERROR_EXECUTION_REVERTED = "EVMRUN_EXECUTION_REVERTED"; string private constant ERROR_PROTECTED_STATE_MODIFIED = "EVMRUN_PROTECTED_STATE_MODIFIED"; event ScriptResult(address indexed executor, bytes script, bytes input, bytes returnData); @@ -34,36 +33,51 @@ contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstant protectState returns (bytes) { - // TODO: Too much data flying around, maybe extracting spec id here is cheaper IEVMScriptExecutor executor = getEVMScriptExecutor(_script); require(address(executor) != address(0), ERROR_EXECUTOR_UNAVAILABLE); bytes4 sig = executor.execScript.selector; bytes memory data = abi.encodeWithSelector(sig, _script, _input, _blacklist); - require(address(executor).delegatecall(data), ERROR_EXECUTION_REVERTED); - bytes memory output = returnedDataDecoded(); - - emit ScriptResult(address(executor), _script, _input, output); + bytes memory output; + assembly { + let success := delegatecall( + gas, // forward all gas + executor, // address + add(data, 0x20), // calldata start + mload(data), // calldata length + 0, // don't write output (we'll handle this ourselves) + 0 // don't write output + ) - return output; - } + output := mload(0x40) // free mem ptr get - /** - * @dev copies and returns last's call data. Needs to ABI decode first - */ - function returnedDataDecoded() internal pure returns (bytes ret) { - assembly { - let size := returndatasize - switch size - case 0 {} + switch success + case 0 { + // If the call errored, forward its full error data + returndatacopy(output, 0, returndatasize) + revert(output, returndatasize) + } default { - ret := mload(0x40) // free mem ptr get - mstore(0x40, add(ret, add(size, 0x20))) // free mem ptr set - returndatacopy(ret, 0x20, sub(size, 0x20)) // copy return data + // Copy result + // + // Needs to perform an ABI decode for the expected `bytes` return type of + // `executor.execScript()` as solidity will automatically ABI encode the returned bytes as: + // [ position of the first dynamic length return value = 0x20 (32 bytes) ] + // [ output length (32 bytes) ] + // [ output content (N bytes) ] + // + // Perform the ABI decode by ignoring the first 32 bytes of the return data + let copysize := sub(returndatasize, 0x20) + returndatacopy(output, 0x20, copysize) + + mstore(0x40, add(output, copysize)) // free mem ptr set } } - return ret; + + emit ScriptResult(address(executor), _script, _input, output); + + return output; } modifier protectState { diff --git a/contracts/evmscript/executors/CallsScript.sol b/contracts/evmscript/executors/CallsScript.sol index e46ebf5bc..7345ae31a 100644 --- a/contracts/evmscript/executors/CallsScript.sol +++ b/contracts/evmscript/executors/CallsScript.sol @@ -16,7 +16,10 @@ contract CallsScript is BaseEVMScriptExecutor { string private constant ERROR_BLACKLISTED_CALL = "EVMCALLS_BLACKLISTED_CALL"; string private constant ERROR_INVALID_LENGTH = "EVMCALLS_INVALID_LENGTH"; + + /* This is manually crafted in assembly string private constant ERROR_CALL_REVERTED = "EVMCALLS_CALL_REVERTED"; + */ event LogScriptCall(address indexed sender, address indexed src, address indexed dst); @@ -25,14 +28,17 @@ contract CallsScript is BaseEVMScriptExecutor { * @param _script [ specId (uint32) ] many calls with this structure -> * [ to (address: 20 bytes) ] [ calldataLength (uint32: 4 bytes) ] [ calldata (calldataLength bytes) ] * @param _blacklist Addresses the script cannot call to, or will revert. - * @return always returns empty byte array + * @return Always returns empty byte array */ function execScript(bytes _script, bytes, address[] _blacklist) external isInitialized returns (bytes) { uint256 location = SCRIPT_START_LOCATION; // first 32 bits are spec id while (location < _script.length) { + // Check there's at least address + calldataLength available + require(_script.length - location >= 0x18, ERROR_INVALID_LENGTH); + address contractAddress = _script.addressAt(location); // Check address being called is not blacklist - for (uint i = 0; i < _blacklist.length; i++) { + for (uint256 i = 0; i < _blacklist.length; i++) { require(contractAddress != _blacklist[i], ERROR_BLACKLISTED_CALL); } @@ -50,11 +56,43 @@ contract CallsScript is BaseEVMScriptExecutor { bool success; assembly { - success := call(sub(gas, 5000), contractAddress, 0, calldataStart, calldataLength, 0, 0) - } + success := call( + sub(gas, 5000), // forward gas left - 5000 + contractAddress, // address + 0, // no value + calldataStart, // calldata start + calldataLength, // calldata length + 0, // don't write output + 0 // don't write output + ) - require(success, ERROR_CALL_REVERTED); + switch success + case 0 { + let ptr := mload(0x40) + + switch returndatasize + case 0 { + // No error data was returned, revert with "EVMCALLS_CALL_REVERTED" + // See remix: doing a `revert("EVMCALLS_CALL_REVERTED")` always results in + // this memory layout + mstore(ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000) // error identifier + mstore(add(ptr, 0x04), 0x0000000000000000000000000000000000000000000000000000000000000020) // starting offset + mstore(add(ptr, 0x24), 0x0000000000000000000000000000000000000000000000000000000000000016) // reason length + mstore(add(ptr, 0x44), 0x45564d43414c4c535f43414c4c5f524556455254454400000000000000000000) // reason + + revert(ptr, 100) // 100 = 4 + 3 * 32 (error identifier + 3 words for the ABI encoded error) + } + default { + // Forward the full error data + returndatacopy(ptr, 0, returndatasize) + revert(ptr, returndatasize) + } + } + default { } + } } + // No need to allocate empty bytes for the return as this can only be called via an delegatecall + // (due to the isInitialized modifier) } function executorType() external pure returns (bytes32) { diff --git a/contracts/test/mocks/AppStubScriptRunner.sol b/contracts/test/mocks/AppStubScriptRunner.sol new file mode 100644 index 000000000..6fcf54aa6 --- /dev/null +++ b/contracts/test/mocks/AppStubScriptRunner.sol @@ -0,0 +1,57 @@ +pragma solidity 0.4.24; + +import "../../apps/AragonApp.sol"; + + +contract AppStubScriptRunner is AragonApp { + event ReturnedBytes(bytes returnedBytes); + + // Initialization is required to access any of the real executors + function initialize() public { + initialized(); + } + + function runScript(bytes script) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), new address[](0)); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithBan(bytes script, address[] memory blacklist) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), blacklist); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) { + bytes memory returnedBytes = runScript(script, input, blacklist); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithNewBytesAllocation(bytes script) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), new address[](0)); + bytes memory newBytes = new bytes(64); + + // Fill in new bytes array with some dummy data to let us check it doesn't corrupt the + // script's returned bytes + uint256 first = uint256(keccak256("test")); + uint256 second = uint256(keccak256("mock")); + assembly { + mstore(add(newBytes, 0x20), first) + mstore(add(newBytes, 0x40), second) + } + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + /* + function getActionsCount(bytes script) public constant returns (uint256) { + return getScriptActionsCount(script); + } + + function getAction(bytes script, uint256 i) public constant returns (address, bytes) { + return getScriptAction(script, i); + } + */ +} diff --git a/contracts/test/mocks/EVMScriptExecutorMock.sol b/contracts/test/mocks/EVMScriptExecutorMock.sol index e96713e42..3db26fe82 100644 --- a/contracts/test/mocks/EVMScriptExecutorMock.sol +++ b/contracts/test/mocks/EVMScriptExecutorMock.sol @@ -6,7 +6,12 @@ import "../../evmscript/executors/BaseEVMScriptExecutor.sol"; contract EVMScriptExecutorMock is BaseEVMScriptExecutor { bytes32 internal constant EXECUTOR_TYPE = keccak256("MOCK_SCRIPT"); - function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) { + function execScript(bytes _script, bytes, address[]) external isInitialized returns (bytes) { + // Return full input script if it's more than just the spec ID, otherwise return an empty + // bytes array + if (_script.length > SCRIPT_START_LOCATION) { + return _script; + } } function executorType() external pure returns (bytes32) { diff --git a/contracts/test/mocks/EVMScriptExecutorRevertMock.sol b/contracts/test/mocks/EVMScriptExecutorRevertMock.sol new file mode 100644 index 000000000..9ee5dd5b7 --- /dev/null +++ b/contracts/test/mocks/EVMScriptExecutorRevertMock.sol @@ -0,0 +1,17 @@ +pragma solidity 0.4.24; + + +import "../../evmscript/executors/BaseEVMScriptExecutor.sol"; + +contract EVMScriptExecutorRevertMock is BaseEVMScriptExecutor { + string public constant ERROR_MOCK_REVERT = "MOCK_REVERT"; + bytes32 internal constant EXECUTOR_TYPE = keccak256("MOCK_REVERT_SCRIPT"); + + function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) { + revert(ERROR_MOCK_REVERT); + } + + function executorType() external pure returns (bytes32) { + return EXECUTOR_TYPE; + } +} diff --git a/contracts/test/mocks/ExecutionTarget.sol b/contracts/test/mocks/ExecutionTarget.sol index c37930c17..55c41a113 100644 --- a/contracts/test/mocks/ExecutionTarget.sol +++ b/contracts/test/mocks/ExecutionTarget.sol @@ -2,6 +2,7 @@ pragma solidity 0.4.24; contract ExecutionTarget { + string public constant ERROR_EXECUTION_TARGET = "EXECUTION_TARGET_REVERTED"; uint public counter; function execute() public { @@ -9,8 +10,12 @@ contract ExecutionTarget { emit Executed(counter); } - function failExecute() public pure { - revert(); + function failExecute(bool errorWithData) public pure { + if (errorWithData) { + revert(ERROR_EXECUTION_TARGET); + } else { + revert(); + } } function setCounter(uint x) public { diff --git a/contracts/test/mocks/MockScriptExecutorApp.sol b/contracts/test/mocks/MockScriptExecutorApp.sol deleted file mode 100644 index 291272390..000000000 --- a/contracts/test/mocks/MockScriptExecutorApp.sol +++ /dev/null @@ -1,33 +0,0 @@ -pragma solidity 0.4.24; - -import "../../apps/AragonApp.sol"; - - -contract MockScriptExecutorApp is AragonApp { - // Initialization is required to access any of the real executors - function initialize() public { - initialized(); - } - - function execute(bytes script) public { - runScript(script, new bytes(0), new address[](0)); - } - - function executeWithBan(bytes script, address[] memory blacklist) public { - runScript(script, new bytes(0), blacklist); - } - - function executeWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) { - return runScript(script, input, blacklist); - } - - /* - function getActionsCount(bytes script) public constant returns (uint256) { - return getScriptActionsCount(script); - } - - function getAction(bytes script, uint256 i) public constant returns (address, bytes) { - return getScriptAction(script, i); - } - */ -} diff --git a/test/evm_script.js b/test/evm_script.js index 48ad317f1..b88cb0f2f 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -3,362 +3,522 @@ const { soliditySha3 } = require('web3-utils') const assertEvent = require('./helpers/assertEvent') const { assertRevert } = require('./helpers/assertThrow') -const { encodeCallScript } = require('./helpers/evmScript') +const { createExecutorId, encodeCallScript } = require('./helpers/evmScript') +const reverts = require('./helpers/revertStrings') const Kernel = artifacts.require('Kernel') +const KernelProxy = artifacts.require('KernelProxy') const ACL = artifacts.require('ACL') -const DAOFactory = artifacts.require('DAOFactory') -const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') const EVMScriptRegistry = artifacts.require('EVMScriptRegistry') const CallsScript = artifacts.require('CallsScript') const IEVMScriptExecutor = artifacts.require('IEVMScriptExecutor') // Mocks +const AppStubScriptRunner = artifacts.require('AppStubScriptRunner') const ExecutionTarget = artifacts.require('ExecutionTarget') const EVMScriptExecutorMock = artifacts.require('EVMScriptExecutorMock') -const MockScriptExecutorApp = artifacts.require('MockScriptExecutorApp') +const EVMScriptExecutorRevertMock = artifacts.require('EVMScriptExecutorRevertMock') +const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock') -const EMPTY_BYTES = '0x' const ZERO_ADDR = '0x0000000000000000000000000000000000000000' +const EMPTY_BYTES = '0x' contract('EVM Script', accounts => { - let callsScriptBase, executorAppBase, executorApp, executionTarget, dao, daoFact, reg, acl - let APP_BASES_NAMESPACE + let kernelBase, aclBase, evmScriptRegBase, dao, acl, evmScriptReg, scriptExecutorMock, scriptExecutorRevertMock + let APP_BASES_NAMESPACE, APP_ADDR_NAMESPACE, APP_MANAGER_ROLE + let EVMSCRIPT_REGISTRY_APP_ID, REGISTRY_ADD_EXECUTOR_ROLE, REGISTRY_MANAGER_ROLE + let ERROR_MOCK_REVERT, ERROR_EXECUTION_TARGET + + const boss = accounts[1] + + const scriptRunnerAppId = '0x1234' + + before(async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + evmScriptRegBase = await EVMScriptRegistry.new() + scriptExecutorMock = await EVMScriptExecutorMock.new() + scriptExecutorRevertMock = await EVMScriptExecutorRevertMock.new() + const evmScriptRegConstants = await EVMScriptRegistryConstantsMock.new() + const executionTarget = await ExecutionTarget.new() + + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + APP_ADDR_NAMESPACE = await kernelBase.APP_ADDR_NAMESPACE() + APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + + EVMSCRIPT_REGISTRY_APP_ID = await evmScriptRegConstants.getEVMScriptRegistryAppId() + REGISTRY_ADD_EXECUTOR_ROLE = await evmScriptRegBase.REGISTRY_ADD_EXECUTOR_ROLE() + REGISTRY_MANAGER_ROLE = await evmScriptRegBase.REGISTRY_MANAGER_ROLE() + + ERROR_MOCK_REVERT = await scriptExecutorRevertMock.ERROR_MOCK_REVERT() + ERROR_EXECUTION_TARGET = await executionTarget.ERROR_EXECUTION_TARGET() + }) + + beforeEach(async () => { + dao = Kernel.at((await KernelProxy.new(kernelBase.address)).address) + await dao.initialize(aclBase.address, boss) + acl = ACL.at(await dao.acl()) + + // Set up app management permissions + await acl.createPermission(boss, dao.address, APP_MANAGER_ROLE, boss, { from: boss }) + + // Set up script registry (MUST use correct app ID and set as default app) + const initPayload = evmScriptRegBase.contract.initialize.getData() + const evmScriptRegReceipt = await dao.newAppInstance(EVMSCRIPT_REGISTRY_APP_ID, evmScriptRegBase.address, initPayload, true, { from: boss }) + evmScriptReg = EVMScriptRegistry.at(evmScriptRegReceipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + }) + + context('> Registry', () => { + beforeEach(async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + }) + + it('is initialized', async () => { + assert.isTrue(await evmScriptReg.hasInitialized(), "should be initialized") + }) - const boss = accounts[1] + it('fails if reinitializing registry', async () => { + await assertRevert(evmScriptReg.initialize(), reverts.INIT_ALREADY_INITIALIZED) + }) - const executorAppId = '0x1234' + it('fails to get an executor if not enough bytes are given', async () => { + // Not enough bytes are given (need 4, given 3) + await assertRevert(evmScriptReg.getScriptExecutor('0x'.padEnd(6, 0)), reverts.EVMREG_SCRIPT_LENGTH_TOO_SHORT) + }) - const createExecutorId = id => `0x${String(id).padStart(8, '0')}` + it('can add a new executor', async () => { + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) - before(async () => { - const regFact = await EVMScriptRegistryFactory.new() - callsScriptBase = CallsScript.at(await regFact.baseCallScript()) + const newExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + const [executorAddress, executorEnabled] = await evmScriptReg.executors(newExecutorId) + const newExecutor = IEVMScriptExecutor.at(executorAddress) + + assert.equal(await scriptExecutorMock.executorType(), await newExecutor.executorType(), "executor type should be the same") + assert.equal(await scriptExecutorMock.address, await executorAddress, "executor address should be the same") + assert.isTrue(executorEnabled, "new executor should be enabled") + }) + + it('fails to add a new executor without the correct permissions', async () => { + await assertRevert(evmScriptReg.addScriptExecutor(scriptExecutorMock.address), reverts.APP_AUTH_FAILED) + }) + + context('> Existing executor', () => { + let installedExecutorId + + beforeEach(async () => { + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + installedExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + }) + + it('can disable an executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + let executorEntry = await evmScriptReg.executors(installedExecutorId) + assert.isTrue(executorEntry[1], "executor should be enabled") + + const receipt = await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + executorEntry = await evmScriptReg.executors(installedExecutorId) - const kernelBase = await Kernel.new(true) // petrify immediately - const aclBase = await ACL.new() - daoFact = await DAOFactory.new(kernelBase.address, aclBase.address, regFact.address) + assertEvent(receipt, 'DisableExecutor') + assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') + }) - APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + it('can re-enable an executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + let executorEntry = await evmScriptReg.executors(installedExecutorId) + assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') + + const receipt = await evmScriptReg.enableScriptExecutor(installedExecutorId, { from: boss }) + executorEntry = await evmScriptReg.executors(installedExecutorId) + + assertEvent(receipt, 'EnableExecutor') + assert.isTrue(executorEntry[1], "executor should now be re-enabled") + assert.notEqual(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return non-zero addr') + }) + + it('fails to disable an executor without the correct permissions', async () => { + await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId), reverts.APP_AUTH_FAILED) + }) + + it('fails to enable an executor without the correct permissions', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId), reverts.APP_AUTH_FAILED) + }) + + it('fails to enable an already enabled executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_ENABLED) + }) + + it('fails to enable a non-existent executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId + 1, { from: boss }), reverts.EVMREG_INEXISTENT_EXECUTOR) + }) + + it('fails to disable an already disabled executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + + await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_DISABLED) + }) + }) + }) + + context('> ScriptRunner', () => { + let scriptRunnerApp + + before(async () => { + scriptRunnerAppBase = await AppStubScriptRunner.new() }) beforeEach(async () => { - const receipt = await daoFact.newDAO(boss) - dao = Kernel.at(receipt.logs.filter(l => l.event == 'DeployDAO')[0].args.dao) - acl = ACL.at(await dao.acl()) - reg = EVMScriptRegistry.at(receipt.logs.filter(l => l.event == 'DeployEVMScriptRegistry')[0].args.reg) - - await acl.createPermission(boss, dao.address, await dao.APP_MANAGER_ROLE(), boss, { from: boss }) - executorAppBase = await MockScriptExecutorApp.new() - await dao.setApp(APP_BASES_NAMESPACE, executorAppId, executorAppBase.address, { from: boss }) + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false, { from: boss }) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await scriptRunnerApp.initialize() }) - it('factory registered just 1 script executor', async () => { - assert.equal(await reg.getScriptExecutor(createExecutorId(0)), ZERO_ADDR) - assert.notEqual(await reg.getScriptExecutor(createExecutorId(1)), ZERO_ADDR) - assert.equal(await reg.getScriptExecutor(createExecutorId(2)), ZERO_ADDR) + it('gets the correct executor registry from the app', async () => { + const registryFromApp = await scriptRunnerApp.getEVMScriptRegistry() + assert.equal(evmScriptReg.address, registryFromApp, 'app should return the same EVMScriptRegistry') }) - it('fails if directly calling base executor', async () => { - const executionTarget = await ExecutionTarget.new() - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('fails to execute if spec ID is 0', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(0)), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) + }) - await assertRevert(() => callsScriptBase.execScript(script, EMPTY_BYTES, [])) + it('fails to execute if spec ID is unknown', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(4)), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) }) - context('> Registry', () => { - it('is initialized', async () => { - assert.isTrue(await reg.hasInitialized(), "should be initialized") - }) + context('> Uninitialized', () => { + beforeEach(async () => { + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + // Install new script runner app + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false, { from: boss }) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + // Explicitly don't initialize the scriptRunnerApp + }) + + it('fails to execute any executor', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(1)), reverts.INIT_NOT_INITIALIZED) + }) + }) - it('fails if reinitializing registry', async () => { - await assertRevert(async () => { - await reg.initialize() - }) - }) + context('> Registry actions', () => { + let executorSpecId - context('> New executor', () => { - let executorMock, newExecutorId - - before(async () => { - executorMock = await EVMScriptExecutorMock.new() - }) - - beforeEach(async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_ADD_EXECUTOR_ROLE(), boss, { from: boss }) - const receipt = await reg.addScriptExecutor(executorMock.address, { from: boss }) - - newExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId - }) - - it('can add a new executor', async () => { - const executorEntry = await reg.executors(newExecutorId) - const newExecutor = IEVMScriptExecutor.at(executorEntry[0]) - - assert.equal(await executorMock.executorType(), await newExecutor.executorType(), "executor type should be the same") - // Enabled flag is second in struct - assert.isTrue(executorEntry[1], "new executor should be enabled") - }) - - it('can disable an executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - - let executorEntry = await reg.executors(newExecutorId) - assert.isTrue(executorEntry[1], "executor should be enabled") - - const receipt = await reg.disableScriptExecutor(newExecutorId, { from: boss }) - executorEntry = await reg.executors(newExecutorId) - - assertEvent(receipt, 'DisableExecutor') - assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') - }) - - it('can re-enable an executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - let executorEntry = await reg.executors(newExecutorId) - assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') - - const receipt = await reg.enableScriptExecutor(newExecutorId, { from: boss }) - executorEntry = await reg.executors(newExecutorId) - - assertEvent(receipt, 'EnableExecutor') - assert.isTrue(executorEntry[1], "executor should now be re-enabled") - assert.notEqual(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return non-zero addr') - }) - - it('fails to add a new executor without the correct permissions', async () => { - await assertRevert(async () => { - await reg.addScriptExecutor(executorMock.address) - }) - }) - - it('fails to disable an executor without the correct permissions', async () => { - await assertRevert(async () => { - await reg.disableScriptExecutor(newExecutorId) - }) - }) - - it('fails to enable an executor without the correct permissions', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId) - }) - }) - - it('fails to enable an already enabled executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId, { from: boss }) - }) - }) - - it('fails to enable a non-existent executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId + 1, { from: boss }) - }) - }) - - it('fails to disable an already disabled executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - - await assertRevert(async () => { - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - }) - }) - }) + beforeEach(async () => { + // Let boss enable and disable executors + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + }) + + it("can't execute disabled spec ID", async () => { + await evmScriptReg.disableScriptExecutor(executorSpecId, { from: boss }) + + await assertRevert(scriptRunnerApp.runScript(encodeCallScript([])), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) + }) + + it('can execute once spec ID is re-enabled', async () => { + // Disable then re-enable the executor + await evmScriptReg.disableScriptExecutor(executorSpecId, { from: boss }) + await evmScriptReg.enableScriptExecutor(executorSpecId, { from: boss }) + + await scriptRunnerApp.runScript(encodeCallScript([])) + }) }) - context('> Uninitialized executor', () => { - beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) - executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) - // Explicitly don't initialize the executorApp - executionTarget = await ExecutionTarget.new() + context('> Returned bytes', () => { + beforeEach(async () => { + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + // Sanity check it's at spec ID 1 + const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + }) + + it('properly returns if no bytes are returned from executor', async () => { + // Only executes executor with bytes for spec ID + const inputScript = createExecutorId(1) + const receipt = await scriptRunnerApp.runScript(inputScript) + + // Check logs + // The executor app always uses 0x for the input and the mock script executor should return + // an empty bytes array if only the spec ID is given + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.script, inputScript, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the correct return data') + }) + + for (inputBytes of [ + soliditySha3('test').slice(2, 10), // bytes4 + soliditySha3('test').slice(2), // bytes32 + `${soliditySha3('test').slice(2)}${soliditySha3('test2').slice(2, 10)}`, // bytes36 + `${soliditySha3('test').slice(2)}${soliditySha3('test2').slice(2)}`, // bytes64 + ]) { + it(`properly returns bytes (length: ${inputBytes.length / 2}) from executor`, async () => { + const inputScript = `${createExecutorId(1)}${inputBytes}` + const receipt = await scriptRunnerApp.runScript(inputScript) + + // Check logs + // The executor app always uses 0x for the input and the mock script executor should return + // the full input script since it's more than just the spec ID + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.script, inputScript, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, inputScript, 'should log the correct return data') }) + } + + it('properly allocates the free memory pointer after no bytes were returned from executor', async () => { + const inputScript = createExecutorId(1) + const receipt = await scriptRunnerApp.runScriptWithNewBytesAllocation(inputScript) + + // Check logs for returned bytes + const returnedBytes = receipt.logs.filter(l => l.event == 'ReturnedBytes')[0] + assert.equal(returnedBytes.args.returnedBytes, EMPTY_BYTES, 'should log the correct return data') + }) + + it('properly allocates the free memory pointer after returning bytes from executor', async () => { + const inputBytes = `${soliditySha3('test').slice(2)}${soliditySha3('mock').slice(2)}` + // Adjust to completely fill a 32-byte words (64 in this case: 4 + 32 + 32 - 4) + const inputScript = `${createExecutorId(1)}${inputBytes}`.slice(0, -8) + const receipt = await scriptRunnerApp.runScriptWithNewBytesAllocation(inputScript) + + // Check logs for returned bytes + const returnedBytes = receipt.logs.filter(l => l.event == 'ReturnedBytes')[0] + assert.equal(returnedBytes.args.returnedBytes, inputScript, 'should log the correct return data') + }) + }) - it('fails to execute any executor', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + context('> Reverted script', () => { + beforeEach(async () => { + // Install mock reverting executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorRevertMock.address, { from: boss }) - await assertRevert(() => executorApp.execute(script)) - }) + // Sanity check it's at spec ID 1 + const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + }) + + it('forwards the revert data correctly', async () => { + // Only executes executor with bytes for spec ID + const inputScript = createExecutorId(1) + + // Should revert and forward the script executor's revert message + assertRevert(scriptRunnerApp.runScript(inputScript), ERROR_MOCK_REVERT) + }) }) - context('> Executor', () => { - beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) - executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) - await executorApp.initialize() - executionTarget = await ExecutionTarget.new() - }) + context('> CallsScript', () => { + let callsScriptBase, executionTarget - it('gets the correct executor registry from the app', async () => { - const appRegistry = await executorApp.getEVMScriptRegistry() - assert.equal(reg.address, appRegistry, 'app should return the same evm script registry') - }) + beforeEach(async () => { + callsScriptBase = await CallsScript.new() - it('fails to execute if spec ID is 0', async () => { - return assertRevert(async () => { - await executorApp.execute(createExecutorId(0)) - }) - }) + // Install CallsScript onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(callsScriptBase.address, { from: boss }) - it('fails to execute if spec ID is unknown', async () => { - return assertRevert(async () => { - await executorApp.execute(createExecutorId(4)) - }) - }) + // Sanity check it's at spec ID 1 + const callsScriptExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(callsScriptExecutorId, 1, 'CallsScript should be installed as spec ID 1') - context('> Spec ID 1', () => { - it('is the correct executor type', async () => { - const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const executor = IEVMScriptExecutor.at(await reg.getScriptExecutor(createExecutorId(1))) - assert.equal(await executor.executorType(), CALLS_SCRIPT_TYPE) - }) + executionTarget = await ExecutionTarget.new() + }) - it('gets the correct executor from the app', async () => { - const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const scriptId = createExecutorId(1) - const executor = await reg.getScriptExecutor(scriptId) + it('fails if directly calling calls script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) - const appExecutor = await executorApp.getEVMScriptExecutor(scriptId) - assert.equal(executor, appExecutor, 'app should return the same evm script executor') - }) + await assertRevert(callsScriptBase.execScript(script, EMPTY_BYTES, []), reverts.INIT_NOT_INITIALIZED) + }) - it('executes single action script', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('is the correct executor type', async () => { + const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') + const executor = IEVMScriptExecutor.at(await evmScriptReg.getScriptExecutor(createExecutorId(1))) + assert.equal(await executor.executorType(), CALLS_SCRIPT_TYPE) + }) - const receipt = await executorApp.execute(script) + it('gets the correct executor from the app', async () => { + const script = createExecutorId(1) + const executor = await evmScriptReg.getScriptExecutor(script) - assert.equal(await executionTarget.counter(), 1, 'should have executed action') + const scriptExecutor = await scriptRunnerApp.getEVMScriptExecutor(script) + assert.equal(executor, scriptExecutor, 'app should return the same evm script executor') + }) - // Check logs - // The Executor always uses 0x for the input and callscripts always have 0x returns - const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] - assert.equal(scriptResult.args.executor, await reg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') - assert.equal(scriptResult.args.script, script, 'should log the same script') - assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') - assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the return data') - }) + it('executes single action script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) + + const receipt = await scriptRunnerApp.runScript(script) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + + // Check logs + // The executor app always uses 0x for the input and the calls script always returns 0x output + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.executor, await evmScriptReg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') + assert.equal(scriptResult.args.script, script, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the empty return data') + }) + + it("can execute if call doesn't contain blacklist addresses", async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) + + await scriptRunnerApp.runScriptWithBan(script, ['0x1234']) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + }) - it('fails to execute if has blacklist addresses being called', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('executes multi action script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action, action, action, action]) - return assertRevert(async () => { - await executorApp.executeWithBan(script, [executionTarget.address]) - }) - }) + await scriptRunnerApp.runScript(script) - it("can execute if call doesn't contain blacklist addresses", async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + assert.equal(await executionTarget.counter(), 4, 'should have executed multiple actions') + }) - await executorApp.executeWithBan(script, ['0x1234']) + it('executes multi action script to multiple addresses', async () => { + const executionTarget2 = await ExecutionTarget.new() - assert.equal(await executionTarget.counter(), 1, 'should have executed action') - }) + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const action2 = { to: executionTarget2.address, calldata: executionTarget2.contract.execute.getData() } - it('executes multi action script', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action, action, action, action]) + const script = encodeCallScript([action2, action, action2, action, action2]) - await executorApp.execute(script) + await scriptRunnerApp.runScript(script) - assert.equal(await executionTarget.counter(), 4, 'should have executed action') - }) + assert.equal(await executionTarget.counter(), 2, 'should have executed action') + assert.equal(await executionTarget2.counter(), 3, 'should have executed action') + }) - it('executes multi action script to multiple addresses', async () => { - const executionTarget2 = await ExecutionTarget.new() + it('executes with parameters', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } + const script = encodeCallScript([action]) - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const action2 = { to: executionTarget2.address, calldata: executionTarget2.contract.execute.getData() } + await scriptRunnerApp.runScript(script) - const script = encodeCallScript([action2, action, action2, action, action2]) + assert.equal(await executionTarget.counter(), 101, 'should have set counter') + }) - await executorApp.execute(script) + it('execution fails if one call fails', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } + const action2 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(true) } - assert.equal(await executionTarget.counter(), 2, 'should have executed action') - assert.equal(await executionTarget2.counter(), 3, 'should have executed action') - }) + const script = encodeCallScript([action1, action2]) - it('executes with parameters', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } - const script = encodeCallScript([action]) + await assertRevert(scriptRunnerApp.runScript(script), ERROR_EXECUTION_TARGET) + }) - await executorApp.execute(script) + it('execution fails with correctly forwarded error data', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(true) } - assert.equal(await executionTarget.counter(), 101, 'should have set counter') - }) + const script = encodeCallScript([action1]) - it('execution fails if one call fails', async () => { - const action1 = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } - const action2 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData() } + await assertRevert(scriptRunnerApp.runScript(script), ERROR_EXECUTION_TARGET) + }) - const script = encodeCallScript([action1, action2]) + it('execution fails with default error data if no error data is returned', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(false) } - return assertRevert(async () => { - await executorApp.execute(script) - }) - }) + const script = encodeCallScript([action1]) - it('can execute empty script', async () => { - await executorApp.execute(encodeCallScript([])) - }) + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_CALL_REVERTED) + }) - context('> Script overflow', () => { - 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') + it('can execute empty script', async () => { + await scriptRunnerApp.runScript(encodeCallScript([])) + }) - // Remove 12 first 0s of padding for addr and 28 0s for uint32 - return script + addr.slice(24) + length.slice(56) + calldata.slice(2) - }, createExecutorId(1)) // spec 1 - } + it('fails to execute if has blacklist addresses being called', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) - it('fails if data length is too big', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScriptBad([action]) + await assertRevert(scriptRunnerApp.runScriptWithBan(script, [executionTarget.address]), reverts.EVMCALLS_BLACKLISTED_CALL) + }) - return assertRevert(async () => { - await executorApp.execute(script) - }) - }) - }) + context('> Script underflow', () => { + const encodeCallScriptAddressUnderflow = actions => { + return actions.reduce((script, { to }) => { + const addr = rawEncode(['address'], [to]).toString('hex') - context('> Registry actions', () => { - it("can't be executed once disabled", async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(1, { from: boss }) + // Remove too much of the address (should just remove first 12 0s of padding as addr) + return script + addr.slice(24 + 4) + }, createExecutorId(1)) // spec 1 + } - return assertRevert(async () => { - await executorApp.execute(encodeCallScript([])) - }) - }) + const encodeCallScriptCalldataUnderflow = actions => { + return actions.reduce((script, { to, calldata }) => { + const addr = rawEncode(['address'], [to]).toString('hex') + const length = rawEncode(['uint256'], [calldata.length]).toString('hex') - it('can be re-enabled', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) + // Remove too much of the calldataLength (should just remove first 28 0s of padding as uint32) + return script + addr.slice(24) + length.slice(56 + 4) + }, createExecutorId(1)) // spec 1 + } - // Disable then re-enable the executor - await reg.disableScriptExecutor(1, { from: boss }) - await reg.enableScriptExecutor(1, { from: boss }) + it('fails if data length is too small to contain address', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptAddressUnderflow([action]) - await executorApp.execute(encodeCallScript([])) - }) - }) + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) + }) + + it('fails if data length is too small to contain calldata', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptCalldataUnderflow([action]) + + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) + }) + }) + + context('> Script overflow', () => { + const encodeCallScriptCalldataOverflow = 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) + }, createExecutorId(1)) // spec 1 + } + + it('fails if data length is too big', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptCalldataOverflow([action]) + + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) }) + }) }) + }) }) diff --git a/test/evm_script_factory.js b/test/evm_script_factory.js new file mode 100644 index 000000000..8a9934d11 --- /dev/null +++ b/test/evm_script_factory.js @@ -0,0 +1,125 @@ +const { soliditySha3 } = require('web3-utils') +const { createExecutorId, encodeCallScript } = require('./helpers/evmScript') + +const Kernel = artifacts.require('Kernel') +const ACL = artifacts.require('ACL') +const EVMScriptRegistry = artifacts.require('EVMScriptRegistry') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +// Mocks +const AppStubScriptRunner = artifacts.require('AppStubScriptRunner') +const ExecutionTarget = artifacts.require('ExecutionTarget') +const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock') + +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' +const EMPTY_BYTES = '0x' + +contract('EVM Script Factory', accounts => { + let evmScriptRegBase, callsScriptBase + let daoFact, regFact, dao, acl, evmScriptReg + let APP_BASES_NAMESPACE, APP_ADDR_NAMESPACE, APP_MANAGER_ROLE, CREATE_PERMISSIONS_ROLE + let EVMSCRIPT_REGISTRY_APP_ID, REGISTRY_ADD_EXECUTOR_ROLE, REGISTRY_MANAGER_ROLE + + const permissionsRoot = accounts[0] + + const scriptRunnerAppId = '0x1234' + + before(async () => { + const kernelBase = await Kernel.new(true) // petrify immediately + const aclBase = await ACL.new() + + regFact = await EVMScriptRegistryFactory.new() + daoFact = await DAOFactory.new(kernelBase.address, aclBase.address, regFact.address) + callsScriptBase = await regFact.baseCallScript() + evmScriptRegBase = EVMScriptRegistry.at(await regFact.baseReg()) + const evmScriptRegConstants = await EVMScriptRegistryConstantsMock.new() + + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + APP_ADDR_NAMESPACE = await kernelBase.APP_ADDR_NAMESPACE() + APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + CREATE_PERMISSIONS_ROLE = await aclBase.CREATE_PERMISSIONS_ROLE() + + EVMSCRIPT_REGISTRY_APP_ID = await evmScriptRegConstants.getEVMScriptRegistryAppId() + REGISTRY_ADD_EXECUTOR_ROLE = await evmScriptRegBase.REGISTRY_ADD_EXECUTOR_ROLE() + REGISTRY_MANAGER_ROLE = await evmScriptRegBase.REGISTRY_MANAGER_ROLE() + }) + + beforeEach(async () => { + const receipt = await daoFact.newDAO(permissionsRoot) + dao = Kernel.at(receipt.logs.filter(l => l.event == 'DeployDAO')[0].args.dao) + evmScriptReg = EVMScriptRegistry.at(receipt.logs.filter(l => l.event == 'DeployEVMScriptRegistry')[0].args.reg) + + acl = ACL.at(await dao.acl()) + }) + + it('factory installed EVMScriptRegistry correctly', async () => { + assert.equal(await dao.getApp(APP_BASES_NAMESPACE, EVMSCRIPT_REGISTRY_APP_ID), evmScriptRegBase.address, 'should have set correct base EVMScriptRegistry app in kernel') + assert.equal(await dao.getApp(APP_ADDR_NAMESPACE, EVMSCRIPT_REGISTRY_APP_ID), evmScriptReg.address, 'should have set correct default EVMScriptRegistry app in kernel') + assert.isTrue(await evmScriptReg.hasInitialized(), "should be initialized") + }) + + it('factory registered just 1 script executor', async () => { + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(0)), ZERO_ADDR, 'spec ID 0 should always be empty') + assert.notEqual(await evmScriptReg.getScriptExecutor(createExecutorId(1)), callsScriptBase.address, 'spec ID 1 should be calls script') + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(2)), ZERO_ADDR, 'spec ID 2 and higher should be empty') + }) + + it('factory cleaned up permissions', async () => { + assert.isFalse(await acl.hasPermission(regFact.address, dao.address, APP_MANAGER_ROLE), 'EVMScriptRegistryFactory should not have APP_MANAGER_ROLE for created kernel') + assert.isFalse(await acl.hasPermission(regFact.address, acl.address, CREATE_PERMISSIONS_ROLE), 'EVMScriptRegistryFactory should not have CREATE_PERMISSIONS_ROLE for created ACL') + assert.isFalse(await acl.hasPermission(regFact.address, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE), 'EVMScriptRegistryFactory should not have REGISTRY_ADD_EXECUTOR_ROLE for created EVMScriptRegistry') + assert.isFalse(await acl.hasPermission(regFact.address, evmScriptReg.address, REGISTRY_MANAGER_ROLE), 'EVMScriptRegistryFactory should not have REGISTRY_MANAGER_ROLE for created EVMScriptRegistry') + + assert.equal(await acl.getPermissionManager(evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE), ZERO_ADDR, 'created EVMScriptRegistry should not have manager for REGISTRY_ADD_EXECUTOR_ROLE') + assert.equal(await acl.getPermissionManager(evmScriptReg.address, REGISTRY_MANAGER_ROLE), ZERO_ADDR, 'created EVMScriptRegistry should not have manager for REGISTRY_MANAGER_ROLE') + }) + + context('> Executor app', () => { + let scriptRunnerAppBase, scriptRunnerApp, executionTarget + + before(async () => { + scriptRunnerAppBase = await AppStubScriptRunner.new() + }) + + beforeEach(async () => { + // Set up app management permissions + await acl.createPermission(permissionsRoot, dao.address, APP_MANAGER_ROLE, permissionsRoot) + + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await scriptRunnerApp.initialize() + executionTarget = await ExecutionTarget.new() + }) + + it('gets the correct executor registry from the app', async () => { + const registryFromApp = await scriptRunnerApp.getEVMScriptRegistry() + assert.equal(evmScriptReg.address, registryFromApp, 'app should return the same EVMScriptRegistry') + }) + + it('gets the correct executor from the app', async () => { + const script = createExecutorId(1) + const executor = await evmScriptReg.getScriptExecutor(script) + + const scriptExecutor = await scriptRunnerApp.getEVMScriptExecutor(script) + assert.equal(executor, scriptExecutor, 'app should return the same evm script executor') + }) + + it('can execute calls script (spec ID 1)', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action], 1) + + const receipt = await scriptRunnerApp.runScript(script) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + + // Check logs + // The executor always uses 0x for the input and callscripts always have 0x returns + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.executor, await evmScriptReg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') + assert.equal(scriptResult.args.script, script, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the return data') + }) + }) +}) diff --git a/test/helpers/assertThrow.js b/test/helpers/assertThrow.js index 94633e96a..f16d8a7fa 100644 --- a/test/helpers/assertThrow.js +++ b/test/helpers/assertThrow.js @@ -1,26 +1,38 @@ -function assertError(error, s, message) { - assert.isAbove(error.message.search(s), -1, `${message}\n\t(error: ${error})`); +const THROW_ERROR_PREFIX = 'VM Exception while processing transaction:' + +function assertError(error, expectedErrorCode) { + assert(error.message.search(expectedErrorCode) > -1, `Expected error code "${expectedErrorCode}" but failed with "${error}" instead.`) } -async function assertThrows(codeBlock, message, errorCode) { - try { - await codeBlock() - } catch (e) { - return assertError(e, errorCode, message) - } - assert.fail('should have thrown before') +async function assertThrows(blockOrPromise, expectedErrorCode, expectedReason) { + try { + (typeof blockOrPromise === 'function') ? await blockOrPromise() : await blockOrPromise + } catch (error) { + assertError(error, expectedErrorCode) + return error + } + // assert.fail() for some reason does not have its error string printed 🤷 + assert(0, `Expected "${expectedErrorCode}"${expectedReason ? ` (with reason: "${expectedReason}")` : ''} but it did not fail`) } module.exports = { - async assertJump(codeBlock, message = 'should have failed with invalid JUMP') { - return assertThrows(codeBlock, message, 'invalid JUMP') - }, + async assertJump(blockOrPromise) { + return assertThrows(blockOrPromise, 'invalid JUMP') + }, - async assertInvalidOpcode(codeBlock, message = 'should have failed with invalid opcode') { - return assertThrows(codeBlock, message, 'invalid opcode') - }, + async assertInvalidOpcode(blockOrPromise) { + return assertThrows(blockOrPromise, 'invalid opcode') + }, - async assertRevert(codeBlock, message = 'should have failed by reverting') { - return assertThrows(codeBlock, message, 'revert') - }, + async assertRevert(blockOrPromise, reason) { + const error = await assertThrows(blockOrPromise, 'revert', reason) + const errorPrefix = `${THROW_ERROR_PREFIX} revert` + if (error.message.includes(errorPrefix)) { + error.reason = error.message.replace(errorPrefix, '').trim() + } + + if (process.env.SOLIDITY_COVERAGE !== 'true' && reason) { + assert.equal(reason, error.reason, `Expected revert reason "${reason}" but failed with "${error.reason || 'no reason'}" instead.`) + } + }, } diff --git a/test/helpers/evmScript.js b/test/helpers/evmScript.js index 999718519..8b44f110b 100644 --- a/test/helpers/evmScript.js +++ b/test/helpers/evmScript.js @@ -1,16 +1,21 @@ const abi = require('ethereumjs-abi') +const createExecutorId = id => `0x${String(id).padStart(8, '0')}` + module.exports = { - // Encodes an array of actions ({ to: address, calldata: bytes}) into the EVM call script format - // Sets spec id 1 = 0x00000001 and - // Concatenates per call [ 20 bytes (address) ] + [ 4 bytes (uint32: calldata length) ] + [ calldataLength bytes (payload) ] - encodeCallScript: actions => { + createExecutorId, + + // Encodes an array of actions ({ to: address, calldata: bytes }) into the EVM call script format + // Sets spec id and concatenates per call + // [ 20 bytes (address) ] + [ 4 bytes (uint32: calldata length) ] + [ calldataLength bytes (payload) ] + // Defaults spec id to 1 + encodeCallScript: (actions, specId = 1) => { return actions.reduce((script, { to, calldata }) => { const addr = abi.rawEncode(['address'], [to]).toString('hex') const length = abi.rawEncode(['uint256'], [(calldata.length - 2) / 2]).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 + }, createExecutorId(specId)) }, } diff --git a/test/helpers/revertStrings.js b/test/helpers/revertStrings.js new file mode 100644 index 000000000..0c8d440cb --- /dev/null +++ b/test/helpers/revertStrings.js @@ -0,0 +1,74 @@ +function makeErrorMappingProxy (target) { + return new Proxy(target, { + get (target, property) { + if (property in target) { + return target[property] + } + + throw new Error(`Could not find error ${property} in error mapping`) + }, + set () { + throw new Error('Unexpected set to error mapping') + } + }) +} + +module.exports = makeErrorMappingProxy({ + // acl/ACL.sol + ACL_AUTH_INIT_KERNEL: 'ACL_AUTH_INIT_KERNEL', + ACL_AUTH_NO_MANAGER: 'ACL_AUTH_NO_MANAGER', + ACL_EXISTENT_MANAGER: 'ACL_EXISTENT_MANAGER', + + // apm/APMRegistry.sol + APMREG_INIT_PERMISSIONS: 'APMREG_INIT_PERMISSIONS', + APMREG_EMPTY_NAME: 'APMREG_EMPTY_NAME', + + // apm/Repo.sol + REPO_INVALID_BUMP: 'REPO_INVALID_BUMP', + REPO_INVALID_VERSION: 'REPO_INVALID_VERSION', + REPO_INEXISTENT_VERSION: 'REPO_INEXISTENT_VERSION', + + // apps/AragonApp.sol + APP_AUTH_FAILED: 'APP_AUTH_FAILED', + + // common/Initializable.sol + INIT_ALREADY_INITIALIZED: 'INIT_ALREADY_INITIALIZED', + INIT_NOT_INITIALIZED: 'INIT_NOT_INITIALIZED', + + // common/SafeERC20.sol + SAFE_ERC_20_BALANCE_REVERTED: 'SAFE_ERC_20_BALANCE_REVERTED', + SAFE_ERC_20_ALLOWANCE_REVERTED: 'SAFE_ERC_20_ALLOWANCE_REVERTED', + + // common/Uint256Helpers.sol + UINT64_NUMBER_TOO_BIG: 'UINT64_NUMBER_TOO_BIG', + + // common/VaultRecoverable.sol + RECOVER_DISALLOWED: 'RECOVER_DISALLOWED', + RECOVER_VAULT_NOT_CONTRACT: 'RECOVER_VAULT_NOT_CONTRACT', + RECOVER_TOKEN_TRANSFER_FAILED: 'RECOVER_TOKEN_TRANSFER_FAILED', + + // ens/ENSSubdomainRegistrar.sol + ENSSUB_NO_NODE_OWNERSHIP: 'ENSSUB_NO_NODE_OWNERSHIP', + ENSSUB_NAME_EXISTS: 'ENSSUB_NAME_EXISTS', + ENSSUB_DOESNT_EXIST: 'ENSSUB_DOESNT_EXIST', + + // evmscript/EVMScriptRegistry.sol + EVMREG_INEXISTENT_EXECUTOR: 'EVMREG_INEXISTENT_EXECUTOR', + EVMREG_EXECUTOR_ENABLED: 'EVMREG_EXECUTOR_ENABLED', + EVMREG_EXECUTOR_DISABLED: 'EVMREG_EXECUTOR_DISABLED', + EVMREG_SCRIPT_LENGTH_TOO_SHORT: 'EVMREG_SCRIPT_LENGTH_TOO_SHORT', + + // evmscript/EVMScriptRunner.sol + EVMRUN_EXECUTOR_UNAVAILABLE: 'EVMRUN_EXECUTOR_UNAVAILABLE', + EVMRUN_PROTECTED_STATE_MODIFIED: 'EVMRUN_PROTECTED_STATE_MODIFIED', + + // evmscript/executors/CallsScript.sol + EVMCALLS_BLACKLISTED_CALL: 'EVMCALLS_BLACKLISTED_CALL', + EVMCALLS_INVALID_LENGTH: 'EVMCALLS_INVALID_LENGTH', + EVMCALLS_CALL_REVERTED: 'EVMCALLS_CALL_REVERTED', + + // kernel/Kernel.sol + KERNEL_APP_NOT_CONTRACT: 'KERNEL_APP_NOT_CONTRACT', + KERNEL_INVALID_APP_CHANGE: 'KERNEL_INVALID_APP_CHANGE', + KERNEL_AUTH_FAILED: 'KERNEL_AUTH_FAILED', +})