Merge pull request #255 from aragon/evmscript-min-return

Require minimum return data from EVMScript executors to prevent selfdestructs

contracts/evmscript/EVMScriptRunner.sol

@@ -20,7 +20,11 @@ contract EVMScriptRunner is AppStorage, EVMScriptRegistryConstants {
 
         require(executorAddr.delegatecall(sig, calldataArgs));
 
-        return returnedDataDecoded();
+        bytes memory ret = returnedDataDecoded();
+
+        require(ret.length > 0);
+
+        return ret;
     }
 
     function getExecutor(bytes _script) public view returns (IEVMScriptExecutor) {
@@ -57,4 +61,4 @@ contract EVMScriptRunner is AppStorage, EVMScriptRegistryConstants {
         require(kernel == preKernel);
         require(appId == preAppId);
     }
-}
+}

contracts/evmscript/executors/CallsScript.sol

@@ -47,5 +47,10 @@ contract CallsScript is IEVMScriptExecutor {
                 switch success case 0 { revert(0, 0) }
             }
         }
+
+        bytes memory ret = new bytes(1);
+        ret[0] = 1;
+
+        return ret;
     }
 }

contracts/evmscript/executors/DelegateScript.sol

@@ -5,7 +5,7 @@ import "../IEVMScriptExecutor.sol";
 
 
 interface DelegateScriptTarget {
-    function exec() public;
+    function exec() public returns (bool);
 }
 
 
@@ -35,7 +35,11 @@ contract DelegateScript is IEVMScriptExecutor {
     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();
+        bytes memory ret = returnedData();
+
+        require(ret.length > 0);
+
+        return ret;
     }
 
     function isContract(address _target) internal view returns (bool) {
@@ -61,4 +65,4 @@ contract DelegateScript is IEVMScriptExecutor {
         }
         return ret;
     }
-}
+}

test/evm_script.js

@@ -7,7 +7,9 @@ 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')
@@ -71,6 +73,8 @@ contract('EVM Script', accounts => {
             const receipt = await dao.newAppInstance(executorAppId, '0x0', { 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 () => {
@@ -86,7 +90,6 @@ 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([]))
@@ -205,6 +208,14 @@ 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
@@ -280,6 +291,30 @@ 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 }) => {

test/mocks/Delegator.sol

@@ -5,15 +5,20 @@ import "./Executor.sol";
 
 
 contract Delegator is ExecutorStorage, DelegateScriptTarget {
-    function exec() public {
+    function exec() public returns (bool) {
         randomNumber += 1234;
     }
 
     function execReturnValue(uint i) public pure returns (uint) { return i; }
 }
 
 contract FailingDelegator is DelegateScriptTarget {
-    function exec() public { revert(); }
+    function exec() public returns (bool) { revert(); }
+}
+
+
+contract DyingDelegator is DelegateScriptTarget {
+    function exec() public returns (bool) { selfdestruct(0); }
 }
 
 
@@ -23,14 +28,13 @@ contract FailingDeployment {
 
 
 contract ProtectionModifierKernel is ExecutorStorage, DelegateScriptTarget {
-    function exec() public {
+    function exec() public returns (bool) {
         kernel = IKernel(0x1234);
     }
 }
 
-
 contract ProtectionModifierAppId is ExecutorStorage, DelegateScriptTarget {
-    function exec() public {
+    function exec() public returns (bool) {
         appId = bytes32(123456);
     }
 }

test/mocks/MockDyingScriptExecutor.sol

@@ -0,0 +1,12 @@
+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);
+    }
+}