From 6d3051d2d4f0ef8c43670d867a27060750534c83 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:39:08 -0500 Subject: [PATCH 01/39] Add BasicOrderParameters2 struct New struct contains all data needed to fulfill any basic order, used to ensure cd ptrs never change. Temporary name until all external fns converted. --- contracts/lib/ConsiderationStructs.sol | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index 05b9bc4b3..c7e05f94a 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -102,14 +102,36 @@ struct BasicOrderParameters { bytes signature; } +// todo: rename to BasicOrderParameters, delete old struct +// todo: remove cdptr comments +// todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem +struct BasicOrderParameters2 { + address receivedToken; // 0x24 + uint256 receivedIdentifier; // 0x44 + uint256 receivedAmount; // 0x64 + address payable offerer; // 0x84 + address offeredToken; // 0xa4 + uint256 offeredIdentifier; // 0xc4 + uint256 offeredAmount; // 0xe4 + OrderType orderType; // 0x104 + uint256 startTime; // 0x124 + uint256 endTime; // 0x144 + uint256 salt; // 0x164 + address zone; // 0x184 + bool useFulfillerProxy; // 0x1a4 + AdditionalRecipient[] additionalRecipients; // 0x1c4 + bytes signature; // 0x1e4 + // len : 0x204 +} + /** * @dev Basic orders can supply any number of additional recipients, with the * implied assumption that they are supplied from the offered ETH (or other * native token) or ERC20 token for the order. */ struct AdditionalRecipient { - address payable recipient; uint256 amount; + address payable recipient; } /** From 22784db6945297e1b83c56944bfb97ee3f600757 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:39:42 -0500 Subject: [PATCH 02/39] Add _assertValidBasicOrderParameterOffsets - fn to validate calldata pointers --- contracts/lib/ConsiderationInternalView.sol | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index 716860904..a40a0b194 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -66,6 +66,24 @@ contract ConsiderationInternalView is ConsiderationPure { } } + + /** + * @dev Validate BasicOrderParameters offset match the default abi encoding. + * This ensures that functions using the calldata object normally will be + * using the same data as the assembly functions. + */ + function _assertValidBasicOrderParameterOffsets() internal pure { + bool validOffsets; + // todo validate signature offset + assembly { + validOffsets := and( + eq(calldataload(0x04), 0x20), // Order parameters have offset of 0x20 + eq(calldataload(0x1c4), 0x1e0) // Additional recipients have offset of 0x1e0 + ) + } + if (!validOffsets) revert InvalidBasicOrderParameterEncoding(); + } + /** * @dev Internal view function to validate whether a token transfer was * successful based on the returned status and data. Note that From ff054d0cde9ffa11f7ec4ef58a00486b4a9eba9a Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:40:12 -0500 Subject: [PATCH 03/39] add InvalidBasicOrderParameterEncoding - error for invalid calldata pointers for basic orders --- contracts/interfaces/ConsiderationEventsAndErrors.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/interfaces/ConsiderationEventsAndErrors.sol b/contracts/interfaces/ConsiderationEventsAndErrors.sol index 3ad131960..4309dd4cb 100644 --- a/contracts/interfaces/ConsiderationEventsAndErrors.sol +++ b/contracts/interfaces/ConsiderationEventsAndErrors.sol @@ -370,4 +370,10 @@ interface ConsiderationEventsAndErrors { does not match the expected proxy implementation. */ error InvalidProxyImplementation(); + + /** + * @dev Revert with an error when attempting to fill a basic order using + * calldata not produced by default ABI encoding, i.e. suboptimal offsets. + */ + error InvalidBasicOrderParameterEncoding(); } From a2924ae068b1b3f8c2af79a81e36a1f9a4f197ea Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:41:19 -0500 Subject: [PATCH 04/39] Update some basic order fulfillment fn definitions --- .../interfaces/ConsiderationInterface.sol | 32 +++---------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/contracts/interfaces/ConsiderationInterface.sol b/contracts/interfaces/ConsiderationInterface.sol index 25d495cc4..cb191577b 100644 --- a/contracts/interfaces/ConsiderationInterface.sol +++ b/contracts/interfaces/ConsiderationInterface.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.12; import { BasicOrderParameters, + BasicOrderParameters2, OrderComponents, Fulfillment, Execution, @@ -26,6 +27,7 @@ import { * Consideration. */ interface ConsiderationInterface { + // todo: review basic order fn natspec /** * @notice Fulfill an order offering a single ERC721 token by supplying * Ether (or the native token for the given chain) as consideration @@ -33,10 +35,6 @@ interface ConsiderationInterface { * also be supplied which will each receive the native token from * the fulfiller as consideration. * - * @param etherAmount Ether (or the native token for the given chain) that - * will be transferred to the offerer of the fulfilled - * order. Note that msg.value must exceed this amount if - * additonal recipients are specified. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract (or * their proxy if indicated by the order) in order for @@ -45,8 +43,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC721Order( - uint256 etherAmount, - BasicOrderParameters calldata parameters + BasicOrderParameters2 calldata parameters ) external payable returns (bool); /** @@ -55,15 +52,6 @@ interface ConsiderationInterface { * order. An arbitrary number of "additional recipients" may also be * supplied which will each receive native tokens from the fulfiller * as consideration. - * - * @param etherAmount Ether (or the native token for the given chain) that - * will be transferred to the offerer of the fulfilled - * order. Note that msg.value must exceed this amount - * if additonal recipients are specified. - * @param erc1155Amount Total offererd ERC1155 tokens that will be - * transferred to the fulfiller. Also note that calling - * contracts must implement `onERC1155Received` in - * order to receive tokens. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract * (or their proxy if indicated by the order) in order @@ -72,9 +60,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC1155Order( - uint256 etherAmount, - uint256 erc1155Amount, - BasicOrderParameters calldata parameters + BasicOrderParameters2 calldata parameters ) external payable returns (bool); /** @@ -83,12 +69,6 @@ interface ConsiderationInterface { * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. * - * @param erc20Token The address of the ERC20 token being supplied as - * consideration to the offerer of the fulfilled order. - * @param erc20Amount ERC20 tokens that will be transferred to the offerer - * of the fulfilled order. Note that the fulfiller must - * first approve this contract before the ERC20 tokens - * required as consideration can be transferred. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract (or * their proxy if indicated by the order) in order for @@ -97,9 +77,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC721Order( - address erc20Token, - uint256 erc20Amount, - BasicOrderParameters calldata parameters + BasicOrderParameters2 calldata parameters ) external returns (bool); /** From 4fc1d8ba85d69d42acb7f6fe89a3798fbb08bcf0 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:45:09 -0500 Subject: [PATCH 05/39] Add _prepareBasicFulfillmentFromCalldata and modified versions of some internal fns that use new BasicOrderParameters struct Some fns are temporarily duplicated until all the external fns are updated to use the new struct. This version contains some outdated variable names and extraneous comments. There are also some optimizations that could be made in the secondary fns for basic orders which have not been made yet. --- contracts/lib/ConsiderationInternal.sol | 340 ++++++++++++++++++++++++ 1 file changed, 340 insertions(+) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index a05462867..1523877ef 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -17,6 +17,7 @@ import { import { AdditionalRecipient, BasicOrderParameters, + BasicOrderParameters2, OfferItem, ConsiderationItem, SpentItem, @@ -58,6 +59,224 @@ contract ConsiderationInternal is ConsiderationInternalView { requiredProxyImplementation ) {} + // todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem + // todo: clean up comments, add natspec + function _prepareBasicFulfillmentFromCalldata( + BasicOrderParameters2 calldata parameters, + ItemType receivedItemType, + ItemType additionalRecipientsItemType, + address additionalRecipientsToken, + ItemType offeredItemType + ) internal returns (bytes32 orderHash, bool useOffererProxy) { + // Ensure this function cannot be triggered during a reentrant call. + _setReentrancyGuard(); + // Ensure current timestamp falls between order start time and end time. + _assertValidTime(parameters.startTime, parameters.endTime); + // Ensure calldata offsets were produced by default encoding + _assertValidBasicOrderParameterOffsets(); + + /* + event OrderFulfilled( + bytes32 orderHash, + address indexed offerer, + address indexed zone, + address fulfiller, + ConsumedItem[] offer, (itemType, token, identifier, amount) + FulfilledItem[] consideration (itemType, token, identifier, amount, recipient) + ) + data + 0x00: orderHash + 0x20: fulfiller + 0x40: offer arr ptr (0x80) + 0x60: consideration arr ptr (0x120) + 0x80: offer arr len (1) + 0xa0: offer.itemType + 0xc0: offer.token + 0xe0: offer.identifier + 0x100: offer.amount + 0x120: 1 + recipients.length + 0x140: recipient 0 + */ + + + { // Handle received items + bytes32 typeHash = _CONSIDERATION_ITEM_TYPEHASH; + + assembly { + // todo: Update event ptr to avoid overwriting order data when additional recipients has length 0 + /* Memory Layout + * 0x60: hash of considerations array + * 0x80-0x160: scratch space for EIP712 hashing of considerations + * - 0x80: _RECEIVED_ITEM_TYPEHASH + * - 0xa0: itemType + * - 0xc0: token + * - 0xe0: identifier + * - 0x100: startAmount + * - 0x120: endAmount + * - 0x140: recipient + * 0x160-END_ARR: array of consideration hashes (END_ARR = 0x180 + ADDITIONAL_RECIPIENTS_LENGTH * 0x20) + * - 0x160: EIP712 hash of primary consideration + * - 0x180-END_ARR: EIP712 hashes of additional recipients considerations + * END_ARR: beginning of data for OrderFulfilled event + * END_ARR + 0x120: length of FulfilledItem array + * END_ARR + 0x140: beginning of data for first FulfilledItem + */ + /* 1. Write first ReceivedItem hash to order's considerations array */ + mstore(0x80, typeHash) + mstore(0xa0, receivedItemType) + calldatacopy(0xc0, 0x24, 0x60) // (token, identifier, startAmount) + calldatacopy(0x120, 0x64, 0x40) // (endAmount, recipient) + mstore(0x160, keccak256(0x80, 0xe0)) // receivedItemHashes[0] = keccak256(abi.encode(receivedItem)) + + /* 2. Write first FulfilledItem to OrderFulfilled data */ + let len := calldataload(0x204) + // END_ARR + 0x120 = 0x2a0 + len*0x20 + let eventArrPtr := add(0x2a0, mul(0x20, len)) + mstore(eventArrPtr, add(len, 1)) // length + // Set ptr to data portion of first FulfilledItem + eventArrPtr := add(eventArrPtr, 0x20) + mstore(eventArrPtr, receivedItemType) + calldatacopy(add(eventArrPtr, 0x20), 0x24, 0x80) // (token, identifier, amount, recipient) + + /* 3. Handle additional recipients */ + let considerationHashesPtr := 0x160 // ptr to current place in receivedItemHashes + // Update scratch space with (type, token, identifier) for additional recipients + mstore(0xa0, additionalRecipientsItemType) + mstore(0xc0, additionalRecipientsToken) + mstore(0xe0, 0) + for { + let i := 0 + } lt(i, len) { + i := add(i, 1) + } { + let additionalRecipientCdPtr := add(0x224, mul(0x40, i)) + /* a. Write ReceivedItem hash to order's considerations array */ + calldatacopy(0x100, additionalRecipientCdPtr, 0x20) // startAmount + calldatacopy(0x120, additionalRecipientCdPtr, 0x40) // endAmount, recipient + // Add 1 word to the pointer each loop to reduce ops needed to get local offset into the array + considerationHashesPtr := add(considerationHashesPtr, 0x20) + mstore(considerationHashesPtr, keccak256(0x80, 0xe0)) // receivedItemHashes[i + 1] = keccak256(abi.encode(receivedItem)) + + /* b. Write FulfilledItem to OrderFulfilled data */ + // At this point, eventArrPtr points to the beginning of the FulfilledItem struct + // for the previous element in the array. + eventArrPtr := add(eventArrPtr, 0xa0) + mstore(eventArrPtr, additionalRecipientsItemType) + mstore(add(eventArrPtr, 0x20), additionalRecipientsToken) + calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) // endAmount, recipient + } + // arrPtr := sub(arrPtr, mul(len, 0x20)) // Get back to original receivedItemHashes pointer + /* 4. Hash packed array of ReceivedItem EIP712 hashes */ + // note: Store receivedItemsHash at 0x60 - all other memory use begins at 0x80 + mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) // keccak256(abi.encodePacked(receivedItemHashes)) + } + } + + { // Handle offered items + /* Memory Layout + * EIP712 data for OfferedItem + * - 0x80: _OFFERED_ITEM_TYPEHASH + * - 0xa0: itemType + * - 0xc0: token + * - 0xe0: identifier (reused to store offered items array hash once calculated) + * - 0x100: startAmount + * - 0x120: endAmount + */ + bytes32 typeHash = _OFFER_ITEM_TYPEHASH; + assembly { + /* 1. Calculate OfferedItem EIP712 hash*/ + mstore(0x80, typeHash) // _OFFERED_ITEM_TYPEHASH + mstore(0xa0, offeredItemType) // itemType + calldatacopy(0xc0, 0xa4, 0x60) // (token, identifier, startAmount) + calldatacopy(0x120, 0xe4, 0x20) // endAmount + mstore(0x00, keccak256(0x80, 0xc0)) // keccak256(abi.encode(offeredItem)) + /* 2. Calculate hash of array of EIP712 hashes */ + // note: Write offeredItemsHash to offer struct + mstore(0xe0, keccak256(0x00, 0x20)) // keccak256(abi.encodePacked(offeredItemHashes)) + /* 3. Write ConsumedItem array to event data */ + // 0x180 + len*32 = event data ptr + // offers array length is stored at 0x80 into the event data + let eventArrPtr := add(0x200, mul(0x20, calldataload(0x204))) + mstore(eventArrPtr, 1) + mstore(add(eventArrPtr, 0x20), offeredItemType) + calldatacopy(add(eventArrPtr, 0x40), 0xa4, 0x60) // (token, identifier, startAmount) + } + } + { // Calculate order hash + address offerer; + address zone; + assembly { + offerer := calldataload(0x84) + zone := calldataload(0x184) + } + uint256 nonce = _nonces[offerer][zone]; + bytes32 typeHash = _ORDER_HASH; + assembly { + /* Memory Layout + * 0x80-0x1c0: EIP712 data for order + * - 0x80: _ORDER_HASH, + * - 0xa0: orderParameters.offerer, + * - 0xc0: orderParameters.zone, + * - 0xe0: keccak256(abi.encodePacked(offerHashes)), + * - 0x100: keccak256(abi.encodePacked(considerationHashes)), + * - 0x120: orderParameters.orderType, + * - 0x140: orderParameters.startTime, + * - 0x160: orderParameters.endTime, + * - 0x180: orderParameters.salt, + * - 0x1a0: nonce + */ + mstore(0x80, typeHash) + // todo: rearrange calldata for zone and offerer + calldatacopy(0xa0, 0x84, 0x20) // offerer + calldatacopy(0xc0, 0x184, 0x20) // zone + mstore(0x100, mload(0x60)) // pull receivedItemsHash out of scratch space + calldatacopy(0x120, 0x104, 0x80) // orderType, startTime, endTime, salt + mstore(0x1a0, nonce) // nonce + orderHash := keccak256(0x80, 0x140) + } + } + /* 0x00: orderHash + 0x20: fulfiller + 0x40: offer arr ptr (0x80) + 0x60: consideration arr ptr (0x120) + 0x80: offer arr len (1) + 0xa0: offer.itemType + 0xc0: offer.token + 0xe0: offer.identifier + 0x100: offer.amount + 0x120: 1 + recipients.length + 0x140: recipient 0 */ + { + // todo: Replace with constant hex + bytes32 sig = keccak256('OrderFulfilled(bytes32,address,address,address,(uint8,address,uint256,uint256)[],(uint8,address,uint256,uint256,address)[])'); + assembly { + let eventDataPtr := add(0x180, mul(0x20, calldataload(0x204))) + mstore(eventDataPtr, orderHash) // orderHash + mstore(add(eventDataPtr, 0x20), caller()) // fulfiller + mstore(add(eventDataPtr, 0x40), 0x80) // ConsumedItem array pointer + mstore(add(eventDataPtr, 0x60), 0x120) // FulfilledItem array pointer + let dataSize := add(0x1e0, mul(calldataload(0x204), 0xa0)) + log3(eventDataPtr, dataSize, sig, calldataload(0x84), calldataload(0x184) /* topic1, topic2 */) + /* Restore the zero slot */ + mstore(0x60, 0) + } + } + + // Verify and update the status of the derived order. + _validateBasicOrderAndUpdateStatus( + orderHash, + parameters.offerer, + parameters.signature + ); + + // Determine if a proxy should be utilized and ensure a valid submitter. + useOffererProxy = _determineProxyUtilizationAndEnsureValidSubmitter( + parameters.orderType, + parameters.offerer, + parameters.zone + ); + } + /** * @dev Internal function to derive and validate an order based on a set of * parameters and a primary item for offer and consideration. @@ -1189,6 +1408,68 @@ contract ConsiderationInternal is ConsiderationInternalView { (success, ) = target.call(callData); } + // todo: delete old version, add natspec, look into optimizations + function _transferEthAndFinalize( + uint256 amount, + BasicOrderParameters2 calldata parameters + ) internal { + // Put ether value supplied by the caller on the stack. + uint256 etherRemaining = msg.value; + + // Iterate over each additional recipient. + for (uint256 i = 0; i < parameters.additionalRecipients.length;) { + // Retrieve the additional recipient. + AdditionalRecipient calldata additionalRecipient = ( + parameters.additionalRecipients[i] + ); + + // Read ether amount to transfer to recipient and place on stack. + uint256 additionalRecipientAmount = additionalRecipient.amount; + + // Ensure that sufficient Ether is available. + if (additionalRecipientAmount > etherRemaining) { + revert InsufficientEtherSupplied(); + } + + // Transfer Ether to the additional recipient. + _transferEth( + additionalRecipient.recipient, + additionalRecipientAmount + ); + + // Skip underflow check as subtracted value is less than remaining. + unchecked { + // Reduce ether value available. + etherRemaining -= additionalRecipientAmount; + } + + // Skip overflow check as for loop is indexed starting at zero. + unchecked { + ++i; + } + } + + // Ensure that sufficient Ether is still available. + if (amount > etherRemaining) { + revert InsufficientEtherSupplied(); + } + + // Transfer Ether to the offerer. + _transferEth(parameters.offerer, amount); + + // If any Ether remains after transfers, return it to the caller. + if (etherRemaining > amount) { + // Skip underflow check as etherRemaining > amount. + unchecked { + // Transfer remaining Ether to the caller. + _transferEth(payable(msg.sender), etherRemaining - amount); + } + } + + // Clear the reentrancy guard. + _reentrancyGuard = _NOT_ENTERED; + } + /** * @dev Internal function to transfer Ether or other native tokens to a * given recipient. Note that proxies are not utilized for native token @@ -1258,6 +1539,64 @@ contract ConsiderationInternal is ConsiderationInternalView { _reentrancyGuard = _NOT_ENTERED; } + // todo: delete old version, add natspec, look into optimizations + /** + * @dev Internal function to transfer ERC20 tokens to a given recipient. + * Note that proxies are not utilized for ERC20 tokens. + * + * @param from The originator of the ERC20 token transfer. + * @param to The recipient of the ERC20 token transfer. + * @param erc20Token The ERC20 token to transfer. + * @param amount The amount of ERC20 tokens to transfer. + * @param parameters The parameters of the order. + * @param fromOfferer Whether to decrement amount from the offered amount. + */ + function _transferERC20AndFinalize( + address from, + address to, + address erc20Token, + uint256 amount, + BasicOrderParameters2 calldata parameters, + bool fromOfferer + ) internal { + // Iterate over each additional recipient. + for (uint256 i = 0; i < parameters.additionalRecipients.length;) { + // Retrieve the additional recipient. + AdditionalRecipient calldata additionalRecipient = ( + parameters.additionalRecipients[i] + ); + + // Decrement the amount to transfer to fulfiller if indicated. + if (fromOfferer) { + amount -= additionalRecipient.amount; + } + + // Transfer ERC20 tokens to additional recipient given approval. + _transferERC20( + erc20Token, + from, + additionalRecipient.recipient, + additionalRecipient.amount + ); + + // Skip overflow check as for loop is indexed starting at zero. + unchecked { + ++i; + } + } + + // Transfer ERC20 token amount (from account must have proper approval). + _transferERC20( + erc20Token, + from, + to, + amount + ); + + // Clear the reentrancy guard. + _reentrancyGuard = _NOT_ENTERED; + } + /** * @dev Internal function to transfer ERC20 tokens to a given recipient. * Note that proxies are not utilized for ERC20 tokens. @@ -1328,6 +1667,7 @@ contract ConsiderationInternal is ConsiderationInternalView { _reentrancyGuard = _ENTERED; } + // todo: delete function _emitOrderFulfilledEvent( bytes32 orderHash, address offerer, From 9d3d8ca7964da432401fd80267428fd17cb0cb8c Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:54:56 -0500 Subject: [PATCH 06/39] Modify some basic order fulfillment functions use new BasicOrderParameters struct and optimized internal fns --- contracts/Consideration.sol | 158 +++++++++++------------------------- 1 file changed, 46 insertions(+), 112 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index 07a0c5c8c..37617b2d1 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -9,6 +9,7 @@ import { ItemType } from "./lib/ConsiderationEnums.sol"; import { BasicOrderParameters, + BasicOrderParameters2, OfferItem, ConsiderationItem, OrderParameters, @@ -49,63 +50,33 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { address requiredProxyImplementation ) ConsiderationInternal(legacyProxyRegistry, requiredProxyImplementation) {} - /** - * @notice Fulfill an order offering a single ERC721 token by supplying - * Ether (or the native token for the given chain) as consideration - * for the order. An arbitrary number of "additional recipients" may - * also be supplied which will each receive the native token from - * the fulfiller as consideration. - * - * @param etherAmount Ether (or the native token for the given chain) that - * will be transferred to the offerer of the fulfilled - * order. Note that msg.value must exceed this amount if - * additonal recipients are specified. - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract (or - * their proxy if indicated by the order) in order for - * their offered ERC721 token to be transferred. - * - * @return A boolean indicating whether the order has been fulfilled. - */ function fulfillBasicEthForERC721Order( - uint256 etherAmount, - BasicOrderParameters memory parameters + BasicOrderParameters2 calldata parameters ) external payable override returns (bool) { + // todo: Look into whether it is necessary to do further validation on parameters + (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.NATIVE, + ItemType.NATIVE, + address(0), + ItemType.ERC721 + ); + // Move the offerer from memory to the stack. address payable offerer = parameters.offerer; - // Derive and validate order using parameters and update order status. - bool useOffererProxy = _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC721, - parameters.token, - parameters.identifier, - 1, // Amount of 1 for ERC721 tokens - 1 // Amount of 1 for ERC721 tokens - ), - ConsiderationItem( - ItemType.NATIVE, - address(0), // No token address for ETH or other native tokens - 0, // No identifier for ETH or other native tokens - etherAmount, - etherAmount, - offerer - ) - ); - // Transfer ERC721 to caller, using offerer's proxy if applicable. _transferERC721( - parameters.token, + parameters.offeredToken, offerer, msg.sender, - parameters.identifier, + parameters.offeredIdentifier, useOffererProxy ? offerer : address(0) ); // Transfer native to recipients, return excess to caller, and wrap up. _transferEthAndFinalize( - etherAmount, + parameters.receivedAmount, parameters ); @@ -119,14 +90,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * supplied which will each receive native tokens from the fulfiller * as consideration. * - * @param etherAmount Ether (or the native token for the given chain) that - * will be transferred to the offerer of the fulfilled - * order. Note that msg.value must exceed this amount - * if additonal recipients are specified. - * @param erc1155Amount Total offererd ERC1155 tokens that will be - * transferred to the fulfiller. Also note that calling - * contracts must implement `onERC1155Received` in - * order to receive tokens. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract * (or their proxy if indicated by the order) in order @@ -135,46 +98,33 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC1155Order( - uint256 etherAmount, - uint256 erc1155Amount, - BasicOrderParameters memory parameters + BasicOrderParameters2 calldata parameters ) external payable override returns (bool) { + // todo: Look into whether it is necessary to do further validation on parameters + (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.NATIVE, + ItemType.NATIVE, + address(0), + ItemType.ERC1155 + ); + // Move the offerer from memory to the stack. address payable offerer = parameters.offerer; - // Derive and validate order using parameters and update order status. - bool useOffererProxy = _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC1155, - parameters.token, - parameters.identifier, - erc1155Amount, - erc1155Amount - ), - ConsiderationItem( - ItemType.NATIVE, - address(0), // No token address for ETH or other native tokens - 0, // No identifier for ETH or other native tokens - etherAmount, - etherAmount, - offerer - ) - ); - // Transfer ERC1155 to caller, using offerer's proxy if applicable. _transferERC1155( - parameters.token, + parameters.offeredToken, offerer, msg.sender, - parameters.identifier, - erc1155Amount, + parameters.offeredIdentifier, + parameters.offeredAmount, useOffererProxy ? offerer : address(0) ); // Transfer native to recipients, return excess to caller, and wrap up. _transferEthAndFinalize( - etherAmount, + parameters.receivedAmount, parameters ); @@ -187,12 +137,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. * - * @param erc20Token The address of the ERC20 token being supplied as - * consideration to the offerer of the fulfilled order. - * @param erc20Amount ERC20 tokens that will be transferred to the offerer - * of the fulfilled order. Note that the fulfiller must - * first approve this contract before the ERC20 tokens - * required as consideration can be transferred. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract (or * their proxy if indicated by the order) in order for @@ -201,45 +145,35 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC721Order( - address erc20Token, - uint256 erc20Amount, - BasicOrderParameters memory parameters + BasicOrderParameters2 calldata parameters ) external override returns (bool) { - // Derive and validate order using parameters and update order status. - bool useOffererProxy = _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC721, - parameters.token, - parameters.identifier, - 1, // Amount of 1 for ERC721 tokens - 1 // Amount of 1 for ERC721 tokens - ), - ConsiderationItem( - ItemType.ERC20, - erc20Token, - 0, // No identifier for ERC20 tokens - erc20Amount, - erc20Amount, - parameters.offerer - ) + // todo: Look into whether it is necessary to do further validation on parameters + (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.ERC20, + ItemType.ERC20, + parameters.receivedToken, + ItemType.ERC721 ); + // Move the offerer from memory to the stack. + address payable offerer = parameters.offerer; + // Transfer ERC721 to caller, using offerer's proxy if applicable. _transferERC721( - parameters.token, - parameters.offerer, + parameters.offeredToken, + offerer, msg.sender, - parameters.identifier, - useOffererProxy ? parameters.offerer : address(0) + parameters.offeredIdentifier, + useOffererProxy ? offerer : address(0) ); // Transfer ERC20 tokens to all recipients and wrap up. _transferERC20AndFinalize( msg.sender, - parameters.offerer, - erc20Token, - erc20Amount, + offerer, + parameters.receivedToken, + parameters.receivedAmount, parameters, false // Transfer full amount indicated by all consideration items. ); From b7ef17e26eb064da0809fea3e49e3b72a37232ed Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 11:55:41 -0500 Subject: [PATCH 07/39] Modify tests for some basic order fns to use new basic order params struct and interface --- test/index.js | 516 ++++++++++++++++++++++---------------------------- 1 file changed, 225 insertions(+), 291 deletions(-) diff --git a/test/index.js b/test/index.js index 044b998ea..070ad24ce 100644 --- a/test/index.js +++ b/test/index.js @@ -40,6 +40,28 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function `0x${[...Array(60)].map(() => Math.floor(Math.random() * 16).toString(16)).join('')}` ); + const getBasicOrderParameters = ( + order, + additionalRecipients = [], + useFulfillerProxy = false + ) => ({ + offerer: order.parameters.offerer, + zone: order.parameters.zone, + orderType: order.parameters.orderType, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: order.parameters.offer[0].endAmount, + receivedToken: order.parameters.consideration[0].token, + receivedIdentifier: order.parameters.consideration[0].identifierOrCriteria, + receivedAmount: order.parameters.consideration[0].endAmount, + startTime: order.parameters.startTime, + endTime: order.parameters.endTime, + salt: order.parameters.salt, + signature: order.signature, + useFulfillerProxy, + additionalRecipients, + }); + const convertSignatureToEIP2098 = (signature) => { if (signature.length === 130) { return signature; @@ -1487,31 +1509,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, { value }); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -1575,7 +1586,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - const basicOrderParameters = { + /* const basicOrderParameters = { offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, @@ -1595,11 +1606,23 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function recipient: owner.address, } ], - }; + }; */ + + + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -1670,31 +1693,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function .withArgs(orderHash, seller.address, zone.address); }); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -1763,31 +1775,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function order.signature = convertSignatureToEIP2098(order.signature); expect(order.signature.length).to.equal(130); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -1851,31 +1852,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value: value.add(1)}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value: value.add(1)}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -2395,31 +2385,22 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]); + + // basicOrderParameters.receivedAmount = basicOrderParameters.receivedAmount.sub(100); 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 tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -2501,31 +2482,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]) 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 tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -2603,31 +2573,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]) 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 tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -3628,31 +3587,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(order.parameters.consideration[0].endAmount, order.parameters.offer[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -3717,31 +3665,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(order.parameters.consideration[0].endAmount, order.parameters.offer[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -8637,30 +8574,19 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function expect(orderStatus.totalFilled).to.equal(1); expect(orderStatus.totalSize).to.equal(2); - 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: order.signature, - additionalRecipients: [ - { - amount: amount.mul(10), - recipient: zone.address, - }, - { - amount: amount.mul(20), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(order.parameters.consideration[0].endAmount, order.parameters.offer[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(basicOrderParameters, {value})).to.be.reverted; }); }); it("Reverts on fully filled order", async () => { @@ -8913,12 +8839,17 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), @@ -8932,21 +8863,21 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); //construct an invalid signature basicOrderParameters.signature = '0x'.padEnd(130, 'f') + '1c'; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); basicOrderParameters.signature = originalSignature; await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -9024,30 +8955,19 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function zone // wrong signer ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(40), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(60), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(40), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(60), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(testERC20.address, tokenAmount.sub(100), basicOrderParameters)).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters)).to.be.reverted; }); }); it("Reverts on invalid 1271 signature and contract does not supply a revert reason", async () => { @@ -9126,30 +9046,19 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function zone // wrong signer ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]) await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(testERC20.address, tokenAmount.sub(100), basicOrderParameters)).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters)).to.be.reverted; }); }); it("Reverts on missing offer or consideration components", async () => { @@ -10263,12 +10172,17 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), @@ -10282,7 +10196,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); // upgrade back @@ -10300,7 +10214,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -10560,12 +10474,17 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), @@ -10579,7 +10498,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); }); it("Reverts on orders that have expired (basic)", async () => { @@ -10645,12 +10564,17 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), @@ -10664,7 +10588,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); }); it("Reverts on orders that have not started (match)", async () => { @@ -10881,12 +10805,17 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), @@ -10900,16 +10829,16 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value: ethers.BigNumber.from(1)})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value: ethers.BigNumber.from(1)})).to.be.reverted; }); await whileImpersonating(buyer.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value: value.sub(1)})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value: value.sub(1)})).to.be.reverted; }); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value}); + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value}); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -11915,16 +11844,21 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - token: order.parameters.offer[0].token, - identifier: order.parameters.offer[0].identifierOrCriteria, + offeredToken: order.parameters.offer[0].token, + offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, + offeredAmount: 1, + receivedToken: constants.AddressZero, + receivedIdentifier: constants.Zero, + receivedAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, signature: order.signature, + useFulfillerProxy: false, additionalRecipients: [ { amount: ethers.utils.parseEther("1"), - recipient: marketplaceContract.address, + recipient: zone.address, }, { amount: ethers.utils.parseEther("1"), @@ -11934,7 +11868,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }; await whileImpersonating(owner.address, provider, async () => { - await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(order.parameters.consideration[0].endAmount, basicOrderParameters, {value})).to.be.reverted; + await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; }); }); }) From 8fe8ecbb591e3060d3797a52d8d7e979e5dde903 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 12:32:50 -0500 Subject: [PATCH 08/39] Convert remaining basic order fns to new struct and fulfillment fn --- contracts/Consideration.sol | 182 ++++++++++----------------------- test/index.js | 198 ++++++++++++------------------------ 2 files changed, 120 insertions(+), 260 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index 37617b2d1..e5e16daa9 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -54,6 +54,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { BasicOrderParameters2 calldata parameters ) external payable override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters + // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, ItemType.NATIVE, @@ -101,6 +102,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { BasicOrderParameters2 calldata parameters ) external payable override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters + // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, ItemType.NATIVE, @@ -148,6 +150,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { BasicOrderParameters2 calldata parameters ) external override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters + // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, ItemType.ERC20, @@ -186,17 +189,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * tokens as consideration. An arbitrary number of "additional * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. - * - * @param erc20Token The address of the ERC20 token being supplied as - * consideration to the offerer of the fulfilled order. - * @param erc20Amount ERC20 tokens that will be transferred to the offerer - * of the fulfilled order. Note that the fulfiller must - * first approve this contract before the ERC20 tokens - * required as consideration can be transferred. - * @param erc1155Amount Total offererd ERC1155 tokens that will be - * transferred to the caller. Also note that calling - * contracts must implement `onERC1155Received` in - * order to receive tokens. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract * (or their proxy if indicated by the order) in order @@ -205,47 +197,37 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC1155Order( - address erc20Token, - uint256 erc20Amount, - uint256 erc1155Amount, - BasicOrderParameters memory parameters + BasicOrderParameters2 calldata parameters ) external override returns (bool) { + // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. - bool useOffererProxy = _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC1155, - parameters.token, - parameters.identifier, - erc1155Amount, - erc1155Amount - ), - ConsiderationItem( - ItemType.ERC20, - erc20Token, - 0, // No identifier for ERC20 tokens - erc20Amount, - erc20Amount, - parameters.offerer - ) + (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.ERC20, + ItemType.ERC20, + parameters.receivedToken, + ItemType.ERC1155 ); + // Move the offerer from memory to the stack. + address payable offerer = parameters.offerer; + // Transfer ERC1155 to caller, using offerer's proxy if applicable. _transferERC1155( - parameters.token, - parameters.offerer, + parameters.offeredToken, + offerer, msg.sender, - parameters.identifier, - erc1155Amount, - useOffererProxy ? parameters.offerer : address(0) + parameters.offeredIdentifier, + parameters.offeredAmount, + useOffererProxy ? offerer : address(0) ); // Transfer ERC20 tokens to all recipients and wrap up. _transferERC20AndFinalize( msg.sender, - parameters.offerer, - erc20Token, - erc20Amount, + offerer, + parameters.receivedToken, + parameters.receivedAmount, parameters, false // Transfer full amount indicated by all consideration items. ); @@ -259,15 +241,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param erc20Token The address of the ERC20 token being offered. - * @param erc20Amount ERC20 tokens that will be transferred from the - * offerer to the fulfiller and any additional - * recipients. Note that the offerer must first - * approve this contract before their offered ERC20 - * tokens to be transferred. Also note that the - * amount transferred to the fulfiller will be less - * than this amount if additional recipients have - * been specified. * @param parameters Additional information on the fulfilled order. * Note that the fulfiller must first approve this * contract (or their proxy if indicated by the @@ -275,56 +248,40 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * consideration can be transferred. Also note that * the sum of all additional recipient amounts * cannot exceed `erc20Amount`. - * @param useFulfillerProxy A boolean indicating whether to utilize the - * fulfiller's proxy when transferring the ERC721 - * item from the fulfiller to the offerer. * * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC721ForERC20Order( - address erc20Token, - uint256 erc20Amount, - BasicOrderParameters memory parameters, - bool useFulfillerProxy + BasicOrderParameters2 calldata parameters ) external override returns (bool) { - // Move the offerer from memory to the stack. - address payable offerer = parameters.offerer; - + // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. - _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC20, - erc20Token, - 0, // No identifier for ERC20 tokens - erc20Amount, - erc20Amount - ), - ConsiderationItem( - ItemType.ERC721, - parameters.token, - parameters.identifier, - 1, // Amount of 1 for ERC721 tokens - 1, // Amount of 1 for ERC721 tokens - offerer - ) + _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.ERC721, + ItemType.ERC20, + parameters.offeredToken, + ItemType.ERC20 ); + // Move the offerer from memory to the stack. + address payable offerer = parameters.offerer; + // Transfer ERC721 to offerer, using caller's proxy if applicable. _transferERC721( - parameters.token, + parameters.receivedToken, msg.sender, offerer, - parameters.identifier, - useFulfillerProxy ? msg.sender : address(0) + parameters.receivedIdentifier, + parameters.useFulfillerProxy ? msg.sender : address(0) ); // Transfer ERC20 tokens to all recipients and wrap up. _transferERC20AndFinalize( offerer, msg.sender, - erc20Token, - erc20Amount, + parameters.offeredToken, + parameters.offeredAmount, parameters, true // Reduce erc20Amount sent to fulfiller by additional amounts. ); @@ -338,19 +295,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param erc20Token The address of the ERC20 token being offered. - * @param erc20Amount ERC20 tokens that will be transferred from the - * offerer to the fulfiller and any additional - * recipients. Note that the offerer must first - * approve this contract before their offered ERC20 - * tokens to be transferred. Also note that the - * amount transferred to the fulfiller will be less - * than this amount if additional recipients have - * been specified. - * @param erc1155Amount Total ERC1155 tokens required to be transferred - * to the offerer as consideration. Note that - * offering contracts must implement - * `onERC1155Received` in order to receive tokens. * @param parameters Additional information on the fulfilled order. * Note that the fulfiller must first approve this * contract (or their proxy if indicated by the @@ -358,58 +302,40 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * consideration can be transferred. Also note that * the sum of all additional recipient amounts * cannot exceed `erc20Amount`. - * @param useFulfillerProxy A boolean indicating whether to utilize the - * fulfiller's proxy when transferring the ERC1155 - * item from the fulfiller to the offerer. * * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC1155ForERC20Order( - address erc20Token, - uint256 erc20Amount, - uint256 erc1155Amount, - BasicOrderParameters memory parameters, - bool useFulfillerProxy + BasicOrderParameters2 calldata parameters ) external override returns (bool) { - // Move the offerer from memory to the stack. - address payable offerer = parameters.offerer; - + // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. - _prepareBasicFulfillment( - parameters, - OfferItem( - ItemType.ERC20, - erc20Token, - 0, // No identifier for ERC20 tokens - erc20Amount, - erc20Amount - ), - ConsiderationItem( - ItemType.ERC1155, - parameters.token, - parameters.identifier, - erc1155Amount, - erc1155Amount, - offerer - ) + _prepareBasicFulfillmentFromCalldata( + parameters, + ItemType.ERC1155, + ItemType.ERC20, + parameters.offeredToken, + ItemType.ERC20 ); + // Move the offerer from memory to the stack. + address payable offerer = parameters.offerer; // Transfer ERC1155 to offerer, using caller's proxy if applicable. _transferERC1155( - parameters.token, + parameters.receivedToken, msg.sender, offerer, - parameters.identifier, - erc1155Amount, - useFulfillerProxy ? msg.sender : address(0) + parameters.receivedIdentifier, + parameters.receivedAmount, + parameters.useFulfillerProxy ? msg.sender : address(0) ); // Transfer ERC20 tokens to all recipients and wrap up. _transferERC20AndFinalize( offerer, msg.sender, - erc20Token, - erc20Amount, + parameters.offeredToken, + parameters.offeredAmount, parameters, true // Reduce erc20Amount sent to fulfiller by additional amounts. ); diff --git a/test/index.js b/test/index.js index 070ad24ce..08ea033f5 100644 --- a/test/index.js +++ b/test/index.js @@ -3050,31 +3050,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - token: order.parameters.consideration[0].token, - identifier: order.parameters.consideration[0].identifierOrCriteria, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC721ForERC20Order(testERC20.address, tokenAmount, basicOrderParameters, false); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC721ForERC20Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -3151,31 +3140,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - token: order.parameters.consideration[0].token, - identifier: order.parameters.consideration[0].identifierOrCriteria, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ], true); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC721ForERC20Order(testERC20.address, tokenAmount, basicOrderParameters, true); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC721ForERC20Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -4092,31 +4070,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC1155Order(testERC20.address, tokenAmount.sub(100), amount, basicOrderParameters); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC1155Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -4192,31 +4159,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC1155Order(testERC20.address, tokenAmount.sub(100), amount, basicOrderParameters); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC1155Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -4671,31 +4627,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - token: order.parameters.consideration[0].token, - identifier: order.parameters.consideration[0].identifierOrCriteria, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ]); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC1155ForERC20Order(testERC20.address, tokenAmount, amount, basicOrderParameters, false); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC1155ForERC20Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; @@ -4773,31 +4718,20 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - token: order.parameters.consideration[0].token, - identifier: order.parameters.consideration[0].identifierOrCriteria, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - additionalRecipients: [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order, [ + { + amount: ethers.BigNumber.from(50), + recipient: zone.address, + }, + { + amount: ethers.BigNumber.from(50), + recipient: owner.address, + } + ], true); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { - const tx = await marketplaceContract.connect(buyer).fulfillBasicERC1155ForERC20Order(testERC20.address, tokenAmount, amount, basicOrderParameters, true); + const tx = await marketplaceContract.connect(buyer).fulfillBasicERC1155ForERC20Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; From 54775c5b828ad20cd3059018fb918fe1db0c2c47 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 12:33:23 -0500 Subject: [PATCH 09/39] Convert remaining basic order fns to new struct and fulfillment fn --- .../interfaces/ConsiderationInterface.sol | 56 +------------------ 1 file changed, 3 insertions(+), 53 deletions(-) diff --git a/contracts/interfaces/ConsiderationInterface.sol b/contracts/interfaces/ConsiderationInterface.sol index cb191577b..a2cde687c 100644 --- a/contracts/interfaces/ConsiderationInterface.sol +++ b/contracts/interfaces/ConsiderationInterface.sol @@ -86,17 +86,6 @@ interface ConsiderationInterface { * of "additional recipients" may also be supplied which will each * receive ERC20 tokens from the fulfiller as consideration. * - * @param erc20Token The address of the ERC20 token being supplied as - * consideration to the offerer of the fulfilled order. - * @param erc20Amount ERC20 tokens that will be transferred to the - * offerer of the fulfilled order. Note that the - * fulfiller must first approve this contract before - * the ERC20 tokens required as consideration can be - * transferred. - * @param erc1155Amount Total offererd ERC1155 tokens that will be - * transferred to the caller. Also note that calling - * contracts must implement `onERC1155Received` in - * order to receive tokens. * @param parameters Additional information on the fulfilled order. Note * that the offerer must first approve this contract * (or their proxy if indicated by the order) in order @@ -105,10 +94,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC1155Order( - address erc20Token, - uint256 erc20Amount, - uint256 erc1155Amount, - BasicOrderParameters calldata parameters + BasicOrderParameters2 calldata parameters ) external returns (bool); /** @@ -116,16 +102,6 @@ interface ConsiderationInterface { * ERC721 token as consideration. An arbitrary number of "additional * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. - * - * @param erc20Token The address of the ERC20 token being offered. - * @param erc20Amount ERC20 tokens that will be transferred from the - * offerer to the fulfiller and any additional - * recipients. Note that the offerer must first - * approve this contract before their offered ERC20 - * tokens to be transferred. Also note that the - * amount transferred to the fulfiller will be less - * than this amount if additional recipients have - * been specified. * @param parameters Additional information on the fulfilled order. * Note that the fulfiller must first approve this * contract (or their proxy if indicated by the @@ -133,17 +109,11 @@ interface ConsiderationInterface { * consideration can be transferred. Also note that * the sum of all additional recipient amounts * cannot exceed `erc20Amount`. - * @param useFulfillerProxy A boolean indicating whether to utilize the - * fulfiller's proxy when transferring the ERC721 - * item from the fulfiller to the offerer. * * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC721ForERC20Order( - address erc20Token, - uint256 erc20Amount, - BasicOrderParameters memory parameters, - bool useFulfillerProxy + BasicOrderParameters2 calldata parameters ) external returns (bool); /** @@ -152,19 +122,6 @@ interface ConsiderationInterface { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param erc20Token The address of the ERC20 token being offered. - * @param erc20Amount ERC20 tokens that will be transferred from the - * offerer to the fulfiller and any additional - * recipients. Note that the offerer must first - * approve this contract before their offered ERC20 - * tokens to be transferred. Also note that the - * amount transferred to the fulfiller will be less - * than this amount if additional recipients have - * been specified. - * @param erc1155Amount Total ERC1155 tokens required to be transferred - * to the offerer as consideration. Note that - * offering contracts must implement - * `onERC1155Received` in order to receive tokens. * @param parameters Additional information on the fulfilled order. * Note that the fulfiller must first approve this * contract (or their proxy if indicated by the @@ -172,18 +129,11 @@ interface ConsiderationInterface { * consideration can be transferred. Also note that * the sum of all additional recipient amounts * cannot exceed `erc20Amount`. - * @param useFulfillerProxy A boolean indicating whether to utilize the - * fulfiller's proxy when transferring the ERC1155 - * item from the fulfiller to the offerer. * * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC1155ForERC20Order( - address erc20Token, - uint256 erc20Amount, - uint256 erc1155Amount, - BasicOrderParameters memory parameters, - bool useFulfillerProxy + BasicOrderParameters2 calldata parameters ) external returns (bool); /** From 75d65b675069f0facc5f30d0f331642ac4e78ee0 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 12:33:50 -0500 Subject: [PATCH 10/39] Remove old basic order functions --- contracts/lib/ConsiderationInternal.sol | 240 ------------------------ 1 file changed, 240 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 1523877ef..c364b90a3 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -277,120 +277,6 @@ contract ConsiderationInternal is ConsiderationInternalView { ); } - /** - * @dev Internal function to derive and validate an order based on a set of - * parameters and a primary item for offer and consideration. - * - * @param parameters The parameters of the basic order. - * @param offerItem The primary item being offered. - * @param considerationItem The primary item received as consideration. - * - * @return useOffererProxy A boolean indicating whether to utilize the - * offerer's proxy. - */ - function _prepareBasicFulfillment( - BasicOrderParameters memory parameters, - OfferItem memory offerItem, - ConsiderationItem memory considerationItem - ) internal returns (bool useOffererProxy) { - // Ensure this function cannot be triggered during a reentrant call. - _setReentrancyGuard(); - - // Pull frequently used arguments from memory & place them on the stack. - address payable offerer = parameters.offerer; - address zone = parameters.zone; - uint256 startTime = parameters.startTime; - uint256 endTime = parameters.endTime; - - // Ensure current timestamp falls between order start time and end time. - _assertValidTime(startTime, endTime); - - // Allocate memory: 1 offer, 1+additionalRecipients consideration items. - OfferItem[] memory offer = new OfferItem[](1); - ConsiderationItem[] memory consideration = new ConsiderationItem[]( - 1 + parameters.additionalRecipients.length - ); - - // Set primary offer + consideration item as respective first elements. - offer[0] = offerItem; - consideration[0] = considerationItem; - - // Get offer item's type and consideration item's token + put on stack. - ItemType itemType = offerItem.itemType; - address token = considerationItem.token; - - // Use offer item's info for additional recipients of native or ERC20. - if (_isEtherOrERC20Item(itemType)) { - // Set token for additional recipients to offer item's token. - token = offerItem.token; - } else { - // Otherwise, use consideration item's type on additional recipient. - itemType = considerationItem.itemType; - } - - // Skip overflow checks as for loop is indexed starting at one. - unchecked { - // Iterate over each consideration beyond primary one on the order. - for (uint256 i = 1; i < consideration.length; ++i) { - // Retrieve additional recipient corresponding to consideration. - AdditionalRecipient memory additionalRecipient = ( - parameters.additionalRecipients[i - 1] - ); - - // Set new consideration item as additional consideration item. - consideration[i] = ConsiderationItem( - itemType, - token, - 0, // No identifier for native tokens or ERC20. - additionalRecipient.amount, - additionalRecipient.amount, - additionalRecipient.recipient - ); - } - } - - // Retrieve current nonce and use it w/ parameters to derive order hash. - bytes32 orderHash = _getNoncedOrderHash( - OrderParameters( - offerer, - zone, - parameters.orderType, - startTime, - endTime, - parameters.salt, - offer, - consideration - ) - ); - - // Verify and update the status of the derived order. - _validateBasicOrderAndUpdateStatus( - orderHash, - offerer, - parameters.signature - ); - - // Determine if a proxy should be utilized and ensure a valid submitter. - useOffererProxy = _determineProxyUtilizationAndEnsureValidSubmitter( - parameters.orderType, - offerer, - zone - ); - - // Emit an event signifying that the order has been fulfilled. - _emitOrderFulfilledEvent( - orderHash, - offerer, - zone, - msg.sender, - offer, - consideration - ); - - // Return a boolean indicating whether to utilize offerer's proxy. - return useOffererProxy; - } - /** * @dev Internal function to verify and update the status of a basic order. * @@ -1470,75 +1356,6 @@ contract ConsiderationInternal is ConsiderationInternalView { _reentrancyGuard = _NOT_ENTERED; } - /** - * @dev Internal function to transfer Ether or other native tokens to a - * given recipient. Note that proxies are not utilized for native token - * transfers. - * - * @param amount The amount of Ether to transfer. - * @param parameters The parameters of the order. - */ - function _transferEthAndFinalize( - uint256 amount, - BasicOrderParameters memory parameters - ) internal { - // Put ether value supplied by the caller on the stack. - uint256 etherRemaining = msg.value; - - // Iterate over each additional recipient. - for (uint256 i = 0; i < parameters.additionalRecipients.length;) { - // Retrieve the additional recipient. - AdditionalRecipient memory additionalRecipient = ( - parameters.additionalRecipients[i] - ); - - // Read ether amount to transfer to recipient and place on stack. - uint256 additionalRecipientAmount = additionalRecipient.amount; - - // Ensure that sufficient Ether is available. - if (additionalRecipientAmount > etherRemaining) { - revert InsufficientEtherSupplied(); - } - - // Transfer Ether to the additional recipient. - _transferEth( - additionalRecipient.recipient, - additionalRecipientAmount - ); - - // Skip underflow check as subtracted value is less than remaining. - unchecked { - // Reduce ether value available. - etherRemaining -= additionalRecipientAmount; - } - - // Skip overflow check as for loop is indexed starting at zero. - unchecked { - ++i; - } - } - - // Ensure that sufficient Ether is still available. - if (amount > etherRemaining) { - revert InsufficientEtherSupplied(); - } - - // Transfer Ether to the offerer. - _transferEth(parameters.offerer, amount); - - // If any Ether remains after transfers, return it to the caller. - if (etherRemaining > amount) { - // Skip underflow check as etherRemaining > amount. - unchecked { - // Transfer remaining Ether to the caller. - _transferEth(payable(msg.sender), etherRemaining - amount); - } - } - - // Clear the reentrancy guard. - _reentrancyGuard = _NOT_ENTERED; - } - // todo: delete old version, add natspec, look into optimizations /** * @dev Internal function to transfer ERC20 tokens to a given recipient. @@ -1597,63 +1414,6 @@ contract ConsiderationInternal is ConsiderationInternalView { _reentrancyGuard = _NOT_ENTERED; } - /** - * @dev Internal function to transfer ERC20 tokens to a given recipient. - * Note that proxies are not utilized for ERC20 tokens. - * - * @param from The originator of the ERC20 token transfer. - * @param to The recipient of the ERC20 token transfer. - * @param erc20Token The ERC20 token to transfer. - * @param amount The amount of ERC20 tokens to transfer. - * @param parameters The parameters of the order. - * @param fromOfferer Whether to decrement amount from the offered amount. - */ - function _transferERC20AndFinalize( - address from, - address to, - address erc20Token, - uint256 amount, - BasicOrderParameters memory parameters, - bool fromOfferer - ) internal { - // Iterate over each additional recipient. - for (uint256 i = 0; i < parameters.additionalRecipients.length;) { - // Retrieve the additional recipient. - AdditionalRecipient memory additionalRecipient = ( - parameters.additionalRecipients[i] - ); - - // Decrement the amount to transfer to fulfiller if indicated. - if (fromOfferer) { - amount -= additionalRecipient.amount; - } - - // Transfer ERC20 tokens to additional recipient given approval. - _transferERC20( - erc20Token, - from, - additionalRecipient.recipient, - additionalRecipient.amount - ); - - // Skip overflow check as for loop is indexed starting at zero. - unchecked { - ++i; - } - } - - // Transfer ERC20 token amount (from account must have proper approval). - _transferERC20( - erc20Token, - from, - to, - amount - ); - - // Clear the reentrancy guard. - _reentrancyGuard = _NOT_ENTERED; - } - /** * @dev Internal function to ensure that the sentinel value for the * reentrancy guard is not currently set and, if not, to set the From 35e21f21b7429394ac81c97eabd8a69ee4b61309 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 12:39:07 -0500 Subject: [PATCH 11/39] Delete old BasicOrderParameters, rename BasicOrderParameters2 => BasicOrderParameters --- contracts/Consideration.sol | 13 ++++++------- .../interfaces/ConsiderationInterface.sol | 13 ++++++------- contracts/lib/ConsiderationInternal.sol | 7 +++---- contracts/lib/ConsiderationStructs.sol | 18 ++---------------- 4 files changed, 17 insertions(+), 34 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index e5e16daa9..cfedba7d5 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -9,7 +9,6 @@ import { ItemType } from "./lib/ConsiderationEnums.sol"; import { BasicOrderParameters, - BasicOrderParameters2, OfferItem, ConsiderationItem, OrderParameters, @@ -51,7 +50,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { ) ConsiderationInternal(legacyProxyRegistry, requiredProxyImplementation) {} function fulfillBasicEthForERC721Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external payable override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. @@ -99,7 +98,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC1155Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external payable override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. @@ -147,7 +146,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC721Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. @@ -197,7 +196,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC1155Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. @@ -252,7 +251,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC721ForERC20Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. @@ -306,7 +305,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC1155ForERC20Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external override returns (bool) { // todo: Look into whether it is necessary to do further validation on parameters // Derive and validate order using parameters and update order status. diff --git a/contracts/interfaces/ConsiderationInterface.sol b/contracts/interfaces/ConsiderationInterface.sol index a2cde687c..74cad3c8d 100644 --- a/contracts/interfaces/ConsiderationInterface.sol +++ b/contracts/interfaces/ConsiderationInterface.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.12; import { BasicOrderParameters, - BasicOrderParameters2, OrderComponents, Fulfillment, Execution, @@ -43,7 +42,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC721Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external payable returns (bool); /** @@ -60,7 +59,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicEthForERC1155Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external payable returns (bool); /** @@ -77,7 +76,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC721Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external returns (bool); /** @@ -94,7 +93,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC20ForERC1155Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external returns (bool); /** @@ -113,7 +112,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC721ForERC20Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external returns (bool); /** @@ -133,7 +132,7 @@ interface ConsiderationInterface { * @return A boolean indicating whether the order has been fulfilled. */ function fulfillBasicERC1155ForERC20Order( - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) external returns (bool); /** diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index c364b90a3..3502e8d84 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -17,7 +17,6 @@ import { import { AdditionalRecipient, BasicOrderParameters, - BasicOrderParameters2, OfferItem, ConsiderationItem, SpentItem, @@ -62,7 +61,7 @@ contract ConsiderationInternal is ConsiderationInternalView { // todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem // todo: clean up comments, add natspec function _prepareBasicFulfillmentFromCalldata( - BasicOrderParameters2 calldata parameters, + BasicOrderParameters calldata parameters, ItemType receivedItemType, ItemType additionalRecipientsItemType, address additionalRecipientsToken, @@ -1297,7 +1296,7 @@ contract ConsiderationInternal is ConsiderationInternalView { // todo: delete old version, add natspec, look into optimizations function _transferEthAndFinalize( uint256 amount, - BasicOrderParameters2 calldata parameters + BasicOrderParameters calldata parameters ) internal { // Put ether value supplied by the caller on the stack. uint256 etherRemaining = msg.value; @@ -1373,7 +1372,7 @@ contract ConsiderationInternal is ConsiderationInternalView { address to, address erc20Token, uint256 amount, - BasicOrderParameters2 calldata parameters, + BasicOrderParameters calldata parameters, bool fromOfferer ) internal { // Iterate over each additional recipient. diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index c7e05f94a..d28e63c17 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -84,28 +84,14 @@ struct ReceivedItem { address payable recipient; } +// todo: remove cdptr comments +// todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem /** * @dev For basic orders involving ETH / native / ERC20 <=> ERC721 / ERC1155 * matching, a group of six functions may be called that only requires a * subset of the usual order arguments. */ struct BasicOrderParameters { - address payable offerer; - address zone; - address token; - uint256 identifier; - OrderType orderType; - uint256 startTime; - uint256 endTime; - uint256 salt; - AdditionalRecipient[] additionalRecipients; - bytes signature; -} - -// todo: rename to BasicOrderParameters, delete old struct -// todo: remove cdptr comments -// todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem -struct BasicOrderParameters2 { address receivedToken; // 0x24 uint256 receivedIdentifier; // 0x44 uint256 receivedAmount; // 0x64 From 4d127251c90f61925e5f0df6ba8504b4f3686df2 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 13:54:55 -0500 Subject: [PATCH 12/39] derive additional recipients in getBasicOrderParameters from considerations array --- test/index.js | 423 ++++---------------------------------------------- 1 file changed, 32 insertions(+), 391 deletions(-) diff --git a/test/index.js b/test/index.js index 08ea033f5..1db5a574a 100644 --- a/test/index.js +++ b/test/index.js @@ -42,7 +42,6 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function const getBasicOrderParameters = ( order, - additionalRecipients = [], useFulfillerProxy = false ) => ({ offerer: order.parameters.offerer, @@ -59,7 +58,12 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function salt: order.parameters.salt, signature: order.signature, useFulfillerProxy, - additionalRecipients, + additionalRecipients: order.parameters.consideration + .slice(1) + .map(({ endAmount, recipient }) => ({ + amount: endAmount, + recipient, + })), }); const convertSignatureToEIP2098 = (signature) => { @@ -1509,16 +1513,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -1586,39 +1581,8 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - /* 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; */ - - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -1693,16 +1657,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function .withArgs(orderHash, seller.address, zone.address); }); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -1775,16 +1730,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function order.signature = convertSignatureToEIP2098(order.signature); expect(order.signature.length).to.equal(130); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -1852,16 +1798,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -2385,16 +2322,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); // basicOrderParameters.receivedAmount = basicOrderParameters.receivedAmount.sub(100); @@ -2482,16 +2410,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -2573,16 +2492,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function seller ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -3050,16 +2960,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { @@ -3140,16 +3041,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], true); + const basicOrderParameters = getBasicOrderParameters(order, true); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { @@ -3565,16 +3457,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -3643,16 +3526,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -4070,16 +3944,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -4159,16 +4024,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { @@ -4627,16 +4483,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]); + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { @@ -4718,16 +4565,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ], true); + const basicOrderParameters = getBasicOrderParameters(order, true); await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], ethers.BigNumber.from(0), null, async () => { @@ -8508,16 +8346,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function expect(orderStatus.totalFilled).to.equal(1); expect(orderStatus.totalSize).to.equal(2); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC1155Order(basicOrderParameters, {value})).to.be.reverted; @@ -8769,32 +8598,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function // set an invalid V value order.signature = order.signature.slice(0, -2) + '01'; - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; @@ -8889,16 +8693,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function zone // wrong signer ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(40), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(60), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters)).to.be.reverted; @@ -8980,16 +8775,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function zone // wrong signer ); - const basicOrderParameters = getBasicOrderParameters(order, [ - { - amount: ethers.BigNumber.from(50), - recipient: zone.address, - }, - { - amount: ethers.BigNumber.from(50), - recipient: owner.address, - } - ]) + const basicOrderParameters = getBasicOrderParameters(order) await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters)).to.be.reverted; @@ -10102,32 +9888,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 4, // FULL_OPEN_VIA_PROXY ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; @@ -10404,32 +10165,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function "NOT_STARTED" ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; @@ -10494,32 +10230,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function "EXPIRED" ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; @@ -10735,32 +10446,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value: ethers.BigNumber.from(1)})).to.be.reverted; @@ -10838,27 +10524,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - 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: order.signature, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(buyer.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillOrder(order, false, {value: ethers.BigNumber.from(1)})).to.be.reverted; @@ -11774,32 +11440,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function 0, // FULL_OPEN ); - const basicOrderParameters = { - offerer: order.parameters.offerer, - zone: order.parameters.zone, - orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: 1, - receivedToken: constants.AddressZero, - receivedIdentifier: constants.Zero, - receivedAmount: order.parameters.consideration[0].endAmount, - startTime: order.parameters.startTime, - endTime: order.parameters.endTime, - salt: order.parameters.salt, - signature: order.signature, - useFulfillerProxy: false, - additionalRecipients: [ - { - amount: ethers.utils.parseEther("1"), - recipient: zone.address, - }, - { - amount: ethers.utils.parseEther("1"), - recipient: owner.address, - } - ], - }; + const basicOrderParameters = getBasicOrderParameters(order); await whileImpersonating(owner.address, provider, async () => { await expect(marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, {value})).to.be.reverted; From 8a6eae185825baeefc0e4172d90a7795a63ce113 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 25 Mar 2022 13:55:32 -0500 Subject: [PATCH 13/39] remove unused fn _isEtherOrERC20Item --- contracts/lib/ConsiderationPure.sol | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/contracts/lib/ConsiderationPure.sol b/contracts/lib/ConsiderationPure.sol index ea1f4fc08..570761b95 100644 --- a/contracts/lib/ConsiderationPure.sol +++ b/contracts/lib/ConsiderationPure.sol @@ -1131,22 +1131,6 @@ contract ConsiderationPure is ConsiderationBase { } } - /** - * @dev Internal pure function to check whether a given item type represents - * an Ether or ERC20 item. - * - * @param itemType The item type in question. - * - * @return A boolean indicating that the item type represents either Ether - * or an ERC20 item. - */ - function _isEtherOrERC20Item( - ItemType itemType - ) internal pure returns (bool) { - // Ether is item type 0 and ERC20 is item type 1. - return uint256(itemType) < 2; - } - /** * @dev Internal pure function to check whether a given item type represents * a criteria-based ERC721 or ERC1155 item (e.g. an item that can be From f41a912e756f5748583e963e3bf28866abc5e512 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 13:31:27 -0500 Subject: [PATCH 14/39] Add tests for calldata encoding --- test/index.js | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/test/index.js b/test/index.js index 1db5a574a..65bd2b7da 100644 --- a/test/index.js +++ b/test/index.js @@ -11448,6 +11448,92 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }) + describe("Basic Order Calldata", () => { + let calldata, value; + + before(async () => { + // Seller mints nft + const nftId = ethers.BigNumber.from(randomHex()); + await testERC721.mint(seller.address, nftId); + + // Seller approves marketplace contract to transfer NFT + await whileImpersonating(seller.address, provider, async () => { + await expect(testERC721.connect(seller).setApprovalForAll(marketplaceContract.address, true)) + .to.emit(testERC721, "ApprovalForAll") + .withArgs(seller.address, marketplaceContract.address, true); + }); + + const offer = [ + { + itemType: 2, // ERC721 + token: testERC721.address, + identifierOrCriteria: nftId, + startAmount: ethers.BigNumber.from(1), + endAmount: ethers.BigNumber.from(1), + }, + ]; + + const consideration = [ + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("10"), + endAmount: ethers.utils.parseEther("10"), + recipient: seller.address, + }, + ]; + let order; + ({ order, value } = await createOrder( + seller, + zone, + offer, + consideration, + 0, // FULL_OPEN + )); + + const basicOrderParameters = getBasicOrderParameters(order); + ({ data: calldata } = await marketplaceContract.populateTransaction.fulfillBasicEthForERC721Order(basicOrderParameters)); + }) + + it("Reverts if BasicOrderParameters has non-default offset", async () => { + const badData = [calldata.slice(0, 73), "1", calldata.slice(74)].join(''); + expect(badData.length).to.eq(calldata.length) + + await whileImpersonating(owner.address, provider, async () => { + await expect(buyer.sendTransaction({ + to: marketplaceContract.address, + data: badData, + value + })).to.be.revertedWith("InvalidBasicOrderParameterEncoding") + }) + }) + + it("Reverts if additionalRecipients has non-default offset", async () => { + const badData = [calldata.slice(0, 969), "1", calldata.slice(970)].join(''); + + await whileImpersonating(owner.address, provider, async () => { + await expect(buyer.sendTransaction({ + to: marketplaceContract.address, + data: badData, + value + })).to.be.revertedWith("InvalidBasicOrderParameterEncoding") + }) + }) + + it("Reverts if signature has non-default offset", async () => { + const badData = [calldata.slice(0, 1033), "1", calldata.slice(1034)].join(''); + + await whileImpersonating(owner.address, provider, async () => { + await expect(buyer.sendTransaction({ + to: marketplaceContract.address, + data: badData, + value + })).to.be.revertedWith("InvalidBasicOrderParameterEncoding") + }) + }) + }) + describe("Reentrancy", async () => { it("Reverts on a reentrant call", async () => { // Seller mints nft From cc7335304833b4d22d3a2cfae07e3314b5b4b873 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 13:38:25 -0500 Subject: [PATCH 15/39] Add signature offset validation to _assertValidBasicOrderParameterOffsets --- contracts/lib/ConsiderationInternalView.sol | 27 +++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index a40a0b194..c0691b0d5 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -66,19 +66,36 @@ contract ConsiderationInternalView is ConsiderationPure { } } - /** - * @dev Validate BasicOrderParameters offset match the default abi encoding. + * @dev Validate calldata offsets for dynamic types in BasicOrderParameters. * This ensures that functions using the calldata object normally will be * using the same data as the assembly functions. + * Note: No parameters because all basic order functions use the same + * calldata encoding. */ function _assertValidBasicOrderParameterOffsets() internal pure { bool validOffsets; - // todo validate signature offset assembly { + /* + * Checks: + * 1. Order parameters struct offset = 0x20 + * 2. Additional recipients arr offset = 0x1e0 + * 3. Signature offset = 0x200 + (recipients.length * 0x40) + */ + validOffsets := and( + // Order parameters have offset of 0x20 + eq(calldataload(0x04), 0x20), + // Additional recipients have offset of 0x1e0 + eq(calldataload(0x1c4), 0x1e0) + ) validOffsets := and( - eq(calldataload(0x04), 0x20), // Order parameters have offset of 0x20 - eq(calldataload(0x1c4), 0x1e0) // Additional recipients have offset of 0x1e0 + validOffsets, + eq( + // Load signature offset from calldata + calldataload(0x1e4), + // Calculate expected offset (start of recipients + len * 64) + add(0x200, mul(calldataload(0x204), 0x40)) + ) ) } if (!validOffsets) revert InvalidBasicOrderParameterEncoding(); From 09bad8fe99a2548d9b153daae0adb6f4b1e61685 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 13:39:10 -0500 Subject: [PATCH 16/39] Move _assertValidBasicOrderParameterOffsets to ConsiderationPure.sol --- contracts/lib/ConsiderationInternalView.sol | 35 --------------------- contracts/lib/ConsiderationPure.sol | 35 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index c0691b0d5..716860904 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -66,41 +66,6 @@ contract ConsiderationInternalView is ConsiderationPure { } } - /** - * @dev Validate calldata offsets for dynamic types in BasicOrderParameters. - * This ensures that functions using the calldata object normally will be - * using the same data as the assembly functions. - * Note: No parameters because all basic order functions use the same - * calldata encoding. - */ - function _assertValidBasicOrderParameterOffsets() internal pure { - bool validOffsets; - assembly { - /* - * Checks: - * 1. Order parameters struct offset = 0x20 - * 2. Additional recipients arr offset = 0x1e0 - * 3. Signature offset = 0x200 + (recipients.length * 0x40) - */ - validOffsets := and( - // Order parameters have offset of 0x20 - eq(calldataload(0x04), 0x20), - // Additional recipients have offset of 0x1e0 - eq(calldataload(0x1c4), 0x1e0) - ) - validOffsets := and( - validOffsets, - eq( - // Load signature offset from calldata - calldataload(0x1e4), - // Calculate expected offset (start of recipients + len * 64) - add(0x200, mul(calldataload(0x204), 0x40)) - ) - ) - } - if (!validOffsets) revert InvalidBasicOrderParameterEncoding(); - } - /** * @dev Internal view function to validate whether a token transfer was * successful based on the returned status and data. Note that diff --git a/contracts/lib/ConsiderationPure.sol b/contracts/lib/ConsiderationPure.sol index 570761b95..9580501b9 100644 --- a/contracts/lib/ConsiderationPure.sol +++ b/contracts/lib/ConsiderationPure.sol @@ -1295,4 +1295,39 @@ contract ConsiderationPure is ConsiderationBase { value := keccak256(0x00, 0x40) // Hash scratch space region. } } + + /** + * @dev Validate calldata offsets for dynamic types in BasicOrderParameters. + * This ensures that functions using the calldata object normally will be + * using the same data as the assembly functions. + * Note: No parameters because all basic order functions use the same + * calldata encoding. + */ + function _assertValidBasicOrderParameterOffsets() internal pure { + bool validOffsets; + assembly { + /* + * Checks: + * 1. Order parameters struct offset = 0x20 + * 2. Additional recipients arr offset = 0x1e0 + * 3. Signature offset = 0x200 + (recipients.length * 0x40) + */ + validOffsets := and( + // Order parameters have offset of 0x20 + eq(calldataload(0x04), 0x20), + // Additional recipients have offset of 0x1e0 + eq(calldataload(0x1c4), 0x1e0) + ) + validOffsets := and( + validOffsets, + eq( + // Load signature offset from calldata + calldataload(0x1e4), + // Calculate expected offset (start of recipients + len * 64) + add(0x200, mul(calldataload(0x204), 0x40)) + ) + ) + } + if (!validOffsets) revert InvalidBasicOrderParameterEncoding(); + } } From 403d765716b857b4ce502fd5a5867ac8dc72139c Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 14:16:28 -0500 Subject: [PATCH 17/39] Move zone to 0xa4 --- contracts/lib/ConsiderationStructs.sol | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index d28e63c17..ed979b487 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -84,29 +84,29 @@ struct ReceivedItem { address payable recipient; } -// todo: remove cdptr comments -// todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem +// todo: update naming scheme to reflect change from +// OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem /** * @dev For basic orders involving ETH / native / ERC20 <=> ERC721 / ERC1155 * matching, a group of six functions may be called that only requires a * subset of the usual order arguments. */ struct BasicOrderParameters { - address receivedToken; // 0x24 - uint256 receivedIdentifier; // 0x44 - uint256 receivedAmount; // 0x64 - address payable offerer; // 0x84 - address offeredToken; // 0xa4 - uint256 offeredIdentifier; // 0xc4 - uint256 offeredAmount; // 0xe4 - OrderType orderType; // 0x104 - uint256 startTime; // 0x124 - uint256 endTime; // 0x144 - uint256 salt; // 0x164 - address zone; // 0x184 - bool useFulfillerProxy; // 0x1a4 + address receivedToken; // 0x24 + uint256 receivedIdentifier; // 0x44 + uint256 receivedAmount; // 0x64 + address payable offerer; // 0x84 + address zone; // 0xa4 + address offeredToken; // 0xc4 + uint256 offeredIdentifier; // 0xe4 + uint256 offeredAmount; // 0x104 + OrderType orderType; // 0x124 + uint256 startTime; // 0x144 + uint256 endTime; // 0x164 + uint256 salt; // 0x184 + bool useFulfillerProxy; // 0x1a4 AdditionalRecipient[] additionalRecipients; // 0x1c4 - bytes signature; // 0x1e4 + bytes signature; // 0x1e4 // len : 0x204 } From bdf80b702d19900367a84e7e20f88a90a3c49e9c Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 14:19:12 -0500 Subject: [PATCH 18/39] Improve comments --- contracts/lib/ConsiderationInternal.sol | 215 ++++++++++++++---------- 1 file changed, 127 insertions(+), 88 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 3502e8d84..8c08ac114 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -58,8 +58,32 @@ contract ConsiderationInternal is ConsiderationInternalView { requiredProxyImplementation ) {} - // todo: update naming scheme to reflect change from OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem - // todo: clean up comments, add natspec + // todo: update naming scheme to reflect change from + // OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem + /** + * @dev Prepare fulfillment of a basic order with manual calldata + * and memory access. This calculates the order hash, emits the + * OrderFulfilled event, and asserts basic order validity. + * + * Note: Calldata offsets are validated in this function because it + * accesses constant calldata pointers for dynamic types that match + * default ABI encoding, but valid ABI encoding can use arbitrary offsets. + * Checking that the offsets were produced by default encoding will ensure + * that other functions using Solidity's calldata accessors (which + * calculate pointers from the stored offsets) are reading the same + * data as the order hash is derived from. + * + * Note: This function accesses memory directly. It does not clear the + * expanded memory regions used, nor does it update the free memory pointer, + * so other direct memory access must not assume that unused memory is empty. + * + * Assertions: + * - Non-reentrancy + * - Valid start and end times + * - Valid calldata encoding + * protection + + */ function _prepareBasicFulfillmentFromCalldata( BasicOrderParameters calldata parameters, ItemType receivedItemType, @@ -74,38 +98,13 @@ contract ConsiderationInternal is ConsiderationInternalView { // Ensure calldata offsets were produced by default encoding _assertValidBasicOrderParameterOffsets(); - /* - event OrderFulfilled( - bytes32 orderHash, - address indexed offerer, - address indexed zone, - address fulfiller, - ConsumedItem[] offer, (itemType, token, identifier, amount) - FulfilledItem[] consideration (itemType, token, identifier, amount, recipient) - ) - data - 0x00: orderHash - 0x20: fulfiller - 0x40: offer arr ptr (0x80) - 0x60: consideration arr ptr (0x120) - 0x80: offer arr len (1) - 0xa0: offer.itemType - 0xc0: offer.token - 0xe0: offer.identifier - 0x100: offer.amount - 0x120: 1 + recipients.length - 0x140: recipient 0 - */ - - { // Handle received items bytes32 typeHash = _CONSIDERATION_ITEM_TYPEHASH; assembly { - // todo: Update event ptr to avoid overwriting order data when additional recipients has length 0 /* Memory Layout * 0x60: hash of considerations array - * 0x80-0x160: scratch space for EIP712 hashing of considerations + * 0x80-0x160: reused space for EIP712 hashing of considerations * - 0x80: _RECEIVED_ITEM_TYPEHASH * - 0xa0: itemType * - 0xc0: token @@ -113,19 +112,24 @@ contract ConsiderationInternal is ConsiderationInternalView { * - 0x100: startAmount * - 0x120: endAmount * - 0x140: recipient - * 0x160-END_ARR: array of consideration hashes (END_ARR = 0x180 + ADDITIONAL_RECIPIENTS_LENGTH * 0x20) + * - 0x160-END_ARR: array of consideration hashes + * (END_ARR = 0x180 + RECIPIENTS_LENGTH * 0x20) * - 0x160: EIP712 hash of primary consideration - * - 0x180-END_ARR: EIP712 hashes of additional recipients considerations + * - 0x180-END_ARR: EIP712 hashes of additional recipient considerations * END_ARR: beginning of data for OrderFulfilled event * END_ARR + 0x120: length of FulfilledItem array * END_ARR + 0x140: beginning of data for first FulfilledItem */ /* 1. Write first ReceivedItem hash to order's considerations array */ + // Write type hash and item type mstore(0x80, typeHash) mstore(0xa0, receivedItemType) - calldatacopy(0xc0, 0x24, 0x60) // (token, identifier, startAmount) - calldatacopy(0x120, 0x64, 0x40) // (endAmount, recipient) - mstore(0x160, keccak256(0x80, 0xe0)) // receivedItemHashes[0] = keccak256(abi.encode(receivedItem)) + // Copy (token, identifier, startAmount) + calldatacopy(0xc0, 0x24, 0x60) + // Copy (endAmount, recipient) + calldatacopy(0x120, 0x64, 0x40) + // receivedItemHashes[0] = keccak256(abi.encode(receivedItem)) + mstore(0x160, keccak256(0x80, 0xe0)) /* 2. Write first FulfilledItem to OrderFulfilled data */ let len := calldataload(0x204) @@ -134,40 +138,48 @@ contract ConsiderationInternal is ConsiderationInternalView { mstore(eventArrPtr, add(len, 1)) // length // Set ptr to data portion of first FulfilledItem eventArrPtr := add(eventArrPtr, 0x20) + // Write item type mstore(eventArrPtr, receivedItemType) - calldatacopy(add(eventArrPtr, 0x20), 0x24, 0x80) // (token, identifier, amount, recipient) + // Copy (token, identifier, amount, recipient) + calldatacopy(add(eventArrPtr, 0x20), 0x24, 0x80) /* 3. Handle additional recipients */ - let considerationHashesPtr := 0x160 // ptr to current place in receivedItemHashes - // Update scratch space with (type, token, identifier) for additional recipients + // ptr to current place in receivedItemHashes + let considerationHashesPtr := 0x160 + // Write type, token, identifier for additional recipients memory + // which will be reused for each recipient mstore(0xa0, additionalRecipientsItemType) mstore(0xc0, additionalRecipientsToken) mstore(0xe0, 0) - for { - let i := 0 - } lt(i, len) { - i := add(i, 1) - } { + for {let i := 0} lt(i, len) {i := add(i, 1)} { let additionalRecipientCdPtr := add(0x224, mul(0x40, i)) + /* a. Write ReceivedItem hash to order's considerations array */ - calldatacopy(0x100, additionalRecipientCdPtr, 0x20) // startAmount - calldatacopy(0x120, additionalRecipientCdPtr, 0x40) // endAmount, recipient - // Add 1 word to the pointer each loop to reduce ops needed to get local offset into the array + // Copy startAmount + calldatacopy(0x100, additionalRecipientCdPtr, 0x20) + // Copy endAmount, recipient + calldatacopy(0x120, additionalRecipientCdPtr, 0x40) + // note: Add 1 word to the pointer each loop to reduce ops + // needed to get local offset into the array considerationHashesPtr := add(considerationHashesPtr, 0x20) - mstore(considerationHashesPtr, keccak256(0x80, 0xe0)) // receivedItemHashes[i + 1] = keccak256(abi.encode(receivedItem)) + // receivedItemHashes[i + 1] = keccak256(abi.encode(receivedItem)) + mstore(considerationHashesPtr, keccak256(0x80, 0xe0)) /* b. Write FulfilledItem to OrderFulfilled data */ - // At this point, eventArrPtr points to the beginning of the FulfilledItem struct - // for the previous element in the array. + // At this point, eventArrPtr points to the beginning of the + // FulfilledItem struct for the previous element in the array. eventArrPtr := add(eventArrPtr, 0xa0) + // Write item type mstore(eventArrPtr, additionalRecipientsItemType) + // Write token mstore(add(eventArrPtr, 0x20), additionalRecipientsToken) - calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) // endAmount, recipient + // Copy endAmount, recipient + calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) } - // arrPtr := sub(arrPtr, mul(len, 0x20)) // Get back to original receivedItemHashes pointer /* 4. Hash packed array of ReceivedItem EIP712 hashes */ - // note: Store receivedItemsHash at 0x60 - all other memory use begins at 0x80 - mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) // keccak256(abi.encodePacked(receivedItemHashes)) + // note: Store at 0x60 - all other memory begins at 0x80 + // keccak256(abi.encodePacked(receivedItemHashes)) + mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) } } @@ -177,7 +189,7 @@ contract ConsiderationInternal is ConsiderationInternalView { * - 0x80: _OFFERED_ITEM_TYPEHASH * - 0xa0: itemType * - 0xc0: token - * - 0xe0: identifier (reused to store offered items array hash once calculated) + * - 0xe0: identifier (reused for offeredItemsHash) * - 0x100: startAmount * - 0x120: endAmount */ @@ -186,19 +198,27 @@ contract ConsiderationInternal is ConsiderationInternalView { /* 1. Calculate OfferedItem EIP712 hash*/ mstore(0x80, typeHash) // _OFFERED_ITEM_TYPEHASH mstore(0xa0, offeredItemType) // itemType - calldatacopy(0xc0, 0xa4, 0x60) // (token, identifier, startAmount) - calldatacopy(0x120, 0xe4, 0x20) // endAmount - mstore(0x00, keccak256(0x80, 0xc0)) // keccak256(abi.encode(offeredItem)) + calldatacopy(0xc0, 0xc4, 0x60) // (token, identifier, startAmount) + calldatacopy(0x120, 0x104, 0x20) // endAmount + // note: Write offered item hash to scratch space + // keccak256(abi.encode(offeredItem)) + mstore(0x00, keccak256(0x80, 0xc0)) /* 2. Calculate hash of array of EIP712 hashes */ // note: Write offeredItemsHash to offer struct - mstore(0xe0, keccak256(0x00, 0x20)) // keccak256(abi.encodePacked(offeredItemHashes)) + // keccak256(abi.encodePacked(offeredItemHashes)) + mstore(0xe0, keccak256(0x00, 0x20)) /* 3. Write ConsumedItem array to event data */ // 0x180 + len*32 = event data ptr // offers array length is stored at 0x80 into the event data let eventArrPtr := add(0x200, mul(0x20, calldataload(0x204))) mstore(eventArrPtr, 1) mstore(add(eventArrPtr, 0x20), offeredItemType) - calldatacopy(add(eventArrPtr, 0x40), 0xa4, 0x60) // (token, identifier, startAmount) + // Copy token, identifier, startAmount to ConsumedItem + calldatacopy( + add(eventArrPtr, 0x40), + 0xc4, + 0x60 + ) } } { // Calculate order hash @@ -206,7 +226,7 @@ contract ConsiderationInternal is ConsiderationInternalView { address zone; assembly { offerer := calldataload(0x84) - zone := calldataload(0x184) + zone := calldataload(0xa4) } uint256 nonce = _nonces[offerer][zone]; bytes32 typeHash = _ORDER_HASH; @@ -225,40 +245,59 @@ contract ConsiderationInternal is ConsiderationInternalView { * - 0x1a0: nonce */ mstore(0x80, typeHash) - // todo: rearrange calldata for zone and offerer - calldatacopy(0xa0, 0x84, 0x20) // offerer - calldatacopy(0xc0, 0x184, 0x20) // zone - mstore(0x100, mload(0x60)) // pull receivedItemsHash out of scratch space - calldatacopy(0x120, 0x104, 0x80) // orderType, startTime, endTime, salt + // Copy offerer and zone + calldatacopy(0xa0, 0x84, 0x40) + // load receivedItemsHash from zero slot + mstore(0x100, mload(0x60)) + // orderType, startTime, endTime, salt + calldatacopy(0x120, 0x124, 0x80) mstore(0x1a0, nonce) // nonce orderHash := keccak256(0x80, 0x140) } } - /* 0x00: orderHash - 0x20: fulfiller - 0x40: offer arr ptr (0x80) - 0x60: consideration arr ptr (0x120) - 0x80: offer arr len (1) - 0xa0: offer.itemType - 0xc0: offer.token - 0xe0: offer.identifier - 0x100: offer.amount - 0x120: 1 + recipients.length - 0x140: recipient 0 */ - { - // todo: Replace with constant hex - bytes32 sig = keccak256('OrderFulfilled(bytes32,address,address,address,(uint8,address,uint256,uint256)[],(uint8,address,uint256,uint256,address)[])'); - assembly { - let eventDataPtr := add(0x180, mul(0x20, calldataload(0x204))) - mstore(eventDataPtr, orderHash) // orderHash - mstore(add(eventDataPtr, 0x20), caller()) // fulfiller - mstore(add(eventDataPtr, 0x40), 0x80) // ConsumedItem array pointer - mstore(add(eventDataPtr, 0x60), 0x120) // FulfilledItem array pointer - let dataSize := add(0x1e0, mul(calldataload(0x204), 0xa0)) - log3(eventDataPtr, dataSize, sig, calldataload(0x84), calldataload(0x184) /* topic1, topic2 */) - /* Restore the zero slot */ - mstore(0x60, 0) - } + /* event OrderFulfilled( + * bytes32 orderHash, + * address indexed offerer, + * address indexed zone, + * address fulfiller, + * ConsumedItem[] offer, (itemType, token, id, amount) + * FulfilledItem[] consideration (itemType, token, id, amount, recipient) + * ) + * topic0 - OrderFulfilled event signature + * topic1 - offerer + * topic2 - zone + * data + * 0x00: orderHash + * 0x20: fulfiller + * 0x40: offer arr ptr (0x80) + * 0x60: consideration arr ptr (0x120) + * 0x80: offer arr len (1) + * 0xa0: offer.itemType + * 0xc0: offer.token + * 0xe0: offer.identifier + * 0x100: offer.amount + * 0x120: 1 + recipients.length + * 0x140: recipient 0 + */ + assembly { + let eventDataPtr := add(0x180, mul(0x20, calldataload(0x204))) + mstore(eventDataPtr, orderHash) // orderHash + mstore(add(eventDataPtr, 0x20), caller()) // fulfiller + mstore(add(eventDataPtr, 0x40), 0x80) // ConsumedItem array pointer + mstore(add(eventDataPtr, 0x60), 0x120) // FulfilledItem array pointer + let dataSize := add(0x1e0, mul(calldataload(0x204), 0xa0)) + log3( + eventDataPtr, + dataSize, + // OrderFulfilled event signature + 0x9d9af8e38d66c62e2c12f0225249fd9d721c54b83f48d9352c97c6cacdcb6f31, + // topic1 - offerer + calldataload(0x84), + // topic2 - zone + calldataload(0xa4) + ) + /* Restore the zero slot */ + mstore(0x60, 0) } // Verify and update the status of the derived order. From 1e7130eb4439f5f8e10f813ae5b2b9a3e90a9498 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 14:19:34 -0500 Subject: [PATCH 19/39] Shorten excessively long lines --- contracts/Consideration.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index cfedba7d5..9e98f274c 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -52,7 +52,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicEthForERC721Order( BasicOrderParameters calldata parameters ) external payable override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -100,7 +100,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicEthForERC1155Order( BasicOrderParameters calldata parameters ) external payable override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -148,7 +148,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC20ForERC721Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -198,7 +198,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC20ForERC1155Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -253,7 +253,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC721ForERC20Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. _prepareBasicFulfillmentFromCalldata( parameters, @@ -307,7 +307,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC1155ForERC20Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: Look into whether it is necessary to do further validation on parameters + // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. _prepareBasicFulfillmentFromCalldata( parameters, From 365141fee044c1f5156d5ee20faccf9f4b8e8267 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 14:26:23 -0500 Subject: [PATCH 20/39] Shorten excessively long line --- contracts/interfaces/ConsiderationEventsAndErrors.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/interfaces/ConsiderationEventsAndErrors.sol b/contracts/interfaces/ConsiderationEventsAndErrors.sol index 4309dd4cb..11a216037 100644 --- a/contracts/interfaces/ConsiderationEventsAndErrors.sol +++ b/contracts/interfaces/ConsiderationEventsAndErrors.sol @@ -373,7 +373,7 @@ interface ConsiderationEventsAndErrors { /** * @dev Revert with an error when attempting to fill a basic order using - * calldata not produced by default ABI encoding, i.e. suboptimal offsets. + * calldata not produced by default ABI encoding. */ error InvalidBasicOrderParameterEncoding(); } From afc6c335b8595c3a0efa4a357a69269c071c2158 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 16:13:10 -0500 Subject: [PATCH 21/39] Fix natspec comment --- contracts/lib/ConsiderationInternal.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 8c08ac114..717bc742a 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -81,8 +81,6 @@ contract ConsiderationInternal is ConsiderationInternalView { * - Non-reentrancy * - Valid start and end times * - Valid calldata encoding - * protection - */ function _prepareBasicFulfillmentFromCalldata( BasicOrderParameters calldata parameters, From 864bb566659f460d0da7f404afeb8a7dbd309630 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Thu, 31 Mar 2022 17:56:15 -0500 Subject: [PATCH 22/39] Fix basic order parameters for fulfillBasicERC20ForERC721Order test --- test/index.js | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/test/index.js b/test/index.js index 7ead221a8..a67509413 100644 --- a/test/index.js +++ b/test/index.js @@ -2575,30 +2575,13 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function ); 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, - } - ], - }; + ...getBasicOrderParameters(order), + signature: "0x" + } // 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; + await expect(marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters)).to.be.reverted; }); // Compute the digest based on the order hash @@ -2613,7 +2596,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function // 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 tx = await marketplaceContract.connect(buyer).fulfillBasicERC20ForERC721Order(basicOrderParameters); const receipt = await tx.wait(); await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); return receipt; From bea4522518616d7d8eefc6aacc54ff9b49edee96 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 16:25:32 -0700 Subject: [PATCH 23/39] make a few naming changes --- contracts/lib/ConsiderationInternal.sol | 34 ++++++++++++------------- contracts/lib/ConsiderationStructs.sol | 6 ++--- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 717bc742a..8e861d5e0 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -58,8 +58,6 @@ contract ConsiderationInternal is ConsiderationInternalView { requiredProxyImplementation ) {} - // todo: update naming scheme to reflect change from - // OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem /** * @dev Prepare fulfillment of a basic order with manual calldata * and memory access. This calculates the order hash, emits the @@ -115,8 +113,8 @@ contract ConsiderationInternal is ConsiderationInternalView { * - 0x160: EIP712 hash of primary consideration * - 0x180-END_ARR: EIP712 hashes of additional recipient considerations * END_ARR: beginning of data for OrderFulfilled event - * END_ARR + 0x120: length of FulfilledItem array - * END_ARR + 0x140: beginning of data for first FulfilledItem + * END_ARR + 0x120: length of ReceivedItem array + * END_ARR + 0x140: beginning of data for first ReceivedItem */ /* 1. Write first ReceivedItem hash to order's considerations array */ // Write type hash and item type @@ -129,12 +127,12 @@ contract ConsiderationInternal is ConsiderationInternalView { // receivedItemHashes[0] = keccak256(abi.encode(receivedItem)) mstore(0x160, keccak256(0x80, 0xe0)) - /* 2. Write first FulfilledItem to OrderFulfilled data */ + /* 2. Write first ReceivedItem to OrderFulfilled data */ let len := calldataload(0x204) // END_ARR + 0x120 = 0x2a0 + len*0x20 let eventArrPtr := add(0x2a0, mul(0x20, len)) mstore(eventArrPtr, add(len, 1)) // length - // Set ptr to data portion of first FulfilledItem + // Set ptr to data portion of first ReceivedItem eventArrPtr := add(eventArrPtr, 0x20) // Write item type mstore(eventArrPtr, receivedItemType) @@ -152,7 +150,7 @@ contract ConsiderationInternal is ConsiderationInternalView { for {let i := 0} lt(i, len) {i := add(i, 1)} { let additionalRecipientCdPtr := add(0x224, mul(0x40, i)) - /* a. Write ReceivedItem hash to order's considerations array */ + /* a. Write ConsiderationItem hash to order's considerations array */ // Copy startAmount calldatacopy(0x100, additionalRecipientCdPtr, 0x20) // Copy endAmount, recipient @@ -163,9 +161,9 @@ contract ConsiderationInternal is ConsiderationInternalView { // receivedItemHashes[i + 1] = keccak256(abi.encode(receivedItem)) mstore(considerationHashesPtr, keccak256(0x80, 0xe0)) - /* b. Write FulfilledItem to OrderFulfilled data */ + /* b. Write ReceivedItem to OrderFulfilled data */ // At this point, eventArrPtr points to the beginning of the - // FulfilledItem struct for the previous element in the array. + // ReceivedItem struct for the previous element in the array. eventArrPtr := add(eventArrPtr, 0xa0) // Write item type mstore(eventArrPtr, additionalRecipientsItemType) @@ -174,7 +172,7 @@ contract ConsiderationInternal is ConsiderationInternalView { // Copy endAmount, recipient calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) } - /* 4. Hash packed array of ReceivedItem EIP712 hashes */ + /* 4. Hash packed array of ConsiderationItem EIP712 hashes */ // note: Store at 0x60 - all other memory begins at 0x80 // keccak256(abi.encodePacked(receivedItemHashes)) mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) @@ -183,7 +181,7 @@ contract ConsiderationInternal is ConsiderationInternalView { { // Handle offered items /* Memory Layout - * EIP712 data for OfferedItem + * EIP712 data for OfferItem * - 0x80: _OFFERED_ITEM_TYPEHASH * - 0xa0: itemType * - 0xc0: token @@ -193,7 +191,7 @@ contract ConsiderationInternal is ConsiderationInternalView { */ bytes32 typeHash = _OFFER_ITEM_TYPEHASH; assembly { - /* 1. Calculate OfferedItem EIP712 hash*/ + /* 1. Calculate OfferItem EIP712 hash*/ mstore(0x80, typeHash) // _OFFERED_ITEM_TYPEHASH mstore(0xa0, offeredItemType) // itemType calldatacopy(0xc0, 0xc4, 0x60) // (token, identifier, startAmount) @@ -205,13 +203,13 @@ contract ConsiderationInternal is ConsiderationInternalView { // note: Write offeredItemsHash to offer struct // keccak256(abi.encodePacked(offeredItemHashes)) mstore(0xe0, keccak256(0x00, 0x20)) - /* 3. Write ConsumedItem array to event data */ + /* 3. Write SpentItem array to event data */ // 0x180 + len*32 = event data ptr // offers array length is stored at 0x80 into the event data let eventArrPtr := add(0x200, mul(0x20, calldataload(0x204))) mstore(eventArrPtr, 1) mstore(add(eventArrPtr, 0x20), offeredItemType) - // Copy token, identifier, startAmount to ConsumedItem + // Copy token, identifier, startAmount to SpentItem calldatacopy( add(eventArrPtr, 0x40), 0xc4, @@ -258,8 +256,8 @@ contract ConsiderationInternal is ConsiderationInternalView { * address indexed offerer, * address indexed zone, * address fulfiller, - * ConsumedItem[] offer, (itemType, token, id, amount) - * FulfilledItem[] consideration (itemType, token, id, amount, recipient) + * SpentItem[] offer, (itemType, token, id, amount) + * ReceivedItem[] consideration (itemType, token, id, amount, recipient) * ) * topic0 - OrderFulfilled event signature * topic1 - offerer @@ -281,8 +279,8 @@ contract ConsiderationInternal is ConsiderationInternalView { let eventDataPtr := add(0x180, mul(0x20, calldataload(0x204))) mstore(eventDataPtr, orderHash) // orderHash mstore(add(eventDataPtr, 0x20), caller()) // fulfiller - mstore(add(eventDataPtr, 0x40), 0x80) // ConsumedItem array pointer - mstore(add(eventDataPtr, 0x60), 0x120) // FulfilledItem array pointer + mstore(add(eventDataPtr, 0x40), 0x80) // SpentItem array pointer + mstore(add(eventDataPtr, 0x60), 0x120) // ReceivedItem array pointer let dataSize := add(0x1e0, mul(calldataload(0x204), 0xa0)) log3( eventDataPtr, diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index ed979b487..d279d1bd2 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -84,14 +84,12 @@ struct ReceivedItem { address payable recipient; } -// todo: update naming scheme to reflect change from -// OfferedItem -> OfferItem & ReceivedItem -> ConsiderationItem /** * @dev For basic orders involving ETH / native / ERC20 <=> ERC721 / ERC1155 * matching, a group of six functions may be called that only requires a * subset of the usual order arguments. */ -struct BasicOrderParameters { +struct BasicOrderParameters { // calldata offset address receivedToken; // 0x24 uint256 receivedIdentifier; // 0x44 uint256 receivedAmount; // 0x64 @@ -107,7 +105,7 @@ struct BasicOrderParameters { bool useFulfillerProxy; // 0x1a4 AdditionalRecipient[] additionalRecipients; // 0x1c4 bytes signature; // 0x1e4 - // len : 0x204 + // Total length, excluding dynamic array data: 0x204 (516) } /** From 7aaf174a6b3a40a0448a2e9d46289f2548c01e68 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 31 Mar 2022 16:53:45 -0700 Subject: [PATCH 24/39] change offered => offer + received => consideration --- contracts/Consideration.sol | 58 +++++++++++++------------- contracts/lib/ConsiderationStructs.sol | 12 +++--- test/index.js | 12 +++--- 3 files changed, 41 insertions(+), 41 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index 9e98f274c..7ff2e6446 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -67,16 +67,16 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC721 to caller, using offerer's proxy if applicable. _transferERC721( - parameters.offeredToken, + parameters.offerToken, offerer, msg.sender, - parameters.offeredIdentifier, + parameters.offerIdentifier, useOffererProxy ? offerer : address(0) ); // Transfer native to recipients, return excess to caller, and wrap up. _transferEthAndFinalize( - parameters.receivedAmount, + parameters.considerationAmount, parameters ); @@ -115,17 +115,17 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC1155 to caller, using offerer's proxy if applicable. _transferERC1155( - parameters.offeredToken, + parameters.offerToken, offerer, msg.sender, - parameters.offeredIdentifier, - parameters.offeredAmount, + parameters.offerIdentifier, + parameters.offerAmount, useOffererProxy ? offerer : address(0) ); // Transfer native to recipients, return excess to caller, and wrap up. _transferEthAndFinalize( - parameters.receivedAmount, + parameters.considerationAmount, parameters ); @@ -154,7 +154,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { parameters, ItemType.ERC20, ItemType.ERC20, - parameters.receivedToken, + parameters.considerationToken, ItemType.ERC721 ); @@ -163,10 +163,10 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC721 to caller, using offerer's proxy if applicable. _transferERC721( - parameters.offeredToken, + parameters.offerToken, offerer, msg.sender, - parameters.offeredIdentifier, + parameters.offerIdentifier, useOffererProxy ? offerer : address(0) ); @@ -174,8 +174,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { _transferERC20AndFinalize( msg.sender, offerer, - parameters.receivedToken, - parameters.receivedAmount, + parameters.considerationToken, + parameters.considerationAmount, parameters, false // Transfer full amount indicated by all consideration items. ); @@ -204,7 +204,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { parameters, ItemType.ERC20, ItemType.ERC20, - parameters.receivedToken, + parameters.considerationToken, ItemType.ERC1155 ); @@ -213,11 +213,11 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC1155 to caller, using offerer's proxy if applicable. _transferERC1155( - parameters.offeredToken, + parameters.offerToken, offerer, msg.sender, - parameters.offeredIdentifier, - parameters.offeredAmount, + parameters.offerIdentifier, + parameters.offerAmount, useOffererProxy ? offerer : address(0) ); @@ -225,8 +225,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { _transferERC20AndFinalize( msg.sender, offerer, - parameters.receivedToken, - parameters.receivedAmount, + parameters.considerationToken, + parameters.considerationAmount, parameters, false // Transfer full amount indicated by all consideration items. ); @@ -259,7 +259,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { parameters, ItemType.ERC721, ItemType.ERC20, - parameters.offeredToken, + parameters.offerToken, ItemType.ERC20 ); @@ -268,10 +268,10 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC721 to offerer, using caller's proxy if applicable. _transferERC721( - parameters.receivedToken, + parameters.considerationToken, msg.sender, offerer, - parameters.receivedIdentifier, + parameters.considerationIdentifier, parameters.useFulfillerProxy ? msg.sender : address(0) ); @@ -279,8 +279,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { _transferERC20AndFinalize( offerer, msg.sender, - parameters.offeredToken, - parameters.offeredAmount, + parameters.offerToken, + parameters.offerAmount, parameters, true // Reduce erc20Amount sent to fulfiller by additional amounts. ); @@ -313,7 +313,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { parameters, ItemType.ERC1155, ItemType.ERC20, - parameters.offeredToken, + parameters.offerToken, ItemType.ERC20 ); // Move the offerer from memory to the stack. @@ -321,11 +321,11 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { // Transfer ERC1155 to offerer, using caller's proxy if applicable. _transferERC1155( - parameters.receivedToken, + parameters.considerationToken, msg.sender, offerer, - parameters.receivedIdentifier, - parameters.receivedAmount, + parameters.considerationIdentifier, + parameters.considerationAmount, parameters.useFulfillerProxy ? msg.sender : address(0) ); @@ -333,8 +333,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { _transferERC20AndFinalize( offerer, msg.sender, - parameters.offeredToken, - parameters.offeredAmount, + parameters.offerToken, + parameters.offerAmount, parameters, true // Reduce erc20Amount sent to fulfiller by additional amounts. ); diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index d279d1bd2..5a84d20c4 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -90,14 +90,14 @@ struct ReceivedItem { * subset of the usual order arguments. */ struct BasicOrderParameters { // calldata offset - address receivedToken; // 0x24 - uint256 receivedIdentifier; // 0x44 - uint256 receivedAmount; // 0x64 + address considerationToken; // 0x24 + uint256 considerationIdentifier; // 0x44 + uint256 considerationAmount; // 0x64 address payable offerer; // 0x84 address zone; // 0xa4 - address offeredToken; // 0xc4 - uint256 offeredIdentifier; // 0xe4 - uint256 offeredAmount; // 0x104 + address offerToken; // 0xc4 + uint256 offerIdentifier; // 0xe4 + uint256 offerAmount; // 0x104 OrderType orderType; // 0x124 uint256 startTime; // 0x144 uint256 endTime; // 0x164 diff --git a/test/index.js b/test/index.js index a67509413..e93fb6146 100644 --- a/test/index.js +++ b/test/index.js @@ -47,12 +47,12 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function offerer: order.parameters.offerer, zone: order.parameters.zone, orderType: order.parameters.orderType, - offeredToken: order.parameters.offer[0].token, - offeredIdentifier: order.parameters.offer[0].identifierOrCriteria, - offeredAmount: order.parameters.offer[0].endAmount, - receivedToken: order.parameters.consideration[0].token, - receivedIdentifier: order.parameters.consideration[0].identifierOrCriteria, - receivedAmount: order.parameters.consideration[0].endAmount, + offerToken: order.parameters.offer[0].token, + offerIdentifier: order.parameters.offer[0].identifierOrCriteria, + offerAmount: order.parameters.offer[0].endAmount, + considerationToken: order.parameters.consideration[0].token, + considerationIdentifier: order.parameters.consideration[0].identifierOrCriteria, + considerationAmount: order.parameters.consideration[0].endAmount, startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, From fb1d3cadd67a311fa9f2a09c52c65f163afda6a0 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Fri, 1 Apr 2022 16:38:51 -0700 Subject: [PATCH 25/39] perform ERC721 amount = 1 check on each transfer --- contracts/Consideration.sol | 3 +++ contracts/lib/ConsiderationInternal.sol | 13 ++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index 7ff2e6446..8714114ff 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -71,6 +71,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { offerer, msg.sender, parameters.offerIdentifier, + parameters.offerAmount, useOffererProxy ? offerer : address(0) ); @@ -167,6 +168,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { offerer, msg.sender, parameters.offerIdentifier, + parameters.offerAmount, useOffererProxy ? offerer : address(0) ); @@ -272,6 +274,7 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { msg.sender, offerer, parameters.considerationIdentifier, + parameters.considerationAmount, parameters.useFulfillerProxy ? msg.sender : address(0) ); diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 8e861d5e0..887ec4b8f 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -997,17 +997,13 @@ contract ConsiderationInternal is ConsiderationInternalView { address proxyOwner = useProxy ? offerer : address(0); if (item.itemType == ItemType.ERC721) { - // Ensure that exactly one 721 item is being transferred. - if (item.amount != 1) { - revert InvalidERC721TransferAmount(); - } - // Transfer ERC721 token from the offerer to the recipient. _transferERC721( item.token, offerer, item.recipient, item.identifier, + item.amount, proxyOwner ); } else { @@ -1111,6 +1107,7 @@ contract ConsiderationInternal is ConsiderationInternalView { * @param from The originator of the transfer. * @param to The recipient of the transfer. * @param identifier The tokenId to transfer. + * @param amount The "amount" (this value must be equal to one). * @param proxyOwner An address indicating the owner of the proxy to utilize * when performing the transfer, or the null address if no * proxy should be utilized. @@ -1120,8 +1117,14 @@ contract ConsiderationInternal is ConsiderationInternalView { address from, address to, uint256 identifier, + uint256 amount, address proxyOwner ) internal { + // Ensure that exactly one 721 item is being transferred. + if (amount != 1) { + revert InvalidERC721TransferAmount(); + } + // Perform transfer, either directly or via proxy. bool success = _callDirectlyOrViaProxy( token, From ec30f7047a9001af513bdfecaedb98903050ec8d Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 1 Apr 2022 19:58:40 -0500 Subject: [PATCH 26/39] Add originalConsiderationsCount to basic order struct --- contracts/lib/ConsiderationInternal.sol | 10 +++++----- contracts/lib/ConsiderationStructs.sol | 7 ++++--- test/index.js | 5 +++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 887ec4b8f..7f04e4703 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -128,7 +128,7 @@ contract ConsiderationInternal is ConsiderationInternalView { mstore(0x160, keccak256(0x80, 0xe0)) /* 2. Write first ReceivedItem to OrderFulfilled data */ - let len := calldataload(0x204) + let len := calldataload(0x224) // END_ARR + 0x120 = 0x2a0 + len*0x20 let eventArrPtr := add(0x2a0, mul(0x20, len)) mstore(eventArrPtr, add(len, 1)) // length @@ -148,7 +148,7 @@ contract ConsiderationInternal is ConsiderationInternalView { mstore(0xc0, additionalRecipientsToken) mstore(0xe0, 0) for {let i := 0} lt(i, len) {i := add(i, 1)} { - let additionalRecipientCdPtr := add(0x224, mul(0x40, i)) + let additionalRecipientCdPtr := add(0x244, mul(0x40, i)) /* a. Write ConsiderationItem hash to order's considerations array */ // Copy startAmount @@ -206,7 +206,7 @@ contract ConsiderationInternal is ConsiderationInternalView { /* 3. Write SpentItem array to event data */ // 0x180 + len*32 = event data ptr // offers array length is stored at 0x80 into the event data - let eventArrPtr := add(0x200, mul(0x20, calldataload(0x204))) + let eventArrPtr := add(0x200, mul(0x20, calldataload(0x224))) mstore(eventArrPtr, 1) mstore(add(eventArrPtr, 0x20), offeredItemType) // Copy token, identifier, startAmount to SpentItem @@ -276,12 +276,12 @@ contract ConsiderationInternal is ConsiderationInternalView { * 0x140: recipient 0 */ assembly { - let eventDataPtr := add(0x180, mul(0x20, calldataload(0x204))) + let eventDataPtr := add(0x180, mul(0x20, calldataload(0x224))) mstore(eventDataPtr, orderHash) // orderHash mstore(add(eventDataPtr, 0x20), caller()) // fulfiller mstore(add(eventDataPtr, 0x40), 0x80) // SpentItem array pointer mstore(add(eventDataPtr, 0x60), 0x120) // ReceivedItem array pointer - let dataSize := add(0x1e0, mul(calldataload(0x204), 0xa0)) + let dataSize := add(0x1e0, mul(calldataload(0x224), 0xa0)) log3( eventDataPtr, dataSize, diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index 5a84d20c4..4829c4baa 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -103,9 +103,10 @@ struct BasicOrderParameters { // calldata offset uint256 endTime; // 0x164 uint256 salt; // 0x184 bool useFulfillerProxy; // 0x1a4 - AdditionalRecipient[] additionalRecipients; // 0x1c4 - bytes signature; // 0x1e4 - // Total length, excluding dynamic array data: 0x204 (516) + uint256 originalConsiderationsCount; // 0c1c4 + AdditionalRecipient[] additionalRecipients; // 0x1e4 + bytes signature; // 0x204 + // Total length, excluding dynamic array data: 0x224 (516) } /** diff --git a/test/index.js b/test/index.js index e93fb6146..5f9fe18bf 100644 --- a/test/index.js +++ b/test/index.js @@ -56,6 +56,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, + originalConsiderationsCount: order.parameters.consideration.length, signature: order.signature, useFulfillerProxy, additionalRecipients: order.parameters.consideration @@ -11610,7 +11611,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }) it("Reverts if additionalRecipients has non-default offset", async () => { - const badData = [calldata.slice(0, 969), "1", calldata.slice(970)].join(''); + const badData = [calldata.slice(0, 1033), "1", calldata.slice(1034)].join(''); await whileImpersonating(owner.address, provider, async () => { await expect(buyer.sendTransaction({ @@ -11622,7 +11623,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }) it("Reverts if signature has non-default offset", async () => { - const badData = [calldata.slice(0, 1033), "1", calldata.slice(1034)].join(''); + const badData = [calldata.slice(0, 1097), "1", calldata.slice(1098)].join(''); await whileImpersonating(owner.address, provider, async () => { await expect(buyer.sendTransaction({ From 4c555807d97269300957ad5602f8f2d2998f2813 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Fri, 1 Apr 2022 19:58:50 -0500 Subject: [PATCH 27/39] Add originalConsiderationsCount to basic order struct --- contracts/lib/ConsiderationPure.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/lib/ConsiderationPure.sol b/contracts/lib/ConsiderationPure.sol index 9580501b9..26cbaee2c 100644 --- a/contracts/lib/ConsiderationPure.sol +++ b/contracts/lib/ConsiderationPure.sol @@ -1315,16 +1315,16 @@ contract ConsiderationPure is ConsiderationBase { validOffsets := and( // Order parameters have offset of 0x20 eq(calldataload(0x04), 0x20), - // Additional recipients have offset of 0x1e0 - eq(calldataload(0x1c4), 0x1e0) + // Additional recipients have offset of 0x200 + eq(calldataload(0x1e4), 0x200) ) validOffsets := and( validOffsets, eq( // Load signature offset from calldata - calldataload(0x1e4), + calldataload(0x204), // Calculate expected offset (start of recipients + len * 64) - add(0x200, mul(calldataload(0x204), 0x40)) + add(0x220, mul(calldataload(0x224), 0x40)) ) ) } From 6e8b00ae16b23b5576c1b26c1de00e62da933958 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Mon, 4 Apr 2022 15:41:54 -0500 Subject: [PATCH 28/39] Include all considerations in event for basic order, only use originals for hash --- contracts/lib/ConsiderationInternal.sol | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 7f04e4703..1335c3226 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -147,7 +147,9 @@ contract ConsiderationInternal is ConsiderationInternalView { mstore(0xa0, additionalRecipientsItemType) mstore(0xc0, additionalRecipientsToken) mstore(0xe0, 0) - for {let i := 0} lt(i, len) {i := add(i, 1)} { + len := calldataload(0x1c4) + let i := 0 + for {} lt(i, len) {i := add(i, 1)} { let additionalRecipientCdPtr := add(0x244, mul(0x40, i)) /* a. Write ConsiderationItem hash to order's considerations array */ @@ -172,6 +174,21 @@ contract ConsiderationInternal is ConsiderationInternalView { // Copy endAmount, recipient calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) } + len := calldataload(0x224) + for {} lt(i, len) {i := add(i, 1)} { + let additionalRecipientCdPtr := add(0x244, mul(0x40, i)) + + /* b. Write ReceivedItem to OrderFulfilled data */ + // At this point, eventArrPtr points to the beginning of the + // ReceivedItem struct for the previous element in the array. + eventArrPtr := add(eventArrPtr, 0xa0) + // Write item type + mstore(eventArrPtr, additionalRecipientsItemType) + // Write token + mstore(add(eventArrPtr, 0x20), additionalRecipientsToken) + // Copy endAmount, recipient + calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) + } /* 4. Hash packed array of ConsiderationItem EIP712 hashes */ // note: Store at 0x60 - all other memory begins at 0x80 // keccak256(abi.encodePacked(receivedItemHashes)) From 40f079c4c4dead264b1402878bc79b01bfb4784f Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 09:00:27 -0700 Subject: [PATCH 29/39] originalConsiderationsCount => totalOriginalConsiderationItems --- contracts/lib/ConsiderationStructs.sol | 4 ++-- test/index.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index 4829c4baa..b544465d3 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -103,10 +103,10 @@ struct BasicOrderParameters { // calldata offset uint256 endTime; // 0x164 uint256 salt; // 0x184 bool useFulfillerProxy; // 0x1a4 - uint256 originalConsiderationsCount; // 0c1c4 + uint256 totalOriginalConsiderationItems; // 0c1c4 AdditionalRecipient[] additionalRecipients; // 0x1e4 bytes signature; // 0x204 - // Total length, excluding dynamic array data: 0x224 (516) + // Total length, excluding dynamic array data: 0x224 (548) } /** diff --git a/test/index.js b/test/index.js index 5f9fe18bf..4ef489610 100644 --- a/test/index.js +++ b/test/index.js @@ -56,7 +56,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, - originalConsiderationsCount: order.parameters.consideration.length, + totalOriginalConsiderationItems: order.parameters.consideration.length, signature: order.signature, useFulfillerProxy, additionalRecipients: order.parameters.consideration From 89e2bd76d9cc709ae93437014925d6f798d31a32 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 09:48:47 -0700 Subject: [PATCH 30/39] implement "tipping" on non-basic functions --- contracts/Consideration.sol | 75 ++++++++++++------- .../ConsiderationEventsAndErrors.sol | 13 +++- contracts/lib/ConsiderationInternal.sol | 4 +- contracts/lib/ConsiderationInternalView.sol | 28 +++++-- contracts/lib/ConsiderationStructs.sol | 4 +- test/index.js | 3 + 6 files changed, 87 insertions(+), 40 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index 8714114ff..d7f798928 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -49,6 +49,20 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { address requiredProxyImplementation ) ConsiderationInternal(legacyProxyRegistry, requiredProxyImplementation) {} + /** + * @notice Fulfill an order offering an ERC721 token by supplying Ether (or + * the native token for the given chain) as consideration for the + * order. An arbitrary number of "additional recipients" may also be + * supplied which will each receive native tokens from the fulfiller + * as consideration. + * + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC721 token to be transferred. + * + * @return A boolean indicating whether the order has been fulfilled. + */ function fulfillBasicEthForERC721Order( BasicOrderParameters calldata parameters ) external payable override returns (bool) { @@ -91,10 +105,10 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * supplied which will each receive native tokens from the fulfiller * as consideration. * - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract - * (or their proxy if indicated by the order) in order - * for their offered ERC1155 tokens to be transferred. + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC1155 tokens to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -139,10 +153,10 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. * - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract (or - * their proxy if indicated by the order) in order for - * their offered ERC721 token to be transferred. + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC721 token to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -190,10 +204,11 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * tokens as consideration. An arbitrary number of "additional * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract - * (or their proxy if indicated by the order) in order - * for their offered ERC1155 tokens to be transferred. + * + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC1155 tokens to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -242,13 +257,12 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param parameters Additional information on the fulfilled order. - * Note that the fulfiller must first approve this - * contract (or their proxy if indicated by the - * order) before the ERC721 token required as - * consideration can be transferred. Also note that - * the sum of all additional recipient amounts - * cannot exceed `erc20Amount`. + * @param parameters Additional information on the fulfilled order. Note + * that the fulfiller must first approve this contract (or + * their proxy if indicated by the order) before the + * ERC721 token required as consideration can be + * transferred. Also note that the sum of all additional + * recipient amounts cannot exceed `erc20Amount`. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -297,13 +311,12 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param parameters Additional information on the fulfilled order. - * Note that the fulfiller must first approve this - * contract (or their proxy if indicated by the - * order) before the ERC1155 token required as - * consideration can be transferred. Also note that - * the sum of all additional recipient amounts - * cannot exceed `erc20Amount`. + * @param parameters Additional information on the fulfilled order. Note + * that the fulfiller must first approve this contract (or + * their proxy if indicated by the order) before the + * ERC1155 token required as consideration can be + * transferred. Also note that the sum of all additional + * recipient amounts cannot exceed `erc20Amount`. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -576,7 +589,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { order.endTime, order.salt, order.offer, - order.consideration + order.consideration, + order.consideration.length ), order.nonce ); @@ -628,7 +642,9 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { offerer = orderParameters.offerer; // Get current nonce and use it w/ params to derive order hash. - orderHash = _getNoncedOrderHash(orderParameters); + orderHash = _assertConsiderationLengthAndGetNoncedOrderHash( + orderParameters + ); // Retrieve the order status using the derived order hash. OrderStatus memory orderStatus = _orderStatus[orderHash]; @@ -713,7 +729,8 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { order.endTime, order.salt, order.offer, - order.consideration + order.consideration, + order.consideration.length ), order.nonce ); diff --git a/contracts/interfaces/ConsiderationEventsAndErrors.sol b/contracts/interfaces/ConsiderationEventsAndErrors.sol index 82c6ba6a7..408b52244 100644 --- a/contracts/interfaces/ConsiderationEventsAndErrors.sol +++ b/contracts/interfaces/ConsiderationEventsAndErrors.sol @@ -86,7 +86,8 @@ interface ConsiderationEventsAndErrors { /** * @dev Revert with an error when attempting to fill an order outside the - * specified start time and end time .*/ + * specified start time and end time . + */ error InvalidTime(); /** @@ -96,6 +97,12 @@ interface ConsiderationEventsAndErrors { */ error InvalidSubmitterOnRestrictedOrder(); + /** + * @dev Revert with an error when an order is supplied for fulfillment with + * a consideration array that is shorter than the original array. + */ + error MissingOriginalConsiderationItems(); + /** * @dev Revert with an error when a fulfillment is provided that does not * declare at least one offer component and at least one consideration @@ -353,13 +360,13 @@ interface ConsiderationEventsAndErrors { /** * @dev Revert with an error when a caller attempts to reenter a protected - function. + * function. */ error NoReentrantCalls(); /** * @dev Revert with an error when the implementation of the respectie proxy - does not match the expected proxy implementation. + * does not match the expected proxy implementation. */ error InvalidProxyImplementation(); diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 1335c3226..910ac5c0c 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -417,7 +417,9 @@ contract ConsiderationInternal is ConsiderationInternalView { } // Retrieve current nonce and use it w/ parameters to derive order hash. - orderHash = _getNoncedOrderHash(orderParameters); + orderHash = _assertConsiderationLengthAndGetNoncedOrderHash( + orderParameters + ); // Determine if a proxy should be utilized and ensure a valid submitter. useOffererProxy = _determineProxyUtilizationAndEnsureValidSubmitter( diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index bf9ba3af7..2fb05660a 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -294,6 +294,9 @@ contract ConsiderationInternalView is ConsiderationPure { /** * @dev Internal view function to derive the order hash for a given order. + * Note that only the original consideration items are included in the + * order hash, as additional consideration items may be supplied by the + * caller. * * @param orderParameters The parameters of the order to hash. * @param nonce The nonce of the order to hash. @@ -304,14 +307,18 @@ contract ConsiderationInternalView is ConsiderationPure { OrderParameters memory orderParameters, uint256 nonce ) internal view returns (bytes32) { - // Put offer and consideration item array lengths onto the stack. + // Get length of full offer array and place it on the stack. uint256 offerLength = orderParameters.offer.length; - uint256 considerationLength = orderParameters.consideration.length; + + // Get length of original consideration array and place it on the stack. + uint256 originalConsiderationLength = ( + orderParameters.totalOriginalConsiderationItems + ); // Designate new memory regions for offer and consideration item hashes. bytes32[] memory offerHashes = new bytes32[](offerLength); bytes32[] memory considerationHashes = new bytes32[]( - considerationLength + originalConsiderationLength ); // Skip overflow checks as all for loops are indexed starting at zero. @@ -322,8 +329,8 @@ contract ConsiderationInternalView is ConsiderationPure { offerHashes[i] = _hashOfferItem(orderParameters.offer[i]); } - // Iterate over each consideration on the order. - for (uint256 i = 0; i < considerationLength; ++i) { + // Iterate over each original consideration on the order. + for (uint256 i = 0; i < originalConsiderationLength; ++i) { // Hash the consideration and place the result into memory. considerationHashes[i] = _hashConsiderationItem( orderParameters.consideration[i] @@ -356,9 +363,18 @@ contract ConsiderationInternalView is ConsiderationPure { * * @return The hash. */ - function _getNoncedOrderHash( + function _assertConsiderationLengthAndGetNoncedOrderHash( OrderParameters memory orderParameters ) internal view returns (bytes32) { + // Ensure supplied consideration array length is not less than original. + if ( + orderParameters.consideration.length < ( + orderParameters.totalOriginalConsiderationItems + ) + ) { + revert MissingOriginalConsiderationItems(); + } + // Derive and return order hash using current nonce for offerer in zone. return _getOrderHash( orderParameters, diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index b544465d3..3802168a0 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -122,7 +122,8 @@ struct AdditionalRecipient { /** * @dev The full set of order components, with the exception of the nonce, must * be supplied when fulfilling more sophisticated orders or groups of - * orders. + * orders. The total number of original consideration items must also be + * supplied, as the caller may specify additional consideration items. */ struct OrderParameters { address offerer; @@ -133,6 +134,7 @@ struct OrderParameters { uint256 salt; OfferItem[] offer; ConsiderationItem[] consideration; + uint256 totalOriginalConsiderationItems; } /** diff --git a/test/index.js b/test/index.js index 4ef489610..926853986 100644 --- a/test/index.js +++ b/test/index.js @@ -209,6 +209,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function zone: zone.address, offer, consideration, + totalOriginalConsiderationItems: consideration.length, orderType, salt, startTime, @@ -365,6 +366,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function ...x, recipient: offerer.address, })), + totalOriginalConsiderationItems: compressedOfferItems.length, orderType: 0, // FULL_OPEN salt, startTime, @@ -436,6 +438,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function .reduce((a, b) => a.add(b), ethers.BigNumber.from(0)) ) })), + totalOriginalConsiderationItems: order.parameters.offer.length, orderType: useProxy ? 4 : 0, // FULL_OPEN_VIA_PROXY or FULL_OPEN salt, startTime, From 5eed9e1abc8a211df3fcbadc8e428845814bd16f Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 10:30:03 -0700 Subject: [PATCH 31/39] include check on consideration array length for basic functions --- contracts/lib/ConsiderationInternal.sol | 45 +++++++++++---------- contracts/lib/ConsiderationInternalView.sol | 18 ++++----- contracts/lib/ConsiderationPure.sol | 41 +++++++++++++++---- 3 files changed, 66 insertions(+), 38 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 910ac5c0c..d82ff925d 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -59,26 +59,19 @@ contract ConsiderationInternal is ConsiderationInternalView { ) {} /** - * @dev Prepare fulfillment of a basic order with manual calldata - * and memory access. This calculates the order hash, emits the - * OrderFulfilled event, and asserts basic order validity. - * - * Note: Calldata offsets are validated in this function because it - * accesses constant calldata pointers for dynamic types that match - * default ABI encoding, but valid ABI encoding can use arbitrary offsets. - * Checking that the offsets were produced by default encoding will ensure - * that other functions using Solidity's calldata accessors (which - * calculate pointers from the stored offsets) are reading the same - * data as the order hash is derived from. - * - * Note: This function accesses memory directly. It does not clear the - * expanded memory regions used, nor does it update the free memory pointer, - * so other direct memory access must not assume that unused memory is empty. - * - * Assertions: - * - Non-reentrancy - * - Valid start and end times - * - Valid calldata encoding + * @dev Internal function to prepare fulfillment of a basic order with manual + * calldata and memory access. This calculates the order hash, emits an + * OrderFulfilled event, and asserts basic order validity. Note that + * calldata offsets must be validated as this function accesses constant + * calldata pointers for dynamic types that match default ABI encoding, + * but valid ABI encoding can use arbitrary offsets. Checking that the + * offsets were produced by default encoding will ensure that other + * functions using Solidity's calldata accessors (which calculate + * pointers from the stored offsets) are reading the same data as the + * order hash is derived from. Also note that This function accesses + * memory directly. It does not clear the expanded memory regions used, + * nor does it update the free memory pointer, so other direct memory + * access must not assume that unused memory is empty. */ function _prepareBasicFulfillmentFromCalldata( BasicOrderParameters calldata parameters, @@ -89,12 +82,20 @@ contract ConsiderationInternal is ConsiderationInternalView { ) internal returns (bytes32 orderHash, bool useOffererProxy) { // Ensure this function cannot be triggered during a reentrant call. _setReentrancyGuard(); + // Ensure current timestamp falls between order start time and end time. _assertValidTime(parameters.startTime, parameters.endTime); - // Ensure calldata offsets were produced by default encoding + + // Ensure calldata offsets were produced by default encoding. _assertValidBasicOrderParameterOffsets(); - { // Handle received items + // Ensure supplied consideration array length is not less than original. + _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength( + parameters.additionalRecipients.length + 1, + parameters.totalOriginalConsiderationItems + ); + + { // Load consideration item typehash from runtime code and place on stack. bytes32 typeHash = _CONSIDERATION_ITEM_TYPEHASH; assembly { diff --git a/contracts/lib/ConsiderationInternalView.sol b/contracts/lib/ConsiderationInternalView.sol index 2fb05660a..e66b3a4f5 100644 --- a/contracts/lib/ConsiderationInternalView.sol +++ b/contracts/lib/ConsiderationInternalView.sol @@ -356,8 +356,11 @@ contract ConsiderationInternalView is ConsiderationPure { } /** - * @dev Internal view function to retrieve the current nonce for a given - * order's offerer and zone and use that to derive the order hash. + * @dev Internal view function to to ensure that the supplied consideration + * array length on a given set of order parameters is not less than the + * original consideration array length for that order and to retrieve + * the current nonce for a given order's offerer and zone and use it to + * derive the order hash. * * @param orderParameters The parameters of the order to hash. * @@ -367,13 +370,10 @@ contract ConsiderationInternalView is ConsiderationPure { OrderParameters memory orderParameters ) internal view returns (bytes32) { // Ensure supplied consideration array length is not less than original. - if ( - orderParameters.consideration.length < ( - orderParameters.totalOriginalConsiderationItems - ) - ) { - revert MissingOriginalConsiderationItems(); - } + _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength( + orderParameters.consideration.length, + orderParameters.totalOriginalConsiderationItems + ); // Derive and return order hash using current nonce for offerer in zone. return _getOrderHash( diff --git a/contracts/lib/ConsiderationPure.sol b/contracts/lib/ConsiderationPure.sol index 26cbaee2c..266edd559 100644 --- a/contracts/lib/ConsiderationPure.sol +++ b/contracts/lib/ConsiderationPure.sol @@ -1297,16 +1297,39 @@ contract ConsiderationPure is ConsiderationBase { } /** - * @dev Validate calldata offsets for dynamic types in BasicOrderParameters. - * This ensures that functions using the calldata object normally will be - * using the same data as the assembly functions. - * Note: No parameters because all basic order functions use the same - * calldata encoding. + * @dev Internal pure function to ensure that the supplied consideration + * array length for an order to be fulfilled is not less than the + * original consideration array length for that order. + * + * @param suppliedConsiderationItemTotal The number of consideration items + * supplied when fulfilling the order. + * @param originalConsiderationItemTotal The number of consideration items + * supplied on initial order creation. + */ + function _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength( + uint256 suppliedConsiderationItemTotal, + uint256 originalConsiderationItemTotal + ) internal pure { + // Ensure supplied consideration array length is not less than original. + if (suppliedConsiderationItemTotal < originalConsiderationItemTotal) { + revert MissingOriginalConsiderationItems(); + } + } + + /** + * @dev Internal pure function to validate calldata offsets for dynamic + * types in BasicOrderParameters. This ensures that functions using the + * calldata object normally will be using the same data as the assembly + * functions. Note that no parameters are supplied as all basic order + * functions use the same calldata encoding. */ function _assertValidBasicOrderParameterOffsets() internal pure { + // Declare a boolean designating basic order parameter offset validity. bool validOffsets; + + // Utilize assembly in order to read offset data directly from calldata. assembly { - /* + /* * Checks: * 1. Order parameters struct offset = 0x20 * 2. Additional recipients arr offset = 0x1e0 @@ -1328,6 +1351,10 @@ contract ConsiderationPure is ConsiderationBase { ) ) } - if (!validOffsets) revert InvalidBasicOrderParameterEncoding(); + + // Revert with an error if basic order parameter offsets are invalid. + if (!validOffsets) { + revert InvalidBasicOrderParameterEncoding(); + } } } From 1dcd84e4e01648e15883f67ec4ed2bbe6e7f11fc Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 10:37:47 -0700 Subject: [PATCH 32/39] add "tipping" description to README with known limitation section --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index dc44c5947..36fcb5fb6 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Each order contains nine key components: - The `identifierOrCriteria` represents either the ERC721 or ERC1155 token identifier or, in the case of a criteria-based item type, a merkle root composed of the valid set of token identifiers for the item. This value will always be zero for Ether and ERC20 item types, and can optionally be zero for criteria-based item types to allow for any identifier. - The `startAmount` represents the amount of the item in question that will be required should the order be fulfilled at the moment the order becomes active. - The `endAmount` represents the amount of the item in question that will be required should the order be fulfilled at the moment the order expires. If this value differs from the item's `startAmount`, the realized amount is calculated linearly based on the time elapsed since the order became active. -- The `consideration` contains an array of items that must be received in order to fulfill the order. It contains all of the same components as an offered item, and additionally includes a `recipient` that will receive each item. +- The `consideration` contains an array of items that must be received in order to fulfill the order. It contains all of the same components as an offered item, and additionally includes a `recipient` that will receive each item. This array may be extended by the fulfiller on order fulfillment so as to support "tipping" (e.g. relayer or referral payments). - The `orderType` designates one of eight types for the order depending on three distinct preferences: - `FULL` indicates that the order does not support partial fills, whereas `PARTIAL` enables filling some fraction of the order, with the important caveat that each item must be cleanly divisible by the supplied fraction (i.e. no remainder after division). - `OPEN` indicates that the call to execute the order can be submitted by any account, whereas `RESTRICTED` requires that the order can only be executed by either the offerer or the zone of the order. @@ -149,6 +149,7 @@ When matching a group of orders via `matchOrders` or `matchAdvancedOrders`, step - As all items on orders supporting partial fills must be "cleanly divisible" when performing a partial fill, orders with multiple items should to be constructed with care. A straightforward heuristic is to start with a "unit" bundle (e.g. 1 NFT item A, 3 NFT item B, and 5 NFT item C for 2 ETH) then applying a multiple to that unit bundle (e.g. 7 of those units results in a partial order for 7 NFT item A, 21 NFT item B, and 35 NFT item C for 14 ETH). - As Ether cannot be "taken" from an account, any order that contains Ether or other native tokens as an offer item (including "implied" mirror orders) must be supplied by the caller executing the order(s) as msg.value. This also explains why there are no `fulfillBasicERC721ForEthOrder` and `fulfillBasicERC1155ForEthOrder` functions, as Ether cannot be taken from the offerer in these cases. One important takeaway from this mechanic is that, technically, anyone can supply Ether on behalf of a given offerer (whereas the offerer themselves must supply all other items). - As restricted orders must be fulfilled by the named zone, multiple restricted orders with differing zones cannot be composed via `matchOrders` — they must either be fulfilled independently or make use of a shared zone that routes orders in accordance with the requirements of each original zone. +- As extensions to the consideration array on fulfillment (i.e. "tipping") can be arbitrarily set by the caller, fulfillments where all matched orders have already been signed for or validated can be frontrun on submission, with the frontrunner modifying any tips. Therefore, it is important that orders fulfilled in this manner either leverage "restricted" order types that route through a zone that enforces appropriate allocation of consideration extensions, or that each offer item is fully spent and each consideration item is appropriately declared on order creation. - As each consideration item on each order fulfilled via `matchOrders` must be met in order for the group of orders to be matchable, any failing transfer will cause the entire group to fail. Two workarounds are available in cases where failing order fulfillment should not prevent the rest of the desired orders from being successfully fulfilled: - The orders can be broken up into distinct calls and submitted via a multicall contract or flashbots bundle. This method does not allow for cross-order fulfillment aggregation and will also require the fulfiller to sign for each order in the multicall contract case. - The orders can be routed through a zone or wrapper contract that determines whether the offered items on each order are still spendable, and filters out unfulfillable orders when composing the final group before providing them to Consideration. This approach allows for cross-order fulfillment aggregation, with the tradeoff of either needing to supply adequate item balances and approvals to the zone or wrapper for items offered by the fulfiller, or to sign for each order as above. This method would add appreciable gas overhead for larger order groups, in many cases negating the gas savings resulting from fulfillment aggregation. From b67c8ad238f7f145d58776a982cf3054103c5ccb Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 14:04:59 -0700 Subject: [PATCH 33/39] update some tests (one still skipped) --- test/index.js | 208 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) diff --git a/test/index.js b/test/index.js index 926853986..bbdd93528 100644 --- a/test/index.js +++ b/test/index.js @@ -1460,6 +1460,76 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }); }); + it("ERC721 <=> ETH (standard with tip)", async () => { + // Seller mints nft + const nftId = ethers.BigNumber.from(randomHex()); + await testERC721.mint(seller.address, nftId); + + // Seller approves marketplace contract to transfer NFT + await whileImpersonating(seller.address, provider, async () => { + await expect(testERC721.connect(seller).setApprovalForAll(marketplaceContract.address, true)) + .to.emit(testERC721, "ApprovalForAll") + .withArgs(seller.address, marketplaceContract.address, true); + }); + + const offer = [ + { + itemType: 2, // ERC721 + token: testERC721.address, + identifierOrCriteria: nftId, + startAmount: ethers.BigNumber.from(1), + endAmount: ethers.BigNumber.from(1), + }, + ]; + + const consideration = [ + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("10"), + endAmount: ethers.utils.parseEther("10"), + recipient: seller.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("1"), + endAmount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + ]; + + const { order, orderHash, value } = await createOrder( + seller, + zone, + offer, + consideration, + 0, // FULL_OPEN + ); + + // Add a tip + order.parameters.consideration.push( + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("1"), + endAmount: ethers.utils.parseEther("1"), + recipient: owner.address, + }, + ); + + await whileImpersonating(buyer.address, provider, async () => { + await withBalanceChecks([order], 0, null, async () => { + const tx = await marketplaceContract.connect(buyer).fulfillOrder(order, false, {value: value.add(ethers.utils.parseEther("1"))}); + const receipt = await tx.wait(); + await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); + return receipt; + }); + }); + }); it("ERC721 <=> ETH (basic)", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); @@ -1528,6 +1598,72 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }); }); + it.skip("ERC721 <=> ETH (basic with tip)", async () => { + // Seller mints nft + const nftId = ethers.BigNumber.from(randomHex()); + await testERC721.mint(seller.address, nftId); + + // Seller approves marketplace contract to transfer NFT + await whileImpersonating(seller.address, provider, async () => { + await expect(testERC721.connect(seller).setApprovalForAll(marketplaceContract.address, true)) + .to.emit(testERC721, "ApprovalForAll") + .withArgs(seller.address, marketplaceContract.address, true); + }); + + const offer = [ + { + itemType: 2, // ERC721 + token: testERC721.address, + identifierOrCriteria: nftId, + startAmount: ethers.BigNumber.from(1), + endAmount: ethers.BigNumber.from(1), + }, + ]; + + const consideration = [ + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("10"), + endAmount: ethers.utils.parseEther("10"), + recipient: seller.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("1"), + endAmount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + ]; + + const { order, orderHash, value } = await createOrder( + seller, + zone, + offer, + consideration, + 0, // FULL_OPEN + ); + + const basicOrderParameters = getBasicOrderParameters(order); + + // Include the tip + basicOrderParameters.additionalRecipients.push({ + amount: ethers.utils.parseEther("1"), + recipient: owner.address, + }); + + await whileImpersonating(buyer.address, provider, async () => { + await withBalanceChecks([order], 0, null, async () => { + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, { value: value.add(ethers.utils.parseEther("1")) }); + const receipt = await tx.wait(); + await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); + return receipt; + }); + }); + }); it("ERC721 <=> ETH (basic via proxy)", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); @@ -8544,6 +8680,78 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function await expect(marketplaceContract.connect(buyer).fulfillAdvancedOrder(order, [], false, {value})).to.be.reverted; }); }); + it("Reverts on inadequate consideration items", async () => { + // Seller mints nft + const nftId = ethers.BigNumber.from(randomHex()); + const amount = ethers.BigNumber.from(randomHex().slice(0, 5)); + await testERC1155.mint(seller.address, nftId, amount.mul(10000)); + + // Seller approves marketplace contract to transfer NFTs + await whileImpersonating(seller.address, provider, async () => { + await expect(testERC1155.connect(seller).setApprovalForAll(marketplaceContract.address, true)) + .to.emit(testERC1155, "ApprovalForAll") + .withArgs(seller.address, marketplaceContract.address, true); + }); + + const offer = [ + { + itemType: 3, // ERC1155 + token: testERC1155.address, + identifierOrCriteria: nftId, + startAmount: amount.mul(10), + endAmount: amount.mul(10), + }, + ]; + + const consideration = [ + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: amount.mul(1000), + endAmount: amount.mul(1000), + recipient: seller.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: amount.mul(10), + endAmount: amount.mul(10), + recipient: zone.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: amount.mul(20), + endAmount: amount.mul(20), + recipient: owner.address, + }, + ]; + + const { order, orderHash, value } = await createOrder( + seller, + zone, + offer, + consideration, + 1, // PARTIAL_OPEN + ); + + // Remove a consideration item + order.parameters.consideration.pop(); + + let orderStatus = await marketplaceContract.getOrderStatus(orderHash); + + expect(orderStatus.isCancelled).to.equal(false); + expect(orderStatus.isValidated).to.equal(false); + expect(orderStatus.totalFilled).to.equal(0); + expect(orderStatus.totalSize).to.equal(0); + + await whileImpersonating(buyer.address, provider, async () => { + await expect(marketplaceContract.connect(buyer).fulfillAdvancedOrder(order, [], false, {value})).to.be.reverted; + }); + }); it("Reverts on invalid submitter when required by order", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); From 62611793155dce7c0558b7c82415e8cafe563e99 Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Tue, 5 Apr 2022 17:30:28 -0500 Subject: [PATCH 34/39] attempt at fixing basic order fulfillment prep fn --- contracts/lib/ConsiderationInternal.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 1335c3226..de03d7c29 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -131,7 +131,7 @@ contract ConsiderationInternal is ConsiderationInternalView { let len := calldataload(0x224) // END_ARR + 0x120 = 0x2a0 + len*0x20 let eventArrPtr := add(0x2a0, mul(0x20, len)) - mstore(eventArrPtr, add(len, 1)) // length + mstore(eventArrPtr, add(calldataload(0x1c4), 1)) // length // Set ptr to data portion of first ReceivedItem eventArrPtr := add(eventArrPtr, 0x20) // Write item type @@ -174,6 +174,11 @@ contract ConsiderationInternal is ConsiderationInternalView { // Copy endAmount, recipient calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) } + /* 4. Hash packed array of ConsiderationItem EIP712 hashes */ + // note: Store at 0x60 - all other memory begins at 0x80 + // keccak256(abi.encodePacked(receivedItemHashes)) + mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) + /* 5. Write tips to event data */ len := calldataload(0x224) for {} lt(i, len) {i := add(i, 1)} { let additionalRecipientCdPtr := add(0x244, mul(0x40, i)) @@ -189,10 +194,6 @@ contract ConsiderationInternal is ConsiderationInternalView { // Copy endAmount, recipient calldatacopy(add(eventArrPtr, 0x60), additionalRecipientCdPtr, 0x40) } - /* 4. Hash packed array of ConsiderationItem EIP712 hashes */ - // note: Store at 0x60 - all other memory begins at 0x80 - // keccak256(abi.encodePacked(receivedItemHashes)) - mstore(0x60, keccak256(0x160, mul(add(len, 1), 32))) } } From 5558ea4ce2a391a28dc4705aecca74c6e07b456a Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Tue, 5 Apr 2022 17:30:42 -0500 Subject: [PATCH 35/39] update test for tips --- test/index.js | 88 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 80 insertions(+), 8 deletions(-) diff --git a/test/index.js b/test/index.js index 5f9fe18bf..f29c8d50c 100644 --- a/test/index.js +++ b/test/index.js @@ -42,7 +42,8 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function const getBasicOrderParameters = ( order, - useFulfillerProxy = false + useFulfillerProxy = false, + tips = [] ) => ({ offerer: order.parameters.offerer, zone: order.parameters.zone, @@ -56,15 +57,15 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, - originalConsiderationsCount: order.parameters.consideration.length, + originalConsiderationsCount: order.parameters.consideration.length - 1, signature: order.signature, useFulfillerProxy, - additionalRecipients: order.parameters.consideration - .slice(1) - .map(({ endAmount, recipient }) => ({ - amount: endAmount, - recipient, - })), + additionalRecipients: [ + ...order.parameters.consideration + .slice(1) + .map(({ endAmount, recipient }) => ({ amount: endAmount, recipient, })), + ...tips, + ], }); const convertSignatureToEIP2098 = (signature) => { @@ -1525,6 +1526,77 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }); }); + it("ERC721 <=> ETH (basic with tips)", async () => { + // Seller mints nft + const nftId = ethers.BigNumber.from(randomHex()); + await testERC721.mint(seller.address, nftId); + + // Seller approves marketplace contract to transfer NFT + await whileImpersonating(seller.address, provider, async () => { + await expect(testERC721.connect(seller).setApprovalForAll(marketplaceContract.address, true)) + .to.emit(testERC721, "ApprovalForAll") + .withArgs(seller.address, marketplaceContract.address, true); + }); + + const offer = [ + { + itemType: 2, // ERC721 + token: testERC721.address, + identifierOrCriteria: nftId, + startAmount: ethers.BigNumber.from(1), + endAmount: ethers.BigNumber.from(1), + }, + ]; + + const consideration = [ + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("10"), + endAmount: ethers.utils.parseEther("10"), + recipient: seller.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("1"), + endAmount: ethers.utils.parseEther("1"), + recipient: zone.address, + }, + { + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("1"), + endAmount: ethers.utils.parseEther("1"), + recipient: owner.address, + }, + ]; + + const { order, orderHash, value } = await createOrder( + seller, + zone, + offer, + consideration, + 0, // FULL_OPEN + ); + + const basicOrderParameters = getBasicOrderParameters(order, false, [{ + amount: ethers.utils.parseEther("2"), + recipient: owner.address + }]); + + await whileImpersonating(buyer.address, provider, async () => { + await withBalanceChecks([order], 0, null, async () => { + const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, { value: value.add(ethers.utils.parseEther("2")) }); + const receipt = await tx.wait(); + await checkExpectedEvents(receipt, [{order, orderHash, fulfiller: buyer.address}]); + return receipt; + }); + }); + }); it("ERC721 <=> ETH (basic via proxy)", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); From 49dc4c3da4ffc481f4d76e5b90fafea122ecb9ba Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Tue, 5 Apr 2022 17:38:07 -0500 Subject: [PATCH 36/39] rename totalOriginalConsiderationItems to totalOriginalAdditionalRecipients in basic order params --- contracts/lib/ConsiderationInternal.sol | 2 +- contracts/lib/ConsiderationStructs.sol | 2 +- test/index.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 2c4a99222..517e8a7f8 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -92,7 +92,7 @@ contract ConsiderationInternal is ConsiderationInternalView { // Ensure supplied consideration array length is not less than original. _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength( parameters.additionalRecipients.length + 1, - parameters.totalOriginalConsiderationItems + parameters.totalOriginalAdditionalRecipients ); { // Load consideration item typehash from runtime code and place on stack. diff --git a/contracts/lib/ConsiderationStructs.sol b/contracts/lib/ConsiderationStructs.sol index 3802168a0..42ecced11 100644 --- a/contracts/lib/ConsiderationStructs.sol +++ b/contracts/lib/ConsiderationStructs.sol @@ -103,7 +103,7 @@ struct BasicOrderParameters { // calldata offset uint256 endTime; // 0x164 uint256 salt; // 0x184 bool useFulfillerProxy; // 0x1a4 - uint256 totalOriginalConsiderationItems; // 0c1c4 + uint256 totalOriginalAdditionalRecipients; // 0c1c4 AdditionalRecipient[] additionalRecipients; // 0x1e4 bytes signature; // 0x204 // Total length, excluding dynamic array data: 0x224 (548) diff --git a/test/index.js b/test/index.js index f201512f8..4e3b3dac1 100644 --- a/test/index.js +++ b/test/index.js @@ -57,7 +57,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function startTime: order.parameters.startTime, endTime: order.parameters.endTime, salt: order.parameters.salt, - totalOriginalConsiderationItems: order.parameters.consideration.length - 1, + totalOriginalAdditionalRecipients: order.parameters.consideration.length - 1, signature: order.signature, useFulfillerProxy, additionalRecipients: [ From 3f2c5fad444186531563f1b03ad4f47b46382e0b Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 15:57:33 -0700 Subject: [PATCH 37/39] poke at failing test --- README.md | 1 - contracts/lib/ConsiderationInternal.sol | 2 +- test/index.js | 11 ++++++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 36fcb5fb6..ada1d71ac 100644 --- a/README.md +++ b/README.md @@ -157,7 +157,6 @@ When matching a group of orders via `matchOrders` or `matchAdvancedOrders`, step ## Feature Wishlist 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. - **General gas efficiency and code size optimizations.** diff --git a/contracts/lib/ConsiderationInternal.sol b/contracts/lib/ConsiderationInternal.sol index 517e8a7f8..0dc7bd53b 100644 --- a/contracts/lib/ConsiderationInternal.sol +++ b/contracts/lib/ConsiderationInternal.sol @@ -132,7 +132,7 @@ contract ConsiderationInternal is ConsiderationInternalView { let len := calldataload(0x224) // END_ARR + 0x120 = 0x2a0 + len*0x20 let eventArrPtr := add(0x2a0, mul(0x20, len)) - mstore(eventArrPtr, add(calldataload(0x1c4), 1)) // length + mstore(eventArrPtr, add(calldataload(0x224), 1)) // length // Set ptr to data portion of first ReceivedItem eventArrPtr := add(eventArrPtr, 0x20) // Write item type diff --git a/test/index.js b/test/index.js index 4e3b3dac1..c820afd65 100644 --- a/test/index.js +++ b/test/index.js @@ -1599,7 +1599,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }); }); - it("ERC721 <=> ETH (basic with tips)", async () => { + it.only("ERC721 <=> ETH (basic with tips)", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); await testERC721.mint(seller.address, nftId); @@ -1661,6 +1661,15 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function recipient: owner.address }]); + order.parameters.consideration.push({ + itemType: 0, // ETH + token: constants.AddressZero, + identifierOrCriteria: 0, // ignored for ETH + startAmount: ethers.utils.parseEther("2"), + endAmount: ethers.utils.parseEther("2"), + recipient: owner.address, + }); + await whileImpersonating(buyer.address, provider, async () => { await withBalanceChecks([order], 0, null, async () => { const tx = await marketplaceContract.connect(buyer).fulfillBasicEthForERC721Order(basicOrderParameters, { value: value.add(ethers.utils.parseEther("2")) }); From 6d9f65e10c3ab31dadd3159c475325ea77bf4d1b Mon Sep 17 00:00:00 2001 From: d1ll0n Date: Tue, 5 Apr 2022 18:09:52 -0500 Subject: [PATCH 38/39] set tip recipient to 0x01 --- test/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/index.js b/test/index.js index c820afd65..d72bb11a2 100644 --- a/test/index.js +++ b/test/index.js @@ -1599,7 +1599,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function }); }); }); - it.only("ERC721 <=> ETH (basic with tips)", async () => { + it("ERC721 <=> ETH (basic with tips)", async () => { // Seller mints nft const nftId = ethers.BigNumber.from(randomHex()); await testERC721.mint(seller.address, nftId); @@ -1658,7 +1658,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function const basicOrderParameters = getBasicOrderParameters(order, false, [{ amount: ethers.utils.parseEther("2"), - recipient: owner.address + recipient: `0x0000000000000000000000000000000000000001` }]); order.parameters.consideration.push({ @@ -1667,7 +1667,7 @@ describe(`Consideration (version: ${VERSION}) — initial test suite`, function identifierOrCriteria: 0, // ignored for ETH startAmount: ethers.utils.parseEther("2"), endAmount: ethers.utils.parseEther("2"), - recipient: owner.address, + recipient: `0x0000000000000000000000000000000000000001` }); await whileImpersonating(buyer.address, provider, async () => { From 1253a6b0caf9e48cade4e2676d00c16a2b4051a4 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Tue, 5 Apr 2022 16:24:55 -0700 Subject: [PATCH 39/39] clean up some comments and TODOs --- contracts/Consideration.sol | 6 -- .../interfaces/ConsiderationInterface.sol | 79 +++++++++---------- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/contracts/Consideration.sol b/contracts/Consideration.sol index d7f798928..663c6ab55 100644 --- a/contracts/Consideration.sol +++ b/contracts/Consideration.sol @@ -66,7 +66,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicEthForERC721Order( BasicOrderParameters calldata parameters ) external payable override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -115,7 +114,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicEthForERC1155Order( BasicOrderParameters calldata parameters ) external payable override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -163,7 +161,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC20ForERC721Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -215,7 +212,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC20ForERC1155Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. (, bool useOffererProxy) = _prepareBasicFulfillmentFromCalldata( parameters, @@ -269,7 +265,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC721ForERC20Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. _prepareBasicFulfillmentFromCalldata( parameters, @@ -323,7 +318,6 @@ contract Consideration is ConsiderationInterface, ConsiderationInternal { function fulfillBasicERC1155ForERC20Order( BasicOrderParameters calldata parameters ) external override returns (bool) { - // todo: is it necessary to do validation on parameters? // Derive and validate order using parameters and update order status. _prepareBasicFulfillmentFromCalldata( parameters, diff --git a/contracts/interfaces/ConsiderationInterface.sol b/contracts/interfaces/ConsiderationInterface.sol index 74cad3c8d..b660115c1 100644 --- a/contracts/interfaces/ConsiderationInterface.sol +++ b/contracts/interfaces/ConsiderationInterface.sol @@ -26,18 +26,17 @@ import { * Consideration. */ interface ConsiderationInterface { - // todo: review basic order fn natspec /** - * @notice Fulfill an order offering a single ERC721 token by supplying - * Ether (or the native token for the given chain) as consideration - * for the order. An arbitrary number of "additional recipients" may - * also be supplied which will each receive the native token from - * the fulfiller as consideration. + * @notice Fulfill an order offering an ERC721 token by supplying Ether (or + * the native token for the given chain) as consideration for the + * order. An arbitrary number of "additional recipients" may also be + * supplied which will each receive native tokens from the fulfiller + * as consideration. * - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract (or - * their proxy if indicated by the order) in order for - * their offered ERC721 token to be transferred. + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC721 token to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -51,10 +50,11 @@ interface ConsiderationInterface { * order. An arbitrary number of "additional recipients" may also be * supplied which will each receive native tokens from the fulfiller * as consideration. - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract - * (or their proxy if indicated by the order) in order - * for their offered ERC1155 tokens to be transferred. + * + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC1155 tokens to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -68,10 +68,10 @@ interface ConsiderationInterface { * recipients" may also be supplied which will each receive ERC20 * tokens from the fulfiller as consideration. * - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract (or - * their proxy if indicated by the order) in order for - * their offered ERC721 token to be transferred. + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC721 token to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -80,15 +80,15 @@ interface ConsiderationInterface { ) external returns (bool); /** - * @notice Fulfill an order offering some amount of a specific ERC1155 token - * by supplying ERC20 tokens as consideration. An arbitrary number - * of "additional recipients" may also be supplied which will each - * receive ERC20 tokens from the fulfiller as consideration. + * @notice Fulfill an order offering ERC1155 tokens by supplying ERC20 + * tokens as consideration. An arbitrary number of "additional + * recipients" may also be supplied which will each receive ERC20 + * tokens from the fulfiller as consideration. * - * @param parameters Additional information on the fulfilled order. Note - * that the offerer must first approve this contract - * (or their proxy if indicated by the order) in order - * for their offered ERC1155 tokens to be transferred. + * @param parameters Additional information on the fulfilled order. Note + * that the offerer must first approve this contract (or + * their proxy if indicated by the order) in order for + * their offered ERC1155 tokens to be transferred. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -101,13 +101,13 @@ interface ConsiderationInterface { * ERC721 token as consideration. An arbitrary number of "additional * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. - * @param parameters Additional information on the fulfilled order. - * Note that the fulfiller must first approve this - * contract (or their proxy if indicated by the - * order) before the ERC721 token required as - * consideration can be transferred. Also note that - * the sum of all additional recipient amounts - * cannot exceed `erc20Amount`. + * + * @param parameters Additional information on the fulfilled order. Note + * that the fulfiller must first approve this contract (or + * their proxy if indicated by the order) before the + * ERC721 token required as consideration can be + * transferred. Also note that the sum of all additional + * recipient amounts cannot exceed `erc20Amount`. * * @return A boolean indicating whether the order has been fulfilled. */ @@ -121,13 +121,12 @@ interface ConsiderationInterface { * recipients" may also be supplied which will each receive ERC20 * tokens from the offerer as consideration. * - * @param parameters Additional information on the fulfilled order. - * Note that the fulfiller must first approve this - * contract (or their proxy if indicated by the - * order) before the ERC1155 token required as - * consideration can be transferred. Also note that - * the sum of all additional recipient amounts - * cannot exceed `erc20Amount`. + * @param parameters Additional information on the fulfilled order. Note + * that the fulfiller must first approve this contract (or + * their proxy if indicated by the order) before the + * ERC1155 token required as consideration can be + * transferred. Also note that the sum of all additional + * recipient amounts cannot exceed `erc20Amount`. * * @return A boolean indicating whether the order has been fulfilled. */