search

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

1 stars 0 forks source link

[WP-H1] `OrderFulfiller.sol#_applyFractionsAndTransferEach()` Orders with `offerItem.itemType == ItemType.NATIVE` are not processed properly #177

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

// Reduce available value if offer spent ETH or a native token.
if (offerItem.itemType == ItemType.NATIVE) {
    // Ensure that sufficient native tokens are still available.
    if (amount > etherRemaining) {
        revert InsufficientEtherSupplied();
    }

    // Skip underflow check as a comparison has just been made.
    unchecked {
        etherRemaining -= amount;
    }
}

When fulfilling an order with offerItem.itemType == ItemType.NATIVE, the caller is required to send the native tokens for the offer item as msg.value.

We believe this is a wrong design and can be used as a way to initiate phishing attacks targeting the pleb users.

  1. The caller is the buyer.

When the caller calls Consideration#fulfillOrder()/fulfillAdvancedOrder() to fulfill or take an order, the call stack is:

Consideration#fulfillOrder()/fulfillAdvancedOrder() -> OrderFulfiller#_validateAndFulfillAdvancedOrder() -> OrderFulfiller#_applyFractionsAndTransferEach() ->

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

// Transfer item from caller to recipient specified by the item.
_transferConsiderationItem(
    considerationItem,
    msg.sender,
    fulfillerConduitKey,
    accumulator
);

The payments or considerations will be taken from the msg.sender to the recipient.

This makes the caller equals the buyer.

  1. It makes no sense for the buyer to pay for the offered native token item.

In the current implementation, the caller/buyer is required to pay for the offered native token item, which will go directly back to the caller within the transaction.

This is inherently meaningless, even if you consider the caller to be a exector contract, there is no point to ask for the native tokens just to send it back right away.

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

// Utilize assembly to set overloaded offerItem arguments.
assembly {
    // Write derived fractional amount to startAmount as amount.
    mstore(add(offerItem, ReceivedItem_amount_offset), amount)
    // Write fulfiller (i.e. caller) to endAmount as recipient.
    mstore(
        add(offerItem, ReceivedItem_recipient_offset),
        caller()
    )
}

// Reduce available value if offer spent ETH or a native token.
if (offerItem.itemType == ItemType.NATIVE) {
    // Ensure that sufficient native tokens are still available.
    if (amount > etherRemaining) {
        revert InsufficientEtherSupplied();
    }

    // Skip underflow check as a comparison has just been made.
    unchecked {
        etherRemaining -= amount;
    }
}

// Transfer the item from the offerer to the caller.
_transferOfferItem(
    offerItem,
    orderParameters.offerer,
    offererConduitKey,
    accumulator
);

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L54-L63

function _transfer(
    ReceivedItem memory item,
    address from,
    bytes32 conduitKey,
    bytes memory accumulator
) internal {
    // If the item type indicates Ether or a native token...
    if (item.itemType == ItemType.NATIVE) {
        // transfer the native tokens to the recipient.
        _transferEth(item.recipient, item.amount);
  1. This can be used as a phishing attack vector.

A sophisticated attacker could create a sell order offering '100 ETH' for an NFT that usually trades at a lower price, and then send the link to a non-technical buyer.

The buyer has trust in the marketplace and was pleased with the price offered. So they called fulfillOrder() through the web UI, and their extension/hardware/mobile app wallet UI may display the 100 ETH in a funny way, the buyer thought that must be a friendly feature to show how much they are getting paid.

So they clicked "Send", and the next thing they learned, was that's not really a friendly feature, and their precious NFT now belongs to the attacker.

Impact

We are aware that requiring the caller to pay for the offer item of native tokens is a designed/intended behavior, as you have clearly documented this in the Out of Scope section of the document.

As Ether cannot be "taken" from an account, any order that contains Ether or other native tokens as an offer item (including "implied" mirror orders) must be supplied by the caller executing the order(s) as msg.value

However, we do not believe that this can be justified as an out-of-scope issue simply because it's an intended/designed behavior.

If it's a bad design that could cause users to lose money, it's a problem that needs to be addressed.

Recommendation

Consider making the following changes:

  1. Adding a set of features to the Conduit contract to support depositing native tokens (Ether) and allowing a trusted caller to pull the native tokens from the Conduit contract and send them to a specified target. In essence, to build a special purposed wrapped ether contract.
  2. Requiring the offerer to use a Conduit to create an order with offerItem.itemType == ItemType.NATIVE.
0age commented 2 years ago

As they point out, we explicitly recognized this as a known limitation going into the competition. We may make a modification here (and appreciate them writing up their perspective on this) but this is not a valid finding (and their proposed solution is incorrect)

0xleastwood commented 2 years ago

As per the sponsor, I believe the finding to be invalid for two reasons: