4.2.0

Release aragonOS 4.2.0

contracts/acl/ACL.sol

@@ -1,6 +1,7 @@
 pragma solidity 0.4.24;
 
 import "../apps/AragonApp.sol";
+import "../common/ConversionHelpers.sol";
 import "../common/TimeHelpers.sol";
 import "./ACLSyntaxSugar.sol";
 import "./IACL.sol";
@@ -242,17 +243,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
     * @return boolean indicating whether the ACL allows the role or not
     */
     function hasPermission(address _who, address _where, bytes32 _what, bytes memory _how) public view returns (bool) {
-        // Force cast the bytes array into a uint256[], by overwriting its length
-        // Note that the uint256[] doesn't need to be initialized as we immediately overwrite it
-        // with _how and a new length, and _how becomes invalid from this point forward
-        uint256[] memory how;
-        uint256 intsLength = _how.length / 32;
-        assembly {
-            how := _how
-            mstore(how, intsLength)
-        }
-
-        return hasPermission(_who, _where, _what, how);
+        return hasPermission(_who, _where, _what, ConversionHelpers.dangerouslyCastBytesToUintArray(_how));
     }
 
     function hasPermission(address _who, address _where, bytes32 _what, uint256[] memory _how) public view returns (bool) {

contracts/acl/ACLSyntaxSugar.sol

@@ -7,7 +7,7 @@ pragma solidity ^0.4.24;
 
 contract ACLSyntaxSugar {
     function arr() internal pure returns (uint256[]) {
-        // solium-disable-previous-line no-empty-blocks
+        return new uint256[](0);
     }
 
     function arr(bytes32 _a) internal pure returns (uint256[] r) {

contracts/apm/APMRegistry.sol

@@ -59,7 +59,7 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMInternalAppNames {
     }
 
     /**
-    * @notice Create new repo in registry with `_name` and first repo version
+    * @notice Create new repo in registry with `_name` and publish a first version with contract `_contractAddress` and content `@fromHex(_contentURI)`
     * @param _name Repo name
     * @param _dev Address that will be given permission to create versions
     * @param _initialSemanticVersion Semantic version for new repo version

contracts/apm/Repo.sol

@@ -38,7 +38,7 @@ contract Repo is AragonApp {
     }
 
     /**
-    * @notice Create new version for repo
+    * @notice Create new version with contract `_contractAddress` and content `@fromHex(_contentURI)`
     * @param _newSemanticVersion Semantic version for new repo version
     * @param _contractAddress address for smart contract logic for version (if set to 0, it uses last versions' contractAddress)
     * @param _contentURI External URI for fetching new version's content

contracts/apps/AragonApp.sol

@@ -5,18 +5,20 @@
 pragma solidity ^0.4.24;
 
 import "./AppStorage.sol";
+import "../acl/ACLSyntaxSugar.sol";
 import "../common/Autopetrified.sol";
+import "../common/ConversionHelpers.sol";
+import "../common/ReentrancyGuard.sol";
 import "../common/VaultRecoverable.sol";
 import "../evmscript/EVMScriptRunner.sol";
-import "../acl/ACLSyntaxSugar.sol";
 
 
 // Contracts inheriting from AragonApp are, by default, immediately petrified upon deployment so
 // that they can never be initialized.
 // Unless overriden, this behaviour enforces those contracts to be usable only behind an AppProxy.
-// ACLSyntaxSugar and EVMScriptRunner are not directly used by this contract, but are included so
-// that they are automatically usable by subclassing contracts
-contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunner, ACLSyntaxSugar {
+// ReentrancyGuard, EVMScriptRunner, and ACLSyntaxSugar are not directly used by this contract, but
+// are included so that they are automatically usable by subclassing contracts
+contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGuard, EVMScriptRunner, ACLSyntaxSugar {
     string private constant ERROR_AUTH_FAILED = "APP_AUTH_FAILED";
 
     modifier auth(bytes32 _role) {
@@ -47,16 +49,12 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunn
             return false;
         }
 
-        // Force cast the uint256[] into a bytes array, by overwriting its length
-        // Note that the bytes array doesn't need to be initialized as we immediately overwrite it
-        // with _params and a new length, and _params becomes invalid from this point forward
-        bytes memory how;
-        uint256 byteLength = _params.length * 32;
-        assembly {
-            how := _params
-            mstore(how, byteLength)
-        }
-        return linkedKernel.hasPermission(_sender, address(this), _role, how);
+        return linkedKernel.hasPermission(
+            _sender,
+            address(this),
+            _role,
+            ConversionHelpers.dangerouslyCastUintArrayToBytes(_params)
+        );
     }
 
     /**

contracts/common/ConversionHelpers.sol

@@ -0,0 +1,30 @@
+pragma solidity ^0.4.24;
+
+
+library ConversionHelpers {
+    string private constant ERROR_IMPROPER_LENGTH = "CONVERSION_IMPROPER_LENGTH";
+
+    function dangerouslyCastUintArrayToBytes(uint256[] memory _input) internal pure returns (bytes memory output) {
+        // Force cast the uint256[] into a bytes array, by overwriting its length
+        // Note that the bytes array doesn't need to be initialized as we immediately overwrite it
+        // with the input and a new length. The input becomes invalid from this point forward.
+        uint256 byteLength = _input.length * 32;
+        assembly {
+            output := _input
+            mstore(output, byteLength)
+        }
+    }
+
+    function dangerouslyCastBytesToUintArray(bytes memory _input) internal pure returns (uint256[] memory output) {
+        // Force cast the bytes array into a uint256[], by overwriting its length
+        // Note that the uint256[] doesn't need to be initialized as we immediately overwrite it
+        // with the input and a new length. The input becomes invalid from this point forward.
+        uint256 intsLength = _input.length / 32;
+        require(_input.length == intsLength * 32, ERROR_IMPROPER_LENGTH);
+
+        assembly {
+            output := _input
+            mstore(output, intsLength)
+        }
+    }
+}

contracts/common/Initializable.sol

@@ -4,8 +4,8 @@
 
 pragma solidity ^0.4.24;
 
+import "./TimeHelpers.sol";
 import "./UnstructuredStorage.sol";
-import "../common/TimeHelpers.sol";
 
 
 contract Initializable is TimeHelpers {

contracts/common/ReentrancyGuard.sol

@@ -0,0 +1,33 @@
+/*
+ * SPDX-License-Identitifer:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "../common/UnstructuredStorage.sol";
+
+
+contract ReentrancyGuard {
+    using UnstructuredStorage for bytes32;
+
+    /* Hardcoded constants to save gas
+    bytes32 internal constant REENTRANCY_MUTEX_POSITION = keccak256("aragonOS.reentrancyGuard.mutex");
+    */
+    bytes32 private constant REENTRANCY_MUTEX_POSITION = 0xe855346402235fdd185c890e68d2c4ecad599b88587635ee285bce2fda58dacb;
+
+    string private constant ERROR_REENTRANT = "REENTRANCY_REENTRANT_CALL";
+
+    modifier nonReentrant() {
+        // Ensure mutex is unlocked
+        require(!REENTRANCY_MUTEX_POSITION.getStorageBool(), ERROR_REENTRANT);
+
+        // Lock mutex before function call
+        REENTRANCY_MUTEX_POSITION.setStorageBool(true);
+
+        // Perform function call
+        _;
+
+        // Unlock mutex after function call
+        REENTRANCY_MUTEX_POSITION.setStorageBool(false);
+    }
+}

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,9 +14,12 @@ 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";
 
+    /* This is manually crafted in assembly
+    string private constant ERROR_EXECUTOR_INVALID_RETURN = "EVMRUN_EXECUTOR_INVALID_RETURN";
+    */
+
     event ScriptResult(address indexed executor, bytes script, bytes input, bytes returnData);
 
     function getEVMScriptExecutor(bytes _script) public view returns (IEVMScriptExecutor) {
@@ -34,36 +37,66 @@ 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
+                switch gt(returndatasize, 0x3f)
+                case 0 {
+                    // Need at least 0x40 bytes returned for properly ABI-encoded bytes values,
+                    // revert with "EVMRUN_EXECUTOR_INVALID_RETURN"
+                    // See remix: doing a `revert("EVMRUN_EXECUTOR_INVALID_RETURN")` always results in
+                    // this memory layout
+                    mstore(output, 0x08c379a000000000000000000000000000000000000000000000000000000000)         // error identifier
+                    mstore(add(output, 0x04), 0x0000000000000000000000000000000000000000000000000000000000000020) // starting offset
+                    mstore(add(output, 0x24), 0x000000000000000000000000000000000000000000000000000000000000001e) // reason length
+                    mstore(add(output, 0x44), 0x45564d52554e5f4558454355544f525f494e56414c49445f52455455524e0000) // reason
+
+                    revert(output, 100) // 100 = 4 + 3 * 32 (error identifier + 3 words for the ABI encoded error)
+                }
+                default {
+                    // 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) {