OpenSea Seaport contest - Chom's results

Total: 4

[Not Aligned with Reference] Comment said that "Set the offerer as the receipient if execution amount is nonzero." but the code done opposite.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/contracts/lib/FulfillmentApplier.sol#L180-L183

_aggregateAvailable function of ReferenceFulfillmentApplier

            // Set the offerer as the receipient if execution amount is nonzero.
            if (execution.item.amount == 0) {
                execution.item.recipient = payable(execution.offerer);
            }

Notice that comment said "if execution amount is nonzero." but code is if (execution.item.amount == 0) {

The correct comment is "if execution amount is zero."

This section also doesn't appear in ReferenceFulfillmentApplier

[Testcase] A testcase is missing considerations

https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/test/index.js#L8569

ERC1155 <=> ETH (match, single item) case

const consideration = [];

Clearly wrong, consideration is empty but it should have ETH to be aligned with testcase title.

[Validation] Add validation on empty offer on consideration to prevent user accident that give hacker free token or NFT.

As above testcase is accidentally missing consideration. Wouldn't it be great to secure it in contract at first? To prevent user error which lead to losing of the fund.

[Testcase] Lack of testcase on ERC1155 <=> ERC1155 trade

I cannot find testcase where both offer has a ERC1155 and consideration has a ERC1155.

Total: 2 topics

Remainder check in _getFraction in AmountDeriver can be simplified to if iszero... revert

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/AmountDeriver.sol#L101-L111

        // Ensure fraction can be applied to the value with no remainder. Note
        // that the denominator cannot be zero.
        bool exact;
        assembly {
            // Ensure new value contains no remainder via mulmod operator.
            // Credit to @hrkrshnn + @axic for proposing this optimal solution.
            exact := iszero(mulmod(value, numerator, denominator))
        }

        // Ensure that division gave a final result with no remainder.
        if (!exact) {
            revert InexactFraction();
        }

Can be simplified into

        assembly {
            if iszero(mulmod(value, numerator, denominator)) {
                mstore(0, InexactFraction_error_signature)
                revert(0, InexactFraction_error_len)
            }
        }

Where InexactFraction_error_signature and InexactFraction_error_len is precomputed and stored as constant based on revert InexactFraction();

This reduce gas on storing exact flag and duplicated if (!exact) check

_applyFulfillment in FulfillmentApplier can set execution.item.recipient = payable(execution.offerer); if execution.item.amount == 0 to reduce gas on processing zero amount execution

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/FulfillmentApplier.sol#L115-L120

We can add

            // Set the offerer as the receipient if execution amount is zero.
            if (execution.item.amount == 0) {
                execution.item.recipient = payable(execution.offerer);
            }

Before returning into

        // Reuse execution struct with consideration amount and recipient.
        execution.item.amount = considerationItem.amount;
        execution.item.recipient = considerationItem.recipient;

        // Return the final execution that will be triggered for relevant items.
        return execution; // Execution(considerationItem, offerer, conduitKey);

which result in

        // Reuse execution struct with consideration amount and recipient.
        execution.item.amount = considerationItem.amount;
        execution.item.recipient = considerationItem.recipient;

        // Set the offerer as the receipient if execution amount is zero.
        if (execution.item.amount == 0) {
            execution.item.recipient = payable(execution.offerer);
        }

        // Return the final execution that will be triggered for relevant items.
        return execution; // Execution(considerationItem, offerer, conduitKey);

To let execution with zero amount being skipped in _executeAvailableFulfillments function in OrderCombiner

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L479-L494

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderCombiner.sol#L504-L521

                // Derive aggregated execution corresponding with fulfillment.
                Execution memory execution = _aggregateAvailable(
                    advancedOrders,
                    Side.OFFER,
                    components,
                    fulfillerConduitKey
                );

                // If offerer and recipient on the execution are the same...
                if (execution.item.recipient == execution.offerer) {
                    // increment total filtered executions.
                    totalFilteredExecutions += 1;
                } else {
                    // Otherwise, assign the execution to the executions array.
                    executions[i - totalFilteredExecutions] = execution;
                }

Which lead to following optimization path:

1. execution.item.amount == 0
2. execution.item.recipient == execution.offerer
3. totalFilteredExecutions += 1 (skipping) instead of inserting into executions list
4. Execution skipped
5. Save a lot of gas on any function that used to execute that execution