CreateOfferer.sol should not enforce the nonce incremented sequentially, otherwise user can DOS the contract by skipping order

mediumCode4rena

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/CreateOffererLib.sol#L184

Vulnerability details

Impact

CreateOfferer.sol should not enforce the nonce incremented sequentially, otherwise user can DOS the contract by skipping order

Proof of Concept

According to

https://github.com/ProjectOpenSea/seaport/blob/main/docs/SeaportDocumentation.md#contract-orders

Seaport v1.2 introduced support for a new type of order: the contract order. In brief, a smart contract that implements the ContractOffererInterface (referred to as an “Seaport app contract” or "Seaport app" in the docs and a “contract offerer” in the code) can now provide a dynamically generated order (a contract order) in response to a buyer or seller’s contract order request.

the CreateOfferer.sol aims to be comply with the interface ContractOffererInterface

the life cycle of the contract life cycle here is here:

https://github.com/ProjectOpenSea/seaport/blob/main/docs/SeaportDocumentation.md#example-lifecycle-journey

first the function _getGeneratedOrder is called,

then after the order execution, the function ratifyOrder is triggered for contract (CreateOfferer.sol) to do post order validation

In the logic of ratifyOrder, the nonce is incremented by calling this line of code Helpers.processNonce

solidity📋
function ratifyOrder(SpentItem[] calldata offer, ReceivedItem[] calldata consideration, bytes calldata context, bytes32[] calldata, uint256 contractNonce)
	external
	checkStage(Enums.Stage.ratify, Enums.Stage.generate)
	onlySeaport(msg.sender)
	returns (bytes4)
{
	Helpers.processNonce(nonce, contractNonce);
	Helpers.verifyCreate(delegateToken, offer[0].identifier, transientState.receivers, consideration[0], context);
	return this.ratifyOrder.selector;
}

this is calling this line of code

solidity📋
function processNonce(CreateOffererStructs.Nonce storage nonce, uint256 contractNonce) internal {
	if (nonce.value != contractNonce) revert CreateOffererErrors.InvalidContractNonce(nonce.value, contractNonce);
	unchecked {
		++nonce.value;
	} // Infeasible this will overflow if starting point is zero
}

the CreateOffererStructs.Nonce data structure is just nonce

solidity📋
library CreateOffererStructs {
    /// @notice Used to track the stage and lock status
    struct Stage {
        CreateOffererEnums.Stage flag;
        CreateOffererEnums.Lock lock;
    }

    /// @notice Used to keep track of the seaport contract nonce of CreateOfferer
    struct Nonce {
        uint256 value;
    }

but this is not how seaport contract track contract nonce

on seaport contract, we are calling _getGeneratedOrder

which calls _callGenerateOrder

solidity📋
{
	// Do a low-level call to get success status and any return data.
	(bool success, bytes memory returnData) = _callGenerateOrder(
		orderParameters,
		context,
		originalOfferItems,
		originalConsiderationItems
	);

	{
		// Increment contract nonce and use it to derive order hash.
		// Note: nonce will be incremented even for skipped orders, and
		// even if generateOrder's return data doesn't meet constraints.
		uint256 contractNonce = (
			_contractNonces[orderParameters.offerer]++
		);

		// Derive order hash from contract nonce and offerer address.
		orderHash = bytes32(
			contractNonce ^
				(uint256(uint160(orderParameters.offerer)) << 96)
		);
	}

as we can see

solidity📋
// Increment contract nonce and use it to derive order hash.
// Note: nonce will be incremented even for skipped orders, and
// even if generateOrder's return data doesn't meet constraints.
uint256 contractNonce = (
	_contractNonces[orderParameters.offerer]++
);

nonce will be incremented even for skipped orders

this is very important, suppose the low level call _callGenerateOrder return false, we are hitting the else block

solidity📋
 return _revertOrReturnEmpty(revertOnInvalid, orderHash);

this is calling _revertOrReturnEmpty

solidity📋
function _revertOrReturnEmpty(
	bool revertOnInvalid,
	bytes32 contractOrderHash
)
	internal
	pure
	returns (
		bytes32 orderHash,
		uint256 numerator,
		uint256 denominator,
		OrderToExecute memory emptyOrder
	)
{
	// If invalid input should not revert...
	if (!revertOnInvalid) {
		// Return the contract order hash and zero values for the numerator
		// and denominator.
		return (contractOrderHash, 0, 0, emptyOrder);
	}

	// Otherwise, revert.
	revert InvalidContractOrder(contractOrderHash);
}

clearly we can see that if the flag revertOnInvalid is set to false, then even the low level call return false, the nonce of the offerer is still incremented

Where in the Seaport code does it set revertOnInvalid to false?

When the seaport wants to combine multiple orders in this line of code

solidity📋
    function _fulfillAvailableAdvancedOrders(
        AdvancedOrder[] memory advancedOrders,
        CriteriaResolver[] memory criteriaResolvers,
        FulfillmentComponent[][] memory offerFulfillments,
        FulfillmentComponent[][] memory considerationFulfillments,
        bytes32 fulfillerConduitKey,
        address recipient,
        uint256 maximumFulfilled
    ) internal returns (bool[] memory, /* availableOrders */ Execution[] memory /* executions */ ) {
        // Validate orders, apply amounts, & determine if they use conduits.
        (bytes32[] memory orderHashes, bool containsNonOpen) = _validateOrdersAndPrepareToFulfill(
            advancedOrders,
            criteriaResolvers,
            false, // Signifies that invalid orders should NOT revert.
            maximumFulfilled,
            recipient
        );

we calls _validateOrderAndUpdateStatus

solidity📋
// Validate it, update status, and determine fraction to fill.
(bytes32 orderHash, uint256 numerator, uint256 denominator) =
_validateOrderAndUpdateStatus(advancedOrder, revertOnInvalid);

finally we call the logic _getGeneratedOrder(orderParameters, advancedOrder.extraData, revertOnInvalid) below with parameter revertOnInvalid false

solidity📋
  // If the order is a contract order, return the generated order.
	if (orderParameters.orderType == OrderType.CONTRACT) {
		// Ensure that the numerator and denominator are both equal to 1.
		assembly {
			// (1 ^ nd =/= 0) => (nd =/= 1) => (n =/= 1) || (d =/= 1)
			// It's important that the values are 120-bit masked before
			// multiplication is applied. Otherwise, the last implication
			// above is not correct (mod 2^256).
			invalidFraction := xor(mul(numerator, denominator), 1)
		}

		// Revert if the supplied numerator and denominator are not valid.
		if (invalidFraction) {
			_revertBadFraction();
		}

		// Return the generated order based on the order params and the
		// provided extra data. If revertOnInvalid is true, the function
		// will revert if the input is invalid.
		return _getGeneratedOrder(orderParameters, advancedOrder.extraData, revertOnInvalid);
	}

Ok what does this mean?

suppose the CreateOfferer.sol fulfill two orders and create two delegate tokens

the nonces start from 0 and then incremen to 1 and then increment to 2

a user craft a contract with malformed minimumReceived and combine with another valid order to call OrderCombine

as we can see above, when multiple order is passed in, the revertOnInvalid is set to false, so the contract order from CreateOfferer.sol is skipped, but the nonce is incremented

then the nonce tracked by CreateOfferer.sol internally is out of sycn with the contract nonce in seaport contract forever

then the CreateOfferer.sol is not usable because if the ratifyOrder callback hit the contract, transaction revert in this check

solidity📋
 if (nonce.value != contractNonce) revert CreateOffererErrors.InvalidContractNonce(nonce.value, contractNonce);

Tools Used

Manual Review

Recommended Mitigation Steps

I would recommend do not validate the order execution in the ractifyOrder call back by using the contract nonce, instead, validate the order using orderHash

solidity📋
if (
ContractOffererInterface(offerer).ratifyOrder(
orderToExecute.spentItems,
orderToExecute.receivedItems,
advancedOrder.extraData,
orderHashes,
uint256(orderHash) ^ (uint256(uint160(offerer)) << 96)
) != ContractOffererInterface.ratifyOrder.selector
)

Assessed type

DoS