search

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

1 stars 0 forks source link

QA Report #123

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

We list 3 low-critical findings and 3 informational findings:

(Low) Anyone can create useless orders to confuse users and the marketplace

Impact

Anyone can create lots of useless orders that offers and considerations are both 0. These useless orders may confuse users and the marketplace.

Proof of Concept

When validating orders (such as _validateOrdersAndPrepareToFulfill in lib/OrderCombiner.sol and _validateAndFulfillAdvancedOrder in lib/OrderFulfiller.sol), they don't check the length of OfferItem and ConsiderationItem should > 0.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderCombiner.sol#L158 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderFulfiller.sol#L75

Recommended Mitigation Steps

Check length of OfferItem and ConsiderationItem should > 0.

(Low) The fulfiller doesn't need to own any considerations to fulfill orders

Impact

When fulfilling, the protocol will first transfer all offers to the fulfiller, then transfer considerations to recipients. These will cause the fulfiller doesn't need to own any considerations. The fulfiller can use ERC token hooks to sell offers from the order, and use DEX to exchange considerations to fulfill the order.

Proof of Concept

In fulfillBasicOrder, fulfillOrder, fulfillAdvancedOrder, fulfillAvailableOrders, and fulfillAvailableAdvancedOrders, these functions always transfer offers to fulfiller first, then transfer considerations to offerer (or recipients).

Recommended Mitigation Steps

It's better to transfer considerations to the offerer first, then transfer offers to the fulfiller. In this mitigation, the offerer may also not need to own any offers to fulfill orders. But it may mitigate the attack scenario that the offerer can not actively trigger the exploit.

(Low) lib/FulfillmentApplier.sol amount == 0 but it doesn't revert

Impact

FulfillmentApplier will aggregate valid fulfillment OfferItems and ConsiderationItems. It will get the first FulfillmentComponent and then iterate over remaining components. But the amount of the first component may equal to 0 and the protocol doesn’t revert.

Proof of Concept

In _aggregateValidFulfillmentOfferItems of lib/FulfillmentApplier.sol: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/FulfillmentApplier.sol#L270-L289

Also in _aggregateValidFulfillmentConsiderationItems of lib/FulfillmentApplier.sol: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/FulfillmentApplier.sol#L567-L587

            // Declare a variable for the final aggregated item amount.
            let amount := 0

            // Create variable to track errors encountered with amount.
            let errorBuffer := 0

            // Only add offer amount to execution amount on a nonzero numerator.
            if mload(add(orderPtr, AdvancedOrder_numerator_offset)) {
                // Retrieve amount pointer using consideration item pointer.
                let amountPtr := add(offerItemPtr, Common_amount_offset)

                // Set the amount.
                amount := mload(amountPtr)

                // Zero out amount on item to indicate it is credited.
                mstore(amountPtr, 0)

                // Buffer indicating whether issues were found.
                errorBuffer := iszero(amount)
            }

The errorBuffer will check the amount. But if numerator in AdvancedOrder is 0, it will not set errorBuffer.

Recommended Mitigation Steps

Check amount outside the if condition:

            // Declare a variable for the final aggregated item amount.
            let amount := 0

            // Create variable to track errors encountered with amount.
            let errorBuffer := 0

            // Only add offer amount to execution amount on a nonzero numerator.
            if mload(add(orderPtr, AdvancedOrder_numerator_offset)) {
                // Retrieve amount pointer using consideration item pointer.
                let amountPtr := add(offerItemPtr, Common_amount_offset)

                // Set the amount.
                amount := mload(amountPtr)

                // Zero out amount on item to indicate it is credited.
                mstore(amountPtr, 0)
            }

            // Buffer indicating whether issues were found.
            errorBuffer := iszero(amount)

(Info) No need to set isCancelled

Impact

Because _verifyOrderStatus ensures isCancelled of the order is false. There's no need to set isCancelled to false again and waste gas.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderValidator.sol#L71 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderValidator.sol#L227 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderValidator.sol#L236

Recommended Mitigation Steps

Delete these lines:

        _orderStatus[orderHash].isCancelled = false;

(Info) ReferenceFulfillmentApplier.sol _checkMatchingOffer doesn't check the result

Impact

The implementation between lib/FulfillmentApplier.sol and ReferenceFulfillmentApplier.sol are inconsistent.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/reference/lib/ReferenceFulfillmentApplier.sol#L340-L344

                        invalidFulfillment = _checkMatchingOffer(
                            orderToExecute,
                            offer,
                            execution
                        );

_checkMatchingOffer returns invalidFulfillment but doesn't check the validation.

Recommended Mitigation Steps

                        if (_checkMatchingOffer(
                            orderToExecute,
                            offer,
                            execution
                        )) {
                            revert InvalidFulfillmentComponentData();
                        }

(Info) Prevent users from sending ethers to the protocol

Impact

_transferEth will only be executed in the seaport protocol, although the conduit key is brought in. Users may misuse _transferEth by sending ETH to conduit. The protocol and conduit contracts can prevent users from sending ethers accidentally.

Proof of Concept

The protocol and conduit contract doesn’t use receive and fallback functions.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/Seaport.sol#L20 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L28

Recommended Mitigation Steps

    receive() external payable {
        revert();
    }

    fallback external {
        revert();
    }
HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/116

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/115

HardlyDifficult commented 2 years ago

Merging with #117 and #118

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/119