EVMScripts: check byte lengths, avoid returning unallocated memory, and forward error data (#496)

Author: sohkai

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

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 {

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) {

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);
+    }
+    */
+}

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) {

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;
+    }
+}

contracts/test/mocks/ExecutionTarget.sol

@@ -2,15 +2,20 @@ pragma solidity 0.4.24;
 
 
 contract ExecutionTarget {
+    string public constant ERROR_EXECUTION_TARGET = "EXECUTION_TARGET_REVERTED";
     uint public counter;
 
     function execute() public {
         counter += 1;
         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 {