Agent app: EVM

future-apps/agent/contracts/Agent.sol

@@ -25,9 +25,6 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
     string private constant ERROR_EXECUTE_TARGET_NOT_CONTRACT = "AGENT_EXEC_TARGET_NO_CONTRACT";
     string private constant ERROR_DESIGNATED_TO_SELF = "AGENT_DESIGNATED_TO_SELF";
 
-    uint256 internal constant ISVALIDSIG_MAX_GAS = 250000;
-    uint256 internal constant EIP165_MAX_GAS = 30000;
-
     mapping (bytes32 => bool) public isPresigned;
     address public designatedSigner;
 
@@ -130,22 +127,11 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         // Short-circuit in case the hash was presigned. Optimization as performing calls
         // and ecrecover is more expensive than an SLOAD.
         if (isPresigned[hash]) {
-            return isValidSignatureReturn(true);
-        }
-
-        // Checks if designatedSigner is a contract, and if it supports the isValidSignature interface
-        if (safeSupportsInterface(IERC165(designatedSigner), ERC1271_INTERFACE_ID)) {
-            // designatedSigner.isValidSignature(hash, signature) as a staticall
-            ERC1271 signerContract = ERC1271(designatedSigner);
-            bytes memory data = abi.encodeWithSelector(signerContract.isValidSignature.selector, hash, signature);
-            return isValidSignatureReturn(safeBoolStaticCall(signerContract, data, ISVALIDSIG_MAX_GAS));
+            return returnIsValidSignatureMagicNumber(true);
         }
 
-        // `safeSupportsInterface` returns false if designatedSigner is a contract but it
-        // doesn't support the interface. Here we check the validity of the ECDSA sig
-        // which will always fail if designatedSigner is not an EOA
-
-        return isValidSignatureReturn(SignatureValidator.isValidSignature(hash, designatedSigner, signature));
+        bool isValid = SignatureValidator.isValidSignature(hash, designatedSigner, signature);
+        return returnIsValidSignatureMagicNumber(isValid);
     }
 
     function canForward(address sender, bytes evmScript) public view returns (bool) {
@@ -154,44 +140,6 @@ contract Agent is IERC165, ERC1271Bytes, IForwarder, IsContract, Vault {
         return canPerform(sender, RUN_SCRIPT_ROLE, params);
     }
 
-    function safeSupportsInterface(IERC165 target, bytes4 interfaceId) internal view returns (bool) {
-        if (!isContract(target)) {
-            return false;
-        }
-
-        bytes memory data = abi.encodeWithSelector(target.supportsInterface.selector, interfaceId);
-        return safeBoolStaticCall(target, data, EIP165_MAX_GAS);
-    }
-
-    function safeBoolStaticCall(address target, bytes data, uint256 maxGas) internal view returns (bool) {
-        uint256 gasLeft = gasleft();
-
-        uint256 callGas = gasLeft > maxGas ? maxGas : gasLeft;
-        bool ok;
-        assembly {
-            ok := staticcall(callGas, target, add(data, 0x20), mload(data), 0, 0)
-        }
-
-        if (!ok) {
-            return false;
-        }
-
-        uint256 size;
-        assembly { size := returndatasize }
-        if (size != 32) {
-            return false;
-        }
-
-        bool result;
-        assembly {
-            let ptr := mload(0x40)       // get next free memory ptr
-            returndatacopy(ptr, 0, size) // copy return from above `staticcall`
-            result := mload(ptr)         // read data at ptr and set it to result
-        }
-
-        return result;
-    }
-
     function getScriptACLParam(bytes evmScript) internal pure returns (uint256) {
         return uint256(keccak256(abi.encodePacked(evmScript)));
     }

future-apps/agent/contracts/SignatureValidator.sol

@@ -1,36 +1,74 @@
 pragma solidity 0.4.24;
 
-// From github.com/DexyProject/protocol
-// + https://github.com/0xProject/0x-monorepo/blob/master/packages/contracts/contracts/protocol/Exchange/MixinSignatureValidator.sol
+// Inspired in https://github.com/horizon-games/multi-token-standard/blob/master/contracts/utils/SignatureValidator.sol
 // This should probably be moved into aOS: https://github.com/aragon/aragonOS/pull/442
 
+import "./standards/ERC1271.sol";
+
 
 library SignatureValidator {
     enum SignatureMode {
-        Invalid,
-        EIP712,
-        EthSign
+        Invalid, // 0x00
+        EIP712,  // 0x01
+        EthSign, // 0x02
+        ERC1271, // 0x03
+        NMode    // 0x04, to check if mode is specified, leave at the end
     }
 
+    // bytes4(keccak256("isValidSignature(bytes,bytes)")
+    bytes4 constant public ERC1271_RETURN_VALID_SIGNATURE = 0x20c13b0b;
+    uint256 internal constant ERC1271_ISVALIDSIG_MAX_GAS = 250000;
+
     /// @dev Validates that a hash was signed by a specified signer.
     /// @param hash Hash which was signed.
     /// @param signer Address of the signer.
-    /// @param signature ECDSA signature along with the mode (0 = Invalid, 1 = EIP712, 2 = EthSign) {mode}{r}{s}{v}.
+    /// @param signature ECDSA signature along with the mode (0 = Invalid, 1 = EIP712, 2 = EthSign, 3 = ERC1271) {mode}{r}{s}{v}.
     /// @return Returns whether signature is from a specified user.
-    function isValidSignature(bytes32 hash, address signer, bytes signature) internal pure returns (bool) {
+    function isValidSignature(bytes32 hash, address signer, bytes signature) internal view returns (bool) {
         if (signature.length == 0) {
             return false;
         }
 
-        SignatureMode mode = SignatureMode(uint8(signature[0]));
+        uint8 modeByte = uint8(signature[0]);
+        if (modeByte >= uint8(SignatureMode.NMode)) {
+            return false;
+        }
+        SignatureMode mode = SignatureMode(modeByte);
 
-        if (mode == SignatureMode.Invalid || signature.length != 66) {
+        if (mode == SignatureMode.EIP712) {
+            return ecVerify(hash, signer, signature);
+        } else if (mode == SignatureMode.EthSign) {
+            return ecVerify(
+                keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)),
+                signer,
+                signature
+            );
+        } else if (mode == SignatureMode.ERC1271) {
+            // Pop the mode byte before sending it down the validation chain
+            (bytes memory newSig,) = popFirstByte(signature); // `signature` IS CORRUPTED AFTER EXECUTING THIS
+            return safeIsValidSignature(signer, hash, signature);
+        } else {
             return false;
         }
+    }
+
+    function ecVerify(bytes32 hash, address signer, bytes memory signature) internal pure returns (bool) {
+        (bool badSig, bytes32 r, bytes32 s, uint8 v) = unpackEcSig(signature);
 
-        uint8 v = uint8(signature[65]);
-        bytes32 r;
-        bytes32 s;
+        if (badSig) {
+            return false;
+        }
+
+        return signer == ecrecover(hash, v, r, s);
+    }
+
+    function unpackEcSig(bytes memory signature) internal pure returns (bool badSig, bytes32 r, bytes32 s, uint8 v) {
+        if (signature.length != 66) {
+            badSig = true;
+            return;
+        }
+
+        v = uint8(signature[65]);
         assembly {
             r := mload(add(signature, 33))
             s := mload(add(signature, 65))
@@ -42,18 +80,58 @@ library SignatureValidator {
         }
 
         if (v != 27 && v != 28) {
-            return false;
+            badSig = true;
         }
+    }
 
-        bytes32 signedHash;
-        if (mode == SignatureMode.EthSign) {
-            signedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash));
-        } else if (mode == SignatureMode.EIP712) {
-            signedHash = hash;
-        } else {
-            return false;
+    /**
+     * @dev DANGEROUS FUNCTION: Modifies input in place, making it invalid after using this function
+     * Memory layout: bytes in parenthesis are returned
+     * L = l - 1
+     * Input:  ([l1][l2]....[l32][b1][b2]....[bn])
+     * Output: [00]([L1][L2]....[L32][b2]....[bn])
+     */
+    function popFirstByte(bytes memory input) internal pure returns (bytes memory output, byte firstByte) {
+        assembly {
+            let length := mload(input)
+            firstByte := mload(add(input, 0x20))
+            output := add(input, 0x01)
+            mstore8(input, 0x00) // Probably safe to remove, as it will most already likely be 0
+            mstore(output, sub(length, 0x01))
+        }
+    }
+
+    function safeIsValidSignature(address validator, bytes32 hash, bytes memory signature) internal view returns (bool) {
+        bytes memory data = abi.encodeWithSelector(ERC1271(validator).isValidSignature.selector, hash, signature);
+        bytes4 erc1271Return = safeBytes4StaticCall(validator, data, ERC1271_ISVALIDSIG_MAX_GAS);
+        return erc1271Return == ERC1271_RETURN_VALID_SIGNATURE;
+    }
+
+    function safeBytes4StaticCall(address target, bytes data, uint256 maxGas) internal view returns (bytes4 ret) {
+        uint256 gasLeft = gasleft();
+
+        uint256 callGas = gasLeft > maxGas ? maxGas : gasLeft;
+        bool ok;
+        assembly {
+            ok := staticcall(callGas, target, add(data, 0x20), mload(data), 0, 0)
+        }
+
+        if (!ok) {
+            return;
+        }
+
+        uint256 size;
+        assembly { size := returndatasize }
+        if (size != 32) {
+            return;
+        }
+
+        assembly {
+            let ptr := mload(0x40)       // get next free memory ptr
+            returndatacopy(ptr, 0, size) // copy return from above `staticcall`
+            ret := mload(ptr)            // read data at ptr and set it to be returned
         }
 
-        return ecrecover(signedHash, v, r, s) == signer;
+        return ret;
     }
 }

future-apps/agent/contracts/standards/ERC1271.sol

@@ -21,7 +21,7 @@ contract ERC1271 {
     */ 
     function isValidSignature(bytes32 _hash, bytes memory _signature) public view returns (bytes4);
 
-    function isValidSignatureReturn(bool isValid) internal pure returns (bytes4) {
+    function returnIsValidSignatureMagicNumber(bool isValid) internal pure returns (bytes4) {
         return isValid ? ERC1271_RETURN_VALID_SIGNATURE : ERC1271_RETURN_INVALID_SIGNATURE;
     }
 }

future-apps/agent/contracts/test/mocks/DesignatedSigner.sol

@@ -10,6 +10,10 @@ contract DesignatedSigner /* is IERC165, ERC1271 */ {
     bool isValidRevert;
     bool modifyState;
 
+    // bytes4(keccak256("isValidSignature(bytes,bytes)")
+    bytes4 constant public ERC1271_RETURN_VALID_SIGNATURE =   0x20c13b0b; // TODO: Likely needs to be updated
+    bytes4 constant public ERC1271_RETURN_INVALID_SIGNATURE = 0x00000000;
+
     constructor (bool _isInterface, bool _isValid, bool _isValidRevert, bool _modifyState) public {
         isInterface = _isInterface;
         isValid = _isValid;
@@ -23,13 +27,13 @@ contract DesignatedSigner /* is IERC165, ERC1271 */ {
     }
 
     // Can't be ERC1271-compliant since this potentially modifies state
-    function isValidSignature(bytes32, bytes) external returns (bool) {
+    function isValidSignature(bytes32, bytes) external returns (bytes4) {
         require(!isValidRevert);
 
         if (modifyState) {
             modifyState = false;
         }
 
-        return isValid;
+        return isValid ? ERC1271_RETURN_VALID_SIGNATURE : ERC1271_RETURN_INVALID_SIGNATURE;
     }
 }

future-apps/agent/test/agent.js

@@ -234,6 +234,14 @@ contract('Agent app', (accounts) => {
     const [_, nobody, presigner, signerDesignator] = accounts
     const HASH = web3.sha3('hash') // careful as it may encode the data in the same way as solidity before hashing
 
+    const SIGNATURE_MODES = {
+      Invalid: '0x00',
+      EIP712:  '0x01',
+      EthSign: '0x02',
+      ERC1271: '0x03',
+      NMode:   '0x04',
+    }
+
     const ERC1271_RETURN_VALID_SIGNATURE = '0x20c13b0b'
     const ERC1271_RETURN_INVALID_SIGNATURE = '0x00000000'
 
@@ -314,24 +322,22 @@ contract('Agent app', (accounts) => {
       const signatureTests = [
         {
           name: 'EIP712',
-          mode: 1,
           signFunction: eip712Sign,
           signer: '0x93070b307c373D7f9344859E909e3EEeF6E4Fd5a',
           signerOrKey: '11bc31e7fef59610dfd6f95d2f78d2396c7b5477e4a9a54d72d9c1b76930e5c1',
           notSignerOrKey: '7224b5bc510e01f75b10e3b6d6c903861ca91adb95a26406d1603e2d28a29e7f',
         },
         {
           name: 'EthSign',
-          mode: 2,
           signFunction: ethSign,
           signer: accounts[7],
           signerOrKey: accounts[7],
           notSignerOrKey: accounts[8]
         }
       ]
 
-      for (let { name, mode, signFunction, signer, signerOrKey, notSignerOrKey } of signatureTests) {
-        const sign = signFunctionGenerator(signFunction, mode)
+      for (let { name, signFunction, signer, signerOrKey, notSignerOrKey } of signatureTests) {
+        const sign = signFunctionGenerator(signFunction, SIGNATURE_MODES[name])
 
         context(`> Signature mode: ${name}`, () => {
           beforeEach(async () => {
@@ -365,15 +371,25 @@ contract('Agent app', (accounts) => {
         })
       }
 
-      context(`> Signature mode: Invalid`, () => {
+      context(`> Signature mode: invalid modes`, () => {
         const randomAccount = accounts[9]
 
         beforeEach(async () => {
           await agent.setDesignatedSigner(randomAccount, { from: signerDesignator })
         })
 
+        it('isValidSignature returns false to an invalid mode signature', async () => {
+          const invalidSignature = SIGNATURE_MODES.Invalid
+          assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
+        })
+
+        it('isValidSignature returns false to an unspecified mode signature', async () => {
+          const unspecifiedSignature = SIGNATURE_MODES.NMode
+          assertIsValidSignature(false, await agent.isValidSignature(HASH, unspecifiedSignature))
+        })
+
         it('isValidSignature returns true to an invalid signature iff the hash was presigned', async () => {
-          const invalidSignature = "0x00"
+          const invalidSignature = SIGNATURE_MODES.Invalid
           assertIsValidSignature(false, await agent.isValidSignature(HASH, invalidSignature))
 
           // Now presign it
@@ -384,6 +400,8 @@ contract('Agent app', (accounts) => {
     })
 
     context('> Designated signer: contracts', () => {
+      const ERC1271_SIG = SIGNATURE_MODES.ERC1271
+
       const setDesignatedSignerContract = async (...params) => {
         const designatedSigner = await DesignatedSigner.new(...params)
         return agent.setDesignatedSigner(designatedSigner.address, { from: signerDesignator })
@@ -396,7 +414,7 @@ contract('Agent app', (accounts) => {
         // false - doesn't modify state on checking sig
         await setDesignatedSignerContract(true, true, false, false)
 
-        assertIsValidSignature(true, await agent.isValidSignature(HASH, NO_SIG))
+        assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
       })
 
       it('isValidSignature returns false if designated signer returns false', async () => {
@@ -407,18 +425,18 @@ contract('Agent app', (accounts) => {
         await setDesignatedSignerContract(true, false, false, false)
 
         // Signature fails check
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
       })
 
-      it('isValidSignature returns false if designated signer doesnt support the interface', async () => {
+      it('isValidSignature returns true even if the designated signer doesnt support the interface', async () => {
         // false - not ERC165 interface compliant
         // true  - any sigs are valid
         // false - doesn't revert on checking sig
         // false - doesn't modify state on checking sig
         await setDesignatedSignerContract(false, true, false, false)
 
         // Requires ERC165 compliance before checking isValidSignature
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        assertIsValidSignature(true, await agent.isValidSignature(HASH, ERC1271_SIG))
       })
 
       it('isValidSignature returns false if designated signer reverts', async () => {
@@ -429,7 +447,7 @@ contract('Agent app', (accounts) => {
         await setDesignatedSignerContract(true, true, true, false)
 
         // Reverts on checking
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
       })
 
       it('isValidSignature returns false if designated signer attempts to modify state', async () => {
@@ -440,7 +458,7 @@ contract('Agent app', (accounts) => {
         await setDesignatedSignerContract(true, true, false, true)
 
         // Checking costs too much gas
-        assertIsValidSignature(false, await agent.isValidSignature(HASH, NO_SIG))
+        assertIsValidSignature(false, await agent.isValidSignature(HASH, ERC1271_SIG))
       })
     })
   })