From 09f83937b5131665e1d50715c261689349ff6ff1 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 10:31:23 -0700 Subject: [PATCH 1/6] support non-standard signature length as part of ERC-1271 --- .../ConsiderationEventsAndErrors.sol | 8 -- contracts/lib/ConsiderationInternalView.sol | 116 ++++++++++-------- 2 files changed, 68 insertions(+), 56 deletions(-) diff --git a/contracts/interfaces/ConsiderationEventsAndErrors.sol b/contracts/interfaces/ConsiderationEventsAndErrors.sol index 3ad131960..67e9cd775 100644 --- a/contracts/interfaces/ConsiderationEventsAndErrors.sol +++ b/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. diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index 716860904..c16ad5f58 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/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 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,22 @@ 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); + } } 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); } // Attempt to recover signer using the digest and signature parameters. @@ -219,41 +221,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); + } + } + + /** + * @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 + ) + ); - // If the call fails... - if (!success) { - // Revert and pass reason along if one was returned. - _revertWithReasonIfOneIsReturned(); + // If the call fails... + if (!success) { + // Revert and pass reason along if one was returned. + _revertWithReasonIfOneIsReturned(); - // Otherwise, revert with a generic error message. - revert BadContractSignature(); - } + // 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) - - // Take value from scratch space and place it on the stack. - result := mload(0) - } - } + // 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(); + } } /** From 5202b525a17e31290a13e7f9e690dfa4b4ac47e4 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 10:34:37 -0700 Subject: [PATCH 2/6] update README --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 0613e6c43..dc44c5947 100644 --- a/README.md +++ b/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 From 2de144e9462dd5d623f818e3e8f99719de0b895a Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 10:41:40 -0700 Subject: [PATCH 3/6] fix typo in comment --- contracts/lib/ConsiderationInternalView.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index c16ad5f58..51af3a881 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -144,7 +144,7 @@ contract ConsiderationInternalView is ConsiderationPure { /** * @dev Internal view function to verify the signature of an order. An * ERC-1271 fallback will be attempted if either the signature length - * is not 32 or 33 bytes or if the recovered signer not match the + * 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. From dede8d659a26681dd3c3942a0a2f1d0789481b76 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 10:45:41 -0700 Subject: [PATCH 4/6] do not attempt to verify the signature twice for non-standard signature lengths --- contracts/lib/ConsiderationInternalView.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index 51af3a881..878e01508 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -210,6 +210,9 @@ contract ConsiderationInternalView is ConsiderationPure { } else { // 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. From 8b71d27ae14bd29b77110ced8b1f1535680fef23 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 10:47:23 -0700 Subject: [PATCH 5/6] add another clarifying comment --- contracts/lib/ConsiderationInternalView.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index 878e01508..bf9ba3af7 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -207,6 +207,7 @@ contract ConsiderationInternalView is ConsiderationPure { if (v != 27 && v != 28) { revert BadSignatureV(v); } + // For all other signature lengths, attempt verification using EIP-1271. } else { // Attempt EIP-1271 static call to offerer in case it's a contract. _verifySignatureViaERC1271(offerer, digest, signature); From d0704a2c87b2385ab7a71f12b54fbb68444f84c7 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 11:00:39 -0700 Subject: [PATCH 6/6] add test to get back to 100% coverage --- contracts/test/EIP1271Wallet.sol | 14 ++++ test/index.js | 117 +++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/contracts/test/EIP1271Wallet.sol b/contracts/test/EIP1271Wallet.sol index 41c3cbd7f..d1a113aac 100644 --- a/contracts/test/EIP1271Wallet.sol +++ b/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; diff --git a/test/index.js b/test/index.js index 044b998ea..a722413e4 100644 --- a/test/index.js +++ b/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());