encodeParam and encodeParams functions

.solcover.js

@@ -3,6 +3,7 @@ const skipFiles = [
   'test',
   'acl/ACLSyntaxSugar.sol',
   'common/DepositableStorage.sol', // Used in tests that send ETH
+  'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287)
   'common/UnstructuredStorage.sol' // Used in tests that send ETH
 ]
 

contracts/acl/ACL.sol

@@ -15,16 +15,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
     */
     bytes32 public constant CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a;
 
-    enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types
-
-    struct Param {
-        uint8 id;
-        uint8 op;
-        uint240 value; // even though value is an uint240 it can store addresses
-        // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal
-        // op and id take less than 1 byte each so it can be kept in 1 sstore
-    }
-
     uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200;
     uint8 internal constant TIMESTAMP_PARAM_ID    = 201;
     // 202 is unused

contracts/acl/ACLParams.sol

@@ -0,0 +1,19 @@
+/*
+ * SPDX-License-Identitifer:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+
+contract ACLParams {
+
+    enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types
+
+    struct Param {
+        uint8 id;
+        uint8 op;
+        uint240 value; // even though value is an uint240 it can store addresses
+        // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal
+        // op and id take less than 1 byte each so it can be kept in 1 sstore
+    }
+}

contracts/acl/ACLSyntaxSugar.sol

@@ -4,9 +4,13 @@
 
 pragma solidity ^0.4.24;
 
+import "./ACLParams.sol";
+
 
 contract ACLSyntaxSugar {
-    function arr() internal pure returns (uint256[]) {}
+    function arr() internal pure returns (uint256[]) {
+        // solium-disable-previous-line no-empty-blocks
+    }
 
     function arr(bytes32 _a) internal pure returns (uint256[] r) {
         return arr(uint256(_a));
@@ -85,7 +89,7 @@ contract ACLSyntaxSugar {
 }
 
 
-contract ACLHelpers {
+contract ACLHelpers is ACLParams {
     function decodeParamOp(uint256 _x) internal pure returns (uint8 b) {
         return uint8(_x >> (8 * 30));
     }
@@ -99,4 +103,26 @@ contract ACLHelpers {
         b = uint32(_x >> (8 * 4));
         c = uint32(_x >> (8 * 8));
     }
+
+    function encodeParams(Param[] params) internal pure returns (uint256[]) {
+        uint256[] memory encodedParams = new uint256[](params.length);
+
+        for (uint i = 0; i < params.length; i++) {
+            encodedParams[i] = encodeParam(params[i]);
+        }
+
+        return encodedParams;
+    }
+
+    function encodeParam(Param param) internal pure returns (uint256) {
+        return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value;
+    }  
+
+    function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) {
+        return uint240(param1 + (param2 << 32) + (0 << 64));
+    }
+
+    function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) {
+        return uint240(condition + (successParam << 32) + (failureParam << 64));
+    }  
 }

contracts/apps/AppProxyUpgradeable.sol

@@ -14,7 +14,7 @@ contract AppProxyUpgradeable is AppProxyBase {
         AppProxyBase(_kernel, _appId, _initializePayload)
         public // solium-disable-line visibility-first
     {
-
+        // solium-disable-previous-line no-empty-blocks
     }
 
     /**

contracts/common/SafeERC20.sol

@@ -0,0 +1,156 @@
+// Inspired by AdEx (https://github.com/AdExNetwork/adex-protocol-eth/blob/b9df617829661a7518ee10f4cb6c4108659dd6d5/contracts/libs/SafeERC20.sol)
+// and 0x (https://github.com/0xProject/0x-monorepo/blob/737d1dc54d72872e24abce5a1dbe1b66d35fa21a/contracts/protocol/contracts/protocol/AssetProxy/ERC20Proxy.sol#L143)
+
+pragma solidity ^0.4.24;
+
+import "../lib/token/ERC20.sol";
+
+
+library SafeERC20 {
+    // Before 0.5, solidity has a mismatch between `address.transfer()` and `token.transfer()`:
+    // https://github.com/ethereum/solidity/issues/3544
+    bytes4 private constant TRANSFER_SELECTOR = 0xa9059cbb;
+
+    string private constant ERROR_TOKEN_BALANCE_REVERTED = "SAFE_ERC_20_BALANCE_REVERTED";
+    string private constant ERROR_TOKEN_ALLOWANCE_REVERTED = "SAFE_ERC_20_ALLOWANCE_REVERTED";
+
+    function invokeAndCheckSuccess(address _addr, bytes memory _calldata)
+        private
+        returns (bool)
+    {
+        bool ret;
+        assembly {
+            let ptr := mload(0x40)    // free memory pointer
+
+            let success := call(
+                gas,                  // forward all gas
+                _addr,                // address
+                0,                    // no value
+                add(_calldata, 0x20), // calldata start
+                mload(_calldata),     // calldata length
+                ptr,                  // write output over free memory
+                0x20                  // uint256 return
+            )
+
+            if gt(success, 0) {
+                // Check number of bytes returned from last function call
+                switch returndatasize
+
+                // No bytes returned: assume success
+                case 0 {
+                    ret := 1
+                }
+
+                // 32 bytes returned: check if non-zero
+                case 0x20 {
+                    // Only return success if returned data was true
+                    // Already have output in ptr
+                    ret := eq(mload(ptr), 1)
+                }
+
+                // Not sure what was returned: don't mark as success
+                default { }
+            }
+        }
+        return ret;
+    }
+
+    function staticInvoke(address _addr, bytes memory _calldata)
+        private
+        view
+        returns (bool, uint256)
+    {
+        bool success;
+        uint256 ret;
+        assembly {
+            let ptr := mload(0x40)    // free memory pointer
+
+            success := staticcall(
+                gas,                  // forward all gas
+                _addr,                // address
+                add(_calldata, 0x20), // calldata start
+                mload(_calldata),     // calldata length
+                ptr,                  // write output over free memory
+                0x20                  // uint256 return
+            )
+
+            if gt(success, 0) {
+                ret := mload(ptr)
+            }
+        }
+        return (success, ret);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.transfer() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) {
+        bytes memory transferCallData = abi.encodeWithSelector(
+            TRANSFER_SELECTOR,
+            _to,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, transferCallData);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.transferFrom() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeTransferFrom(ERC20 _token, address _from, address _to, uint256 _amount) internal returns (bool) {
+        bytes memory transferFromCallData = abi.encodeWithSelector(
+            _token.transferFrom.selector,
+            _from,
+            _to,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, transferFromCallData);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.approve() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeApprove(ERC20 _token, address _spender, uint256 _amount) internal returns (bool) {
+        bytes memory approveCallData = abi.encodeWithSelector(
+            _token.approve.selector,
+            _spender,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, approveCallData);
+    }
+
+    /**
+    * @dev Static call into ERC20.balanceOf().
+    * Reverts if the call fails for some reason (should never fail).
+    */
+    function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) {
+        bytes memory balanceOfCallData = abi.encodeWithSelector(
+            _token.balanceOf.selector,
+            _owner
+        );
+
+        (bool success, uint256 tokenBalance) = staticInvoke(_token, balanceOfCallData);
+        require(success, ERROR_TOKEN_BALANCE_REVERTED);
+
+        return tokenBalance;
+    }
+
+    /**
+    * @dev Static call into ERC20.allowance().
+    * Reverts if the call fails for some reason (should never fail).
+    */
+    function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) {
+        bytes memory allowanceCallData = abi.encodeWithSelector(
+            _token.allowance.selector,
+            _owner,
+            _spender
+        );
+
+        (bool success, uint256 allowance) = staticInvoke(_token, allowanceCallData);
+        require(success, ERROR_TOKEN_ALLOWANCE_REVERTED);
+
+        return allowance;
+    }
+}

contracts/common/VaultRecoverable.sol

@@ -8,11 +8,15 @@ import "../lib/token/ERC20.sol";
 import "./EtherTokenConstant.sol";
 import "./IsContract.sol";
 import "./IVaultRecoverable.sol";
+import "./SafeERC20.sol";
 
 
 contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract {
+    using SafeERC20 for ERC20;
+
     string private constant ERROR_DISALLOWED = "RECOVER_DISALLOWED";
     string private constant ERROR_VAULT_NOT_CONTRACT = "RECOVER_VAULT_NOT_CONTRACT";
+    string private constant ERROR_TOKEN_TRANSFER_FAILED = "RECOVER_TOKEN_TRANSFER_FAILED";
 
     /**
      * @notice Send funds to recovery Vault. This contract should never receive funds,
@@ -27,8 +31,9 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract {
         if (_token == ETH) {
             vault.transfer(address(this).balance);
         } else {
-            uint256 amount = ERC20(_token).balanceOf(this);
-            ERC20(_token).transfer(vault, amount);
+            ERC20 token = ERC20(_token);
+            uint256 amount = token.staticBalanceOf(this);
+            require(token.safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED);
         }
     }
 

contracts/kernel/IKernel.sol

@@ -8,10 +8,13 @@ import "../acl/IACL.sol";
 import "../common/IVaultRecoverable.sol";
 
 
-// This should be an interface, but interfaces can't inherit yet :(
-contract IKernel is IVaultRecoverable {
+interface IKernelEvents {
     event SetApp(bytes32 indexed namespace, bytes32 indexed appId, address app);
+}
 
+
+// This should be an interface, but interfaces can't inherit yet :(
+contract IKernel is IKernelEvents, IVaultRecoverable {
     function acl() public view returns (IACL);
     function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool);
 

contracts/kernel/KernelProxy.sol

@@ -7,7 +7,7 @@ import "../common/DepositableDelegateProxy.sol";
 import "../common/IsContract.sol";
 
 
-contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy {
+contract KernelProxy is IKernelEvents, KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy {
     /**
     * @dev KernelProxy is a proxy contract to a kernel implementation. The implementation
     *      can update the reference, which effectively upgrades the contract
@@ -16,6 +16,12 @@ contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, I
     constructor(IKernel _kernelImpl) public {
         require(isContract(address(_kernelImpl)));
         apps[KERNEL_CORE_NAMESPACE][KERNEL_CORE_APP_ID] = _kernelImpl;
+
+        // Note that emitting this event is important for verifying that a KernelProxy instance
+        // was never upgraded to a malicious Kernel logic contract over its lifespan.
+        // This starts the "chain of trust", that can be followed through later SetApp() events
+        // emitted during kernel upgrades.
+        emit SetApp(KERNEL_CORE_NAMESPACE, KERNEL_CORE_APP_ID, _kernelImpl);
     }
 
     /**

contracts/test/TestACLHelpers.sol

@@ -0,0 +1,42 @@
+pragma solidity 0.4.24;
+
+import "./helpers/Assert.sol";
+import "../acl/ACLSyntaxSugar.sol";
+import "../acl/ACL.sol";
+
+
+contract TestACLHelpers is ACL {
+
+    function testEncodeParam() public {
+        Param memory param = Param(2, uint8(Op.EQ), 5294967297);
+
+        uint256 encodedParam = encodeParam(param);
+
+        (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam);
+
+        Assert.equal(uint256(param.id), uint256(id), "Encoded id is not equal");
+        Assert.equal(uint256(param.op), uint256(op), "Encoded op is not equal");
+        Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal");
+    }
+
+    function testEncodeParams() public {
+        Param[] memory params = new Param[](4);
+
+        params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3));
+        params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3));   
+        params[2] = Param(2, uint8(Op.EQ), 1);   
+        params[3] = Param(3, uint8(Op.NEQ), 2); 
+
+        uint256[] memory encodedParam = encodeParams(params);
+
+        for (uint256 i = 0; i < 4; i++) {
+            (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam[i]);
+
+            Assert.equal(uint256(params[i].id), uint256(id), "Encoded id is not equal");
+            Assert.equal(uint256(params[i].op), uint256(op), "Encoded op is not equal");
+            Assert.equal(uint256(params[i].value), uint256(value), "Encoded value is not equal");
+        } 
+
+    }    
+
+}

contracts/test/TestACLInterpreter.sol

@@ -2,10 +2,10 @@ pragma solidity 0.4.24;
 
 import "../acl/ACL.sol";
 import "./helpers/Assert.sol";
-import "./helpers/ACLHelper.sol";
+import "./helpers/ACLOracleHelper.sol";
 
 
-contract TestACLInterpreter is ACL, ACLHelper {
+contract TestACLInterpreter is ACL {
     function testEqualityUint() public {
         // Assert param 0 is equal to 10, given that params are [10, 11]
         assertEval(arr(uint256(10), 11), 0, Op.EQ, 10, true);