From b1e7b553d6242b1cb1f7cd66d2cf946eb0c9abd4 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 29 Feb 2024 12:40:59 -0500 Subject: [PATCH 1/2] clean up a duplicate assignment in OrderCombiner and add some comments --- src/core/lib/OrderCombiner.sol | 35 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/core/lib/OrderCombiner.sol b/src/core/lib/OrderCombiner.sol index ab9e463..be8a162 100644 --- a/src/core/lib/OrderCombiner.sol +++ b/src/core/lib/OrderCombiner.sol @@ -410,13 +410,15 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { ) ); + // Set the start amount as equal to the current amount. considerationItem.startAmount = currentAmount; // Utilize assembly to manually "shift" the recipient value, // then to copy the start amount to the recipient. // Note that this sets up the memory layout that is // subsequently relied upon by - // _aggregateValidFulfillmentConsiderationItems. + // _aggregateValidFulfillmentConsiderationItems as well as + // during comparison against generated contract orders. assembly { // Derive the pointer to the recipient using the item // pointer along with the offset to the recipient. @@ -471,12 +473,7 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { // Iterate over each order. for (uint256 i = OneWord; i < terminalMemoryOffset; i += OneWord) { - AdvancedOrder memory advancedOrder = ( - _getReadAdvancedOrderByOffset( - // MemoryPointerLib.pptrOffset - )(advancedOrders, i) - ); - + // Retrieve order hash, bypassing out-of-range check. assembly { orderHash := mload(add(orderHashes, i)) } @@ -486,25 +483,28 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { continue; } - // Retrieve order using assembly to bypass out-of-range check. - assembly { - advancedOrder := mload(add(advancedOrders, i)) - } + // Retrieve the order by index, bypassing out-of-range check. + AdvancedOrder memory advancedOrder = ( + _getReadAdvancedOrderByOffset()(advancedOrders, i) + ); // Determine if max number orders have already been fulfilled. if (maximumFulfilled == 0) { + // If so, set the order hash to zero. assembly { mstore(add(orderHashes, i), 0) } + // Set the numerator to zero to signal to skip the order. advancedOrder.numerator = 0; // Continue iterating through the remaining orders. continue; } - // Update order status as long as some fraction is available. + // Handle final checks and status updates based on order type. if (advancedOrder.parameters.orderType != OrderType.CONTRACT) { + // Check authorization for restricted orders. if ( !_checkRestrictedAdvancedOrderAuthorization( advancedOrder, @@ -514,15 +514,19 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { revertOnInvalid ) ) { + // If authorization check fails, set order hash to zero. assembly { mstore(add(orderHashes, i), 0) } + // Set numerator to zero to signal to skip the order. advancedOrder.numerator = 0; + // Continue iterating through the remaining orders. continue; } + // Update status as long as some fraction is available. if (!_updateStatus( orderHash, advancedOrder.numerator, @@ -532,12 +536,15 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { revertOnInvalid ) )) { + // If status update fails, set the order hash to zero. assembly { mstore(add(orderHashes, i), 0) } + // Set numerator to zero to signal to skip the order. advancedOrder.numerator = 0; + // Continue iterating through the remaining orders. continue; } } else { @@ -550,13 +557,17 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { revertOnInvalid ); + // Write the derived order hash to the order hashes array. assembly { mstore(add(orderHashes, i), orderHash) } + // Handle invalid orders, indicated by a zero order hash. if (orderHash == bytes32(0)) { + // Set numerator to zero to signal to skip the order. advancedOrder.numerator = 0; + // Continue iterating through the remaining orders. continue; } } From c96dcd41d65e98c2fac69beb09e4866033240f01 Mon Sep 17 00:00:00 2001 From: 0age <0age@protonmail.com> Date: Thu, 29 Feb 2024 13:12:09 -0500 Subject: [PATCH 2/2] more cleanup and small optimizations --- src/core/lib/LowLevelHelpers.sol | 31 ++++++++++++- src/core/lib/OrderCombiner.sol | 80 +++++++++++++++++++++----------- 2 files changed, 82 insertions(+), 29 deletions(-) diff --git a/src/core/lib/LowLevelHelpers.sol b/src/core/lib/LowLevelHelpers.sol index 8f73bcf..3a010fd 100644 --- a/src/core/lib/LowLevelHelpers.sol +++ b/src/core/lib/LowLevelHelpers.sol @@ -16,7 +16,10 @@ import { MemoryPointerLib } from "seaport-types/src/helpers/PointerLibraries.sol"; -import { AdvancedOrder } from "seaport-types/src/lib/ConsiderationStructs.sol"; +import { + AdvancedOrder, + Execution +} from "seaport-types/src/lib/ConsiderationStructs.sol"; /** * @title LowLevelHelpers @@ -144,6 +147,32 @@ contract LowLevelHelpers { } } + /** + * @dev Internal pure function to cast the `pptrOffset` function from + * `MemoryPointerLib` to a function that takes a memory array of + * `Execution` and an offset in memory and returns the + * `Execution` whose pointer is stored at that offset from the + * array length. + */ + function _getReadExecutionByOffset( + ) internal pure returns ( + function ( + Execution[] memory, + uint256 + ) internal pure returns (Execution memory) fn2 + ) { + function ( + MemoryPointer, + uint256 + ) internal pure returns ( + MemoryPointer + ) fn1 = MemoryPointerLib.pptrOffset; + + assembly { + fn2 := fn1 + } + } + /** * @dev Internal pure function to return a `true` value that solc * will not recognize as a compile time constant. diff --git a/src/core/lib/OrderCombiner.sol b/src/core/lib/OrderCombiner.sol index be8a162..70db10d 100644 --- a/src/core/lib/OrderCombiner.sol +++ b/src/core/lib/OrderCombiner.sol @@ -25,11 +25,16 @@ import { FulfillmentApplier } from "./FulfillmentApplier.sol"; import { _revertConsiderationNotMet, - _revertInsufficientNativeTokensSupplied, _revertInvalidNativeOfferItem, _revertNoSpecifiedOrdersAvailable } from "seaport-types/src/lib/ConsiderationErrors.sol"; +import { + Error_selector_offset, + InsufficientNativeTokensSupplied_error_selector, + InsufficientNativeTokensSupplied_error_length +} from "seaport-types/src/lib/ConsiderationErrorConstants.sol"; + import { AccumulatorDisarmed, ConsiderationItem_recipient_offset, @@ -258,9 +263,7 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { // Retrieve order using pointer libraries to bypass out-of-range // check & cast function to avoid additional memory allocation. AdvancedOrder memory advancedOrder = ( - _getReadAdvancedOrderByOffset( - // MemoryPointerLib.pptrOffset - )(advancedOrders, i) + _getReadAdvancedOrderByOffset()(advancedOrders, i) ); // Validate it, update status, and determine fraction to fill. @@ -483,7 +486,8 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { continue; } - // Retrieve the order by index, bypassing out-of-range check. + // Retrieve order using pointer libraries to bypass out-of-range + // check & cast function to avoid additional memory allocation. AdvancedOrder memory advancedOrder = ( _getReadAdvancedOrderByOffset()(advancedOrders, i) ); @@ -795,29 +799,54 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { // accessed and modified, however. bytes memory accumulator = new bytes(AccumulatorDisarmed); - { - // Declare a variable for the available native token balance. - uint256 nativeTokenBalance; - - // Retrieve the length of the executions array and place on stack. - uint256 totalExecutions = executions.length; + // Skip overflow check: loop index & executions length are both bounded. + unchecked { + // Determine the memory offset to terminate on during loops. + uint256 terminalMemoryOffset = ( + (executions.length + 1) << OneWordShift + ); // Iterate over each execution. - for (uint256 i = 0; i < totalExecutions;) { - // Retrieve the execution and the associated received item. - Execution memory execution = executions[i]; + for (uint256 i = OneWord; i < terminalMemoryOffset; i += OneWord) { + // Get execution using pointer libraries to bypass out-of-range + // check & cast function to avoid additional memory allocation. + Execution memory execution = ( + _getReadExecutionByOffset()(executions, i) + ); + + // Retrieve the associated received item. ReceivedItem memory item = execution.item; - // If execution transfers native tokens, reduce value available. - if (item.itemType == ItemType.NATIVE) { - // Get the current available balance of native tokens. - assembly { - nativeTokenBalance := selfbalance() - } + // If execution transfers native tokens, check available value. + assembly { + // Ensure sufficient available balance for native transfers. + if and( + iszero(mload(item)), // itemType == ItemType.NATIVE + // item.amount > address(this).balance + gt( + mload( + add( + item, + ReceivedItem_amount_offset + ) + ), + selfbalance() + ) + ) { + // Store left-padded selector with push4, + // mem[28:32] = selector + mstore( + 0, + InsufficientNativeTokensSupplied_error_selector + ) - // Ensure that sufficient native tokens are still available. - if (item.amount > nativeTokenBalance) { - _revertInsufficientNativeTokensSupplied(); + // revert(abi.encodeWithSignature( + // "InsufficientNativeTokensSupplied()" + // )) + revert( + Error_selector_offset, + InsufficientNativeTokensSupplied_error_length + ) } } @@ -825,11 +854,6 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier { _transfer( item, execution.offerer, execution.conduitKey, accumulator ); - - // Skip overflow check as for loop is indexed starting at zero. - unchecked { - ++i; - } } }