Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L205-L212 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/AmountDeriver.sol#L99-L106

Vulnerability details

DOS attack is possible as OrderValidator._validateOrderAndUpdateStatus sets order's numerator and denominator based on an arbitrary fulfiller provided input. When there is a large bulk sale, an attacker can become one of the first buyers (partial fulfillers) and set numerator and denominator to be large up to the uint256 type limit, so that her order will pass, but all the subsequent orders will pass only if fully similar to the attackers', i.e. all fulfillments otherwise correct, but having different numerator and denominator, will be reverted due to numerical overflow.

In many cases this will have clear economic incentive for the attacker as she will own a scarce asset while other participants still be figuring out the correct order setup. This incentive can be further exacerbated by a decreasing amount of the sale's consideration (or an increasing amount of its offer).

Impact

Despite being inartistically a denial of service and function unavailability issue, it can be utilized for stealing the funds from other sale participants or from an offerer, while both the technical possibility and economic incentive are present. I.e. oversubscribed NFT sales are common, using a desired denominator and performing a front running can be achieved straightforwardly, while the expected payoff from blocking other sale participants can vary, but often enough is big enough. Per guidelines marking the issue as medium severity.

Proof of Concept

Suppose there is a bundle ERC721 offer that can be partially filled, i.e. say it is a sale of 10000 NFTs and the offerer assumes that there will be many buyers, allowing partial fulfillments.

If, for simplicity, we assume that the offer price in ERC20 terms have the same 4 decimals, then attacker can buy the lowest possible amount, say 1 NFT, providing denominator = ((2**256 - 1) / 10000) * 10000 and numerator = (2**256 - 1) / 10000.

After that no fulfillments with other denominator and nominator can pass through. There is an economic incentive for the attack as such a sale blocking limits the available NFT supply, i.e. such a restriction implied on all participants can allow an attacker to, for example, sell a newly obtained item first, while other participants figure out and adjust to the limitation. As an example, the NFT being sold can be exchangeable to an another NFT for which there is an active market and the attacker will do such an exchange first, quickly sell the liquid NFT and return with another order, etc. This way such a strategy can be used for profit at the expense of other participants being slowed down by the attack.

Another example can be, say, decreasing amount for a given consideration (or increasing amount of an offer), and, if such an implied dutch auction period is short, an attacker can gain some time by slowing down all others, and get an item at the better price. For example, an attacker can front run everyone else with such a big denominator buy, then monitor for the first valid buy from all other participants, and front run it again with as many buys as she can afford, this way lowering the effective price paid at the expense of the offerer.

Schematic POC:

Bob hosts hot NFT sale, the offer is 10000 new NFTs for 1 WBTC.
Alice the attacker wants to remove or slow down other participants. Not having the funds to buy it all, she buys 1 NFT with denominator = ((2**256 - 1) / 10**8) * 10**8, nominator = ((2**256 - 1) / 10*8).
Fulfillments with other denominators will yield updating denominator up and will not go through, as the calculations involved revert.

There are 2 sources for that, the core is the denominator recalculation logic of _validateOrderAndUpdateStatus, and the secondary is the amount calculation logic of getFraction() (it will trigger here, so 10**8). Until other participants figure it out and make the very same fulfilment, Alice have time to sell the NFT elsewhere, profiting from being one of the few owners.

OrderValidator's _validateOrderAndUpdateStatus biggest common denominator logic can revert being run with a denominator already maxed out:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L192-L239

        // Read filled amount as numerator and denominator and put on the stack.
        uint256 filledNumerator = orderStatus.numerator;
        uint256 filledDenominator = orderStatus.denominator;

        // If order currently has a non-zero denominator it is partially filled.
        if (filledDenominator != 0) {
            // If denominator of 1 supplied, fill all remaining amount on order.
            if (denominator == 1) {
                // Scale numerator & denominator to match current denominator.
                numerator = filledDenominator;
                denominator = filledDenominator;
            }
            // Otherwise, if supplied denominator differs from current one...
            else if (filledDenominator != denominator) {
                // scale current numerator by the supplied denominator, then...
                filledNumerator *= denominator;

                // the supplied numerator & denominator by current denominator.
                numerator *= filledDenominator;
                denominator *= filledDenominator;
            }

            // Once adjusted, if current+supplied numerator exceeds denominator:
            if (filledNumerator + numerator > denominator) {
                // Skip underflow check: denominator >= orderStatus.numerator
                unchecked {
                    // Reduce current numerator so it + supplied = denominator.
                    numerator = denominator - filledNumerator;
                }
            }

            // Skip overflow check: checked above unless numerator is reduced.
            unchecked {
                // Update order status and fill amount, packing struct values.
                _orderStatus[orderHash].isValidated = true;
                _orderStatus[orderHash].isCancelled = false;
                _orderStatus[orderHash].numerator = uint120(
                    filledNumerator + numerator
                );
                _orderStatus[orderHash].denominator = uint120(denominator);
            }
        } else {
            // Update order status and fill amount, packing struct values.
            _orderStatus[orderHash].isValidated = true;
            _orderStatus[orderHash].isCancelled = false;
            _orderStatus[orderHash].numerator = uint120(numerator);
            _orderStatus[orderHash].denominator = uint120(denominator);
        }

I.e. the logic can execute only if orderStatus.denominator == denominator, most other legitimate cases will lead to overflows at denominator recalculation as an already maxed current denominator will be multiplied further:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderValidator.sol#L205-L212

            else if (filledDenominator != denominator) {
                // scale current numerator by the supplied denominator, then...
                filledNumerator *= denominator;

                // the supplied numerator & denominator by current denominator.
                numerator *= filledDenominator;
                denominator *= filledDenominator;
            }

The second element of the implementation that limits the denominator's max possible value is AmountDeriver's _getFraction():

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

        // 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))
        }

It is either one or another, i.e. an attacker will utilize the parameters that do not revert at a limit, either sourced by _validateOrderAndUpdateStatus() or by _getFraction(), i.e. the lesser limit will define the parameters of the attack.

Notice, that _getFraction() logic is mentioned in TOB-OSC-11, which describes overflow in general. Here it is denominator based attack that isn't fixable by the changes in getFraction(). It's actually otherwise, if getFraction() be rewritten to exclude the possibility of overflow, the attack surface of the current issue expands somewhat as a possible overflow in getFraction() acts as a limitation to the range of the initial trade the attacker must perform in order to initiate the desired state of the system.

If this limitation is lifted, the attacker, for example, can set d = ((2**256 - 1) / 10) * 10 as a denominator and n = d / 10 as a numerator when the offer has 10 NFTs. Now its impossible as mulmod(value, numerator, denominator) implies that numerator being multiplied by the consideration amount should not overflow, so both n and d have to be lower (provided that the corresponding ERC20 decimals being higher than 2).

Both _validateOrderAndUpdateStatus() and _applyFractionsAndTransferEach() -> _applyFraction() -> _getFraction() are called by _validateAndFulfillAdvancedOrder():

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/OrderFulfiller.sol#L86-L117

        // Validate order, update status, and determine fraction to fill.
        (
            bytes32 orderHash,
            uint256 fillNumerator,
            uint256 fillDenominator
        ) = _validateOrderAndUpdateStatus(
                advancedOrder,
                criteriaResolvers,
                true,
                priorOrderHashes
            );

        // Create an array with length 1 containing the order.
        AdvancedOrder[] memory advancedOrders = new AdvancedOrder[](1);

        // Populate the order as the first and only element of the new array.
        advancedOrders[0] = advancedOrder;

        // Apply criteria resolvers using generated orders and details arrays.
        _applyCriteriaResolvers(advancedOrders, criteriaResolvers);

        // Retrieve the order parameters after applying criteria resolvers.
        OrderParameters memory orderParameters = advancedOrders[0].parameters;

        // Perform each item transfer with the appropriate fractional amount.
        _applyFractionsAndTransferEach(
            orderParameters,
            fillNumerator,
            fillDenominator,
            orderParameters.conduitKey,
            fulfillerConduitKey
        );

Recommended Mitigation Steps

Consider limiting the denominator value accepted from the fulfillers. For example, 2**32 limit will allow any non-malicious use-cases, enabling the calculations for the common denominator.

However, as in the current logic the denominator can keep increasing, it is possible to push it to any given limit with a sequence of fulfillments. This way a combination of a scaling down logic (variations of greatest common divisor) and a limitations (i.e. if it's impossible to obtain a non-increasing denominator for a fulfilment, it can be reverted with a specific mistake; or the denominator can be forced to be current once it surpasses the limit) is advised.

As an another combination of those consider limiting the possible values of denominator to a subset (as an example, 2**k for all k > 0), reverting if the supplied one doesn't belong there, and having a more straightforward and simple logic given that current and fulfillments' denominators are from the subset.

code-423n4 / 2022-05-opensea-seaport-findings

Order partial fulfilling can be blocked by the first fulfiller #180

Lines of code

Vulnerability details

Impact

Proof of Concept

Recommended Mitigation Steps