Merge pull request #59 from ProjectOpenSea/1271-compatibility-extension

1271 compatibility extension

README.md

@@ -7,7 +7,7 @@ Consideration is a marketplace contract for safely and efficiently creating and
 ## Order
 
 Each order contains nine key components:
--   The `offerer` of the order supplies all offered items and must either fulfill the order personally (i.e. `msg.sender == offerer`) or approve the order via ECDSA signature or by listing the order on-chain (i.e. calling `validate`).
+-   The `offerer` of the order supplies all offered items and must either fulfill the order personally (i.e. `msg.sender == offerer`) or approve the order via signature (either standard 65-byte EDCSA, 64-byte EIP-2098, or an EIP-1271 `isValidSignature` check) or by listing the order on-chain (i.e. calling `validate`).
 - The `zone` of the order is an optional secondary account attached to the order with two additional privileges:
    - The zone may cancel orders where it is named as the zone, either for specific orders (by calling `cancel`) or for a whole category of orders (by calling `incrementNonce`).
    - Only the zone or the offerer can fulfill "restricted" orders if specified by the order type.
@@ -158,7 +158,6 @@ When matching a group of orders via `matchOrders` or `matchAdvancedOrders`, step
 The following features are good candidates for investigation and potential integration:
 - **Support for "tipping" on basic and/or standard order fulfillment.** While currently supported via match orders, this would make single order fulfillments more efficient and straightforward while still enabling referral / relayer fees to be paid out. This feature would allow the fulfiller to specify an optional, arbitrary array of consideration items alongside the fulfilled order, and could be accomplished via either an additional argument on existing functions or additional functions that leverage much of the existing logic and perform the additional transfers once all items specified by the fulfilled order have been transferred.
 - **Native support for attempting fulfillment for a group of orders without requiring that every order fulfillment succeeds.** One potential solution would be to implement a `fulfillAvailableOrders` function that iterates over the supplied orders and attempts to transfer all offer items for each order to the fulfiller; if a particular transfer fails, all prior offer item transfers and order status updates for that order would be rolled back and the order would be excluded from the group. Then, remaining consideration items can be aggregated by item type + token + identifier + recipient and transferred from the fulfiller to the recipient.
-- **Improved ERC-1271 compatibility.** Currently, Consideration only allows a single 64 or 65 byte ECDSA signature. Enabling support for a wider range of signature schemes, such as multiple signatures or additional data, will enable a number of interesting use-cases when making offers from ERC-1271-compliant contracts.
 - **General gas efficiency and code size optimizations.**
 
 ## Usage

contracts/interfaces/ConsiderationEventsAndErrors.sol

@@ -121,14 +121,6 @@ interface ConsiderationEventsAndErrors {
      */
     error FulfilledOrderConsiderationIndexOutOfRange();
 
-    /**
-     * @dev Revert with an error when a signature that is not either 64 bytes or
-     *      65 bytes in length has been supplied.
-     *
-     * @param signatureLength The invalid signature length.
-     */
-    error BadSignatureLength(uint256 signatureLength);
-
     /**
      * @dev Revert with an error when a signature that does not contain a v
      *      value of 27 or 28 has been supplied.

contracts/lib/ConsiderationInternalView.sol

@@ -143,9 +143,11 @@ contract ConsiderationInternalView is ConsiderationPure {
 
     /**
      * @dev Internal view function to verify the signature of an order. An
-     *      ERC-1271 fallback will be attempted should the recovered signature
-     *      not match the supplied offerer. Note that only 32-byte or 33-byte
-     *      ECDSA signatures are supported.
+     *      ERC-1271 fallback will be attempted if either the signature length
+     *      is not 32 or 33 bytes or if the recovered signer does not match the
+     *      supplied offerer. Note that in cases where a 32 or 33 byte signature
+     *      is supplied, only standard ECDSA signatures that recover to a
+     *      non-zero address are supported.
      *
      * @param offerer   The offerer for the order.
      * @param orderHash The order hash.
@@ -170,21 +172,8 @@ contract ConsiderationInternalView is ConsiderationPure {
         bytes32 s;
         uint8 v;
 
-        // If signature contains 65 bytes, parse as standard signature. (r+s+v)
-        if (signature.length == 65) {
-            // Read each parameter directly from the signature's memory region.
-            assembly {
-                r := mload(add(signature, 0x20)) // Put first word on stack at r
-                s := mload(add(signature, 0x40)) // Put next word on stack at s
-                v := byte(0, mload(add(signature, 0x60))) // Put last byte at v
-            }
-
-            // Ensure v value is properly formatted.
-            if (v != 27 && v != 28) {
-                revert BadSignatureV(v);
-            }
         // If signature contains 64 bytes, parse as EIP-2098 signature. (r+s&v)
-        } else if (signature.length == 64) {
+        if (signature.length == 64) {
             // Declare temporary vs that will be decomposed into s and v.
             bytes32 vs;
 
@@ -205,9 +194,26 @@ contract ConsiderationInternalView is ConsiderationPure {
                 // Extract yParity from highest bit of vs and add 27 to get v.
                 v := add(shr(255, vs), 27)
             }
+        // If signature contains 65 bytes, parse as standard signature. (r+s+v)
+        } else if (signature.length == 65) {
+            // Read each parameter directly from the signature's memory region.
+            assembly {
+                r := mload(add(signature, 0x20)) // Put first word on stack at r
+                s := mload(add(signature, 0x40)) // Put next word on stack at s
+                v := byte(0, mload(add(signature, 0x60))) // Put last byte at v
+            }
+
+            // Ensure v value is properly formatted.
+            if (v != 27 && v != 28) {
+                revert BadSignatureV(v);
+            }
+        // For all other signature lengths, attempt verification using EIP-1271.
         } else {
-            // Disallow signatures that are not 64 or 65 bytes long.
-            revert BadSignatureLength(signature.length);
+            // Attempt EIP-1271 static call to offerer in case it's a contract.
+            _verifySignatureViaERC1271(offerer, digest, signature);
+
+            // Return early if the ERC-1271 signature check succeeded.
+            return;
         }
 
         // Attempt to recover signer using the digest and signature parameters.
@@ -219,41 +225,59 @@ contract ConsiderationInternalView is ConsiderationPure {
         // Should a signer be recovered, but it doesn't match the offerer...
         } else if (signer != offerer) {
             // Attempt EIP-1271 static call to offerer in case it's a contract.
-            (bool success, ) = offerer.staticcall(
-                abi.encodeWithSelector(
-                    EIP1271Interface.isValidSignature.selector,
-                    digest,
-                    signature
-                )
-            );
+            _verifySignatureViaERC1271(offerer, digest, signature);
+        }
+    }
 
-            // If the call fails...
-            if (!success) {
-                // Revert and pass reason along if one was returned.
-                _revertWithReasonIfOneIsReturned();
+    /**
+     * @dev Internal view function to verify the signature of an order using
+     *      ERC-1271 (i.e. contract signatures via `isValidSignature`).
+     *
+     * @param offerer   The offerer for the order.
+     * @param digest    The signature digest, derived from the domain separator
+     *                  and the order hash.
+     * @param signature A signature (or other data) used to validate the digest.
+     */
+    function _verifySignatureViaERC1271(
+        address offerer,
+        bytes32 digest,
+        bytes memory signature
+    ) internal view {
+        // Attempt an EIP-1271 static call to the offerer.
+        (bool success, ) = offerer.staticcall(
+            abi.encodeWithSelector(
+                EIP1271Interface.isValidSignature.selector,
+                digest,
+                signature
+            )
+        );
 
-                // Otherwise, revert with a generic error message.
-                revert BadContractSignature();
-            }
+        // If the call fails...
+        if (!success) {
+            // Revert and pass reason along if one was returned.
+            _revertWithReasonIfOneIsReturned();
 
-            // Extract result from returndata buffer in case of memory overflow.
-            bytes4 result;
-            assembly {
-                // Only put result on stack if return data is exactly 32 bytes.
-                if eq(returndatasize(), 0x20) {
-                    // Copy directly from return data into scratch space.
-                    returndatacopy(0, 0, 0x20)
-
-                    // Take value from scratch space and place it on the stack.
-                    result := mload(0)
-                }
-            }
+            // Otherwise, revert with a generic error message.
+            revert BadContractSignature();
+        }
+
+        // Extract result from returndata buffer in case of memory overflow.
+        bytes4 result;
+        assembly {
+            // Only put result on stack if return data is exactly 32 bytes.
+            if eq(returndatasize(), 0x20) {
+                // Copy directly from return data into scratch space.
+                returndatacopy(0, 0, 0x20)
 
-            // Ensure result was extracted and matches EIP-1271 magic value.
-            if (result != EIP1271Interface.isValidSignature.selector) {
-                revert InvalidSigner();
+                // Take value from scratch space and place it on the stack.
+                result := mload(0)
             }
         }
+
+        // Ensure result was extracted and matches EIP-1271 magic value.
+        if (result != EIP1271Interface.isValidSignature.selector) {
+            revert InvalidSigner();
+        }
     }
 
     /**

contracts/test/EIP1271Wallet.sol

@@ -16,6 +16,8 @@ contract EIP1271Wallet {
 
     bool public showRevertMessage;
 
+    mapping (bytes32 => bool) public digestApproved;
+
     constructor(address _owner) {
         owner = _owner;
         showRevertMessage = true;
@@ -25,6 +27,10 @@ contract EIP1271Wallet {
         showRevertMessage = showMessage;
     }
 
+    function registerDigest(bytes32 digest, bool approved) external {
+        digestApproved[digest] = approved;
+    }
+
     function approveERC20(ERC20ApprovalInterface token, address operator, uint256 amount) external {
         if (msg.sender != owner) {
             revert ("Only owner");
@@ -45,6 +51,14 @@ contract EIP1271Wallet {
         bytes32 digest,
         bytes memory signature
     ) external view returns (bytes4) {
+        if (digestApproved[digest]) {
+            return _EIP_1271_MAGIC_VALUE;
+        }
+
+        if (signature.length != 65) {
+            revert();
+        }
+
         bytes32 r;
         bytes32 s;
         uint8 v;

test/index.js

@@ -2634,6 +2634,123 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function
             });
           });
         });
+        it("ERC721 <=> ERC20 (basic, EIP-1271 signature w/ non-standard length)", async () => {
+          // Seller mints nft to contract
+          const nftId = ethers.BigNumber.from(randomHex());
+          await testERC721.mint(sellerContract.address, nftId);
+
+          // Seller approves marketplace contract to transfer NFT
+          await whileImpersonating(seller.address, provider, async () => {
+            await expect(sellerContract.connect(seller).approveNFT(testERC721.address, marketplaceContract.address))
+              .to.emit(testERC721, "ApprovalForAll")
+              .withArgs(sellerContract.address, marketplaceContract.address, true);
+          });
+
+          // Buyer mints ERC20
+          const tokenAmount = ethers.BigNumber.from(randomLarge()).add(100);
+          await testERC20.mint(buyer.address, tokenAmount);
+
+          // Buyer approves marketplace contract to transfer tokens
+          await whileImpersonating(buyer.address, provider, async () => {
+            await expect(testERC20.connect(buyer).approve(marketplaceContract.address, tokenAmount))
+              .to.emit(testERC20, "Approval")
+              .withArgs(buyer.address, marketplaceContract.address, tokenAmount);
+          });
+
+          const offer = [
+            {
+              itemType: 2, // ERC721
+              token: testERC721.address,
+              identifierOrCriteria: nftId,
+              startAmount: ethers.BigNumber.from(1),
+              endAmount: ethers.BigNumber.from(1),
+            },
+          ];
+
+          const consideration = [
+            {
+              itemType: 1, // ERC20
+              token: testERC20.address,
+              identifierOrCriteria: 0, // ignored for ERC20
+              startAmount: tokenAmount.sub(100),
+              endAmount: tokenAmount.sub(100),
+              recipient: sellerContract.address,
+            },
+            {
+              itemType: 1, // ERC20
+              token: testERC20.address,
+              identifierOrCriteria: 0, // ignored for ERC20
+              startAmount: ethers.BigNumber.from(50),
+              endAmount: ethers.BigNumber.from(50),
+              recipient: zone.address,
+            },
+            {
+              itemType: 1, // ERC20
+              token: testERC20.address,
+              identifierOrCriteria: 0, // ignored for ERC20
+              startAmount: ethers.BigNumber.from(50),
+              endAmount: ethers.BigNumber.from(50),
+              recipient: owner.address,
+            },
+          ];
+
+          const { order, orderHash, value } = await createOrder(
+            sellerContract,
+            zone,
+            offer,
+            consideration,
+            0, // FULL_OPEN
+            [],
+            null,
+            seller
+          );
+
+          const basicOrderParameters = {
+            offerer: order.parameters.offerer,
+            zone: order.parameters.zone,
+            orderType: order.parameters.orderType,
+            token: order.parameters.offer[0].token,
+            identifier: order.parameters.offer[0].identifierOrCriteria,
+            startTime: order.parameters.startTime,
+            endTime: order.parameters.endTime,
+            salt: order.parameters.salt,
+            signature: "0x", // EMPTY
+            additionalRecipients: [
+              {
+                amount: ethers.BigNumber.from(50),
+                recipient: zone.address,
+              },
+              {
+                amount: ethers.BigNumber.from(50),
+                recipient: owner.address,
+              }
+            ],
+          };
+
+          // Fails before seller contract approves the digest
+          await whileImpersonating(buyer.address, provider, async () => {
+            await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(testERC20.address, tokenAmount.sub(100), basicOrderParameters)).to.be.reverted;
+          });
+
+          // Compute the digest based on the order hash
+          const domainSeparator = await marketplaceContract.DOMAIN_SEPARATOR();
+          const digest = ethers.utils.keccak256(`0x1901${domainSeparator.slice(2)}${orderHash.slice(2)}`);
+
+          // Seller approves the digest
+          await whileImpersonating(seller.address, provider, async () => {
+            await sellerContract.connect(seller).registerDigest(digest, true);
+          });
+
+          // Now it succeeds
+          await whileImpersonating(buyer.address, provider, async () => {
+            await withBalanceChecks([order], 0, null, async () => {
+              const tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(testERC20.address, tokenAmount.sub(100), basicOrderParameters);
+              const receipt = await tx.wait();
+              await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]);
+              return receipt;
+            });
+          });
+        });
         it("ERC721 <=> ERC20 (match)", async () => {
           // Seller mints nft
           const nftId = ethers.BigNumber.from(randomHex());