search

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

1 stars 0 forks source link

Gas Optimizations #109

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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:

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:

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:

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:

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:

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:

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:

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:

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
}
HardlyDifficult commented 2 years ago

Redundant variable initialization

The optimizer seems to take care of this scenario now - I tested all the changes together and gas went up & down, best savings ~22 gas.

Usage of unchecked can be added Prefix increment or decrement variables costs less gas Redundant return statements Cache array's length when iterating through it

There is some savings to be had here.

Increment loop index in the end of the loop body

I'm not sure this tactic is worth the impact to readability, unless combining with unchecked increments.

Use > 0 instead of != 0

I got some mixed results when testing these changes - ~/- ~25 gas. Very small impact and does not appear to be a consistent win.

The rest of the findings do appear to be valid considerations and each could provide small savings.