[hebao] Update SignatureUtil and ERC1271 support (#1741)

dong77 · kongliangzhong · web-flow · commit aedd3dadac7c · 2020-09-15T22:21:52.000+08:00
* Update SignatureUtil.sol * fix compile errors * fix ERC1271Module and test * fix a bug in SignatureUtil.sol * ERC1271Module.sol refine * Improve signature check dong (#1743) * remove support of legacy erc1271 * remove support of legacy erc1271 * remove support of legacy erc1271 * fix test Co-authored-by: kongliangzhong <kongliangzhong@gmail.com>
diff --git a/packages/hebao_v1/contracts/lib/ERC1271.sol b/packages/hebao_v1/contracts/lib/ERC1271.sol
@@ -3,28 +3,8 @@
 pragma solidity ^0.7.0;
 
 abstract contract ERC1271 {
-
-    // bytes4(keccak256("isValidSignature(bytes,bytes)")
-    bytes4 constant internal ERC1271_MAGICVALUE_BS = 0x20c13b0b;
-
     // bytes4(keccak256("isValidSignature(bytes32,bytes)")
-    bytes4 constant internal ERC1271_MAGICVALUE_B32 = 0x1626ba7e;
-
-    bytes4 constant internal ERC1271_SELECTOR_BS = bytes4(
-        keccak256(bytes("isValidSignature(bytes,bytes)"))
-    );
-
-    bytes4 constant internal ERC1271_SELECTOR_B32 = bytes4(
-        keccak256(bytes("isValidSignature(bytes32,bytes)"))
-    );
-
-    function isValidSignature(
-        bytes memory _data,
-        bytes memory _signature)
-        public
-        view
-        virtual
-        returns (bytes4 magicValueBS);
+    bytes4 constant internal ERC1271_MAGICVALUE = 0x1626ba7e;
 
     function isValidSignature(
         bytes32      _hash,
diff --git a/packages/hebao_v1/contracts/lib/SignatureUtil.sol b/packages/hebao_v1/contracts/lib/SignatureUtil.sol
@@ -5,15 +5,14 @@ pragma experimental ABIEncoderV2;
 
 import "../thirdparty/BytesUtil.sol";
 import "./AddressUtil.sol";
+import "./ERC1271.sol";
 import "./MathUint.sol";
 
 
 /// @title SignatureUtil
 /// @author Daniel Wang - <daniel@loopring.org>
-/// @dev This method supports multihash standard. Each signature's first byte indicates
-///      the signature's type, the second byte indicates the signature's length, therefore,
-///      each signature will have 2 extra bytes prefix. Mulitple signatures are concatenated
-///      together.
+/// @dev This method supports multihash standard. Each signature's last byte indicates
+///      the signature's type.
 library SignatureUtil
 {
     using BytesUtil     for bytes;
@@ -28,16 +27,7 @@ library SignatureUtil
         WALLET   // deprecated
     }
 
-    bytes4 constant internal ERC1271_MAGICVALUE_BS = 0x20c13b0b;
-    bytes4 constant internal ERC1271_MAGICVALUE_B32 = 0x1626ba7e;
-
-    bytes4 constant internal ERC1271_SELECTOR_BS = bytes4(
-        keccak256(bytes("isValidSignature(bytes,bytes)"))
-    );
-
-    bytes4 constant internal ERC1271_SELECTOR_B32 = bytes4(
-        keccak256(bytes("isValidSignature(bytes32,bytes)"))
-    );
+    bytes4 constant internal ERC1271_MAGICVALUE = 0x1626ba7e;
 
     function verifySignatures(
         bytes32          signHash,
@@ -47,45 +37,19 @@ library SignatureUtil
         internal
         view
         returns (bool)
-    {
-        return verifySignatures(abi.encodePacked(signHash), signers, signatures);
-    }
-
-    function verifySignatures(
-        bytes     memory data,
-        address[] memory signers,
-        bytes[]   memory signatures
-        )
-        internal
-        view
-        returns (bool)
     {
         require(signers.length == signatures.length, "BAD_SIGNATURE_DATA");
         address lastSigner;
         for (uint i = 0; i < signers.length; i++) {
             require(signers[i] > lastSigner, "INVALID_SIGNERS_ORDER");
             lastSigner = signers[i];
-            if (!verifySignature(data, signers[i], signatures[i])) {
+            if (!verifySignature(signHash, signers[i], signatures[i])) {
                 return false;
             }
         }
         return true;
     }
 
-    function verifySignature(
-        bytes   memory data,
-        address        signer,
-        bytes   memory signature
-        )
-        internal
-        view
-        returns (bool)
-    {
-        return signer.isContract() ?
-            verifyERC1271Signature(data, signer, signature) :
-            verifyEOASignature(data, signer, signature);
-    }
-
     function verifySignature(
         bytes32        signHash,
         address        signer,
@@ -95,7 +59,13 @@ library SignatureUtil
         view
         returns (bool)
     {
-        return verifySignature(abi.encodePacked(signHash), signer, signature);
+        if (signer == address(0)) {
+            return false;
+        }
+
+        return signer.isContract()?
+            verifyERC1271Signature(signHash, signer, signature):
+            verifyEOASignature(signHash, signer, signature);
     }
 
     function recoverECDSASigner(
@@ -132,28 +102,14 @@ library SignatureUtil
         }
     }
 
-    function recoverECDSASigner(
-        bytes memory data,
-        bytes memory signature
-        )
-        internal
-        pure
-        returns (address addr1, address addr2)
-    {
-        if (data.length == 32) {
-            addr1 = recoverECDSASigner(data.toBytes32(0), signature);
-        }
-        addr2 = recoverECDSASigner(keccak256(data), signature);
-    }
-
     function verifyEOASignature(
-        bytes   memory data,
+        bytes32        signHash,
         address        signer,
         bytes   memory signature
         )
         private
         pure
-        returns (bool)
+        returns (bool success)
     {
         if (signer == address(0)) {
             return false;
@@ -162,67 +118,32 @@ library SignatureUtil
         uint signatureTypeOffset = signature.length.sub(1);
         SignatureType signatureType = SignatureType(signature.toUint8(signatureTypeOffset));
 
-        bytes memory stripped = signature.slice(0, signatureTypeOffset);
+        // Strip off the last byte of the signature by updating the length
+        assembly {
+            mstore(signature, signatureTypeOffset)
+        }
 
         if (signatureType == SignatureType.EIP_712) {
-            (address addr1, address addr2) = recoverECDSASigner(data, stripped);
-            return addr1 == signer || addr2 == signer;
+            success = (signer == recoverECDSASigner(signHash, signature));
         } else if (signatureType == SignatureType.ETH_SIGN) {
-            if (data.length == 32) {
-                bytes32 hash = keccak256(
-                    abi.encodePacked("\x19Ethereum Signed Message:\n32", data.toBytes32(0))
-                );
-                if (recoverECDSASigner(hash, stripped) == signer) {
-                    return true;
-                }
-            }
             bytes32 hash = keccak256(
-                abi.encodePacked("\x19Ethereum Signed Message:\n32", keccak256(data))
+                abi.encodePacked("\x19Ethereum Signed Message:\n32", signHash)
             );
-            return recoverECDSASigner(hash, stripped) == signer;
+            success = (signer == recoverECDSASigner(hash, signature));
         } else {
-            return false;
+            success = false;
         }
-    }
 
-    function verifyERC1271Signature(
-        bytes   memory data,
-        address signer,
-        bytes   memory signature
-        )
-        private
-        view
-        returns (bool)
-    {
-        return data.length == 32 &&
-            verifyERC1271WithBytes32(data.toBytes32(0), signer, signature) ||
-            verifyERC1271WithBytes(data, signer, signature);
-    }
+        // Restore the signature length
+        assembly {
+            mstore(signature, add(signatureTypeOffset, 1))
+        }
 
-    function verifyERC1271WithBytes(
-        bytes   memory data,
-        address signer,
-        bytes   memory signature
-        )
-        private
-        view
-        returns (bool)
-    {
-        bytes memory callData = abi.encodeWithSelector(
-            ERC1271_SELECTOR_BS,
-            data,
-            signature
-        );
-        (bool success, bytes memory result) = signer.staticcall(callData);
-        return (
-            success &&
-            result.length == 32 &&
-            result.toBytes4(0) == ERC1271_MAGICVALUE_BS
-        );
+        return success;
     }
 
-    function verifyERC1271WithBytes32(
-        bytes32 hash,
+    function verifyERC1271Signature(
+        bytes32 signHash,
         address signer,
         bytes   memory signature
         )
@@ -231,15 +152,15 @@ library SignatureUtil
         returns (bool)
     {
         bytes memory callData = abi.encodeWithSelector(
-            ERC1271_SELECTOR_B32,
-            hash,
+            ERC1271.isValidSignature.selector,
+            signHash,
             signature
         );
         (bool success, bytes memory result) = signer.staticcall(callData);
         return (
             success &&
             result.length == 32 &&
-            result.toBytes4(0) == ERC1271_MAGICVALUE_B32
+            result.toBytes4(0) == ERC1271_MAGICVALUE
         );
     }
 }
diff --git a/packages/hebao_v1/contracts/modules/OfficialGuardian.sol b/packages/hebao_v1/contracts/modules/OfficialGuardian.sol
@@ -12,33 +12,21 @@ import "../thirdparty/BytesUtil.sol";
 /// @author Freeman Zhong - <kongliang@loopring.org>
 contract OfficialGuardian is OwnerManagable, ERC1271
 {
-    using SignatureUtil for bytes;
+    using SignatureUtil for bytes32;
     mapping (address => bool) public whitelist;
 
     function isValidSignature(
-        bytes32        _hash,
+        bytes32        _signHash,
         bytes   memory _signature
         )
         public
         view
         override
         returns (bytes4)
     {
-        (address addr1, address addr2) = abi.encodePacked(_hash).recoverECDSASigner(_signature);
-        return isManager(addr1) || isManager(addr2) ?  ERC1271_MAGICVALUE_B32 : bytes4(0);
-    }
-
-    function isValidSignature(
-        bytes memory _data,
-        bytes memory _signature
-        )
-        public
-        view
-        override
-        returns (bytes4)
-    {
-        (address addr1, address addr2) = _data.recoverECDSASigner(_signature);
-        return isManager(addr1) || isManager(addr2) ?  ERC1271_MAGICVALUE_BS : bytes4(0);
+        return isManager(_signHash.recoverECDSASigner(_signature))?
+            ERC1271_MAGICVALUE:
+            bytes4(0);
     }
 
     function addWhitelist(address target, bool toAdd)
diff --git a/packages/hebao_v1/contracts/modules/core/ERC1271Module.sol b/packages/hebao_v1/contracts/modules/core/ERC1271Module.sol
@@ -26,9 +26,8 @@ abstract contract ERC1271Module is ERC1271, BaseModule
         pure
         returns (bytes4[] memory methods)
     {
-        methods = new bytes4[](2);
-        methods[0] = ERC1271_SELECTOR_BS;
-        methods[1] = ERC1271_SELECTOR_B32;
+        methods = new bytes4[](1);
+        methods[0] = ERC1271.isValidSignature.selector;
     }
 
     // Will use msg.sender to detect the wallet, so this function should be called through
@@ -39,38 +38,24 @@ abstract contract ERC1271Module is ERC1271, BaseModule
     // The verificaiton of Wallet1's signature will succeed if the final EOA's signature is
     // valid.
     function isValidSignature(
-        bytes memory _data,
+        bytes32      _signHash,
         bytes memory _signature
         )
         public
         view
         override
-        returns (bytes4)
+        returns (bytes4 magicValue)
     {
         address wallet = msg.sender;
         (uint _lock,) = controller().securityStore().getLock(wallet);
         if (_lock > block.timestamp) { // wallet locked
             return 0;
         }
 
-        if (_data.verifySignature(Wallet(wallet).owner(), _signature)) {
-            return ERC1271_MAGICVALUE_BS;
+        if (_signHash.verifySignature(Wallet(wallet).owner(), _signature)) {
+            return ERC1271_MAGICVALUE;
         } else {
             return 0;
         }
     }
-
-    function isValidSignature(
-        bytes32      _hash,
-        bytes memory _signature)
-        public
-        view
-        override
-        returns (bytes4 magicValue)
-    {
-        magicValue = isValidSignature(abi.encodePacked(_hash), _signature);
-        if (magicValue == ERC1271_MAGICVALUE_BS) {
-            magicValue = ERC1271_MAGICVALUE_B32;
-        }
-    }
 }
diff --git a/packages/hebao_v1/test/testERC1271Module.ts b/packages/hebao_v1/test/testERC1271Module.ts
