Malicious offerers can renew orders by incrementing nonce

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Assertions.sol#L58-L63

Vulnerability details

Impact

The seaport contract retrieves the nonce of order by fetching the offerer's current nonce. Assertions.sol#L58-L63 The transaction sender may land on a different offer if the offerer front-run the fulfill transaction and increment the nonce.

The attack scenario:

1. A malicious offerer makes an order and fulfills 9/10 of the order.
2. A User wants to buy one NFT and tries to fulfill to order with denominator == 1.
3. The offerer observes the fulfill tx and front-run the tx by. a) increment the nonce. b) validate a new order with the same order parameter.
4. User ends up buying 10 NFT.

   Proof of Concept

advance_order = ((*order_par,  1), 9, 10, "", "")
seaport.functions.fulfillAdvancedOrder(advance_order, [], "").transact()

# Attacker renew the order by incrementing the nonce and validating the order
seaport.functions.incrementNonce().transact({'from': attacker})
seaport.functions.validate([order]).transact({'from': attacker})

#
# User tries to buy 1/10 of the order but insteading buying 10 times more
advance_order = ((*order_par,  1), 1, 1, "", "")
seaport.functions.fulfillAdvancedOrder(advance_order, [], "").transact()

Tools Used

Manual inspection

Recommended Mitigation Steps

Since the same order parameters may lead to a different order state, it's necessary to provide the order nonce in a transaction. Verifying whether the order's nonce is the latest one would be safe enough.

    function _assertConsiderationLengthAndGetNoncedOrderHash(
        OrderParameters memory orderParameters
    ) internal view returns (bytes32) {
        // Ensure supplied consideration array length is not less than original.
        _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength(
            orderParameters.consideration.length,
            orderParameters.totalOriginalConsiderationItems
        );

        if (orderParameters.orderNonce != _getNonce(orderParameters.offerer))
        revert NonceNotMetch();

        // Derive and return order hash using current nonce for the offerer.
        return
            _deriveOrderHash(
                orderParameters,
                orderParameters.orderNonce)
            );
    }

[0age]

This is an interesting finding, but ultimately can be remediated by simply clarifying that this is possible and clarifying that fulfillers should not provide a partial fill fraction above what they actually want to receive back, regardless of the fill fraction.

[HardlyDifficult]

Grouping with the warden's QA report https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/150