OpenSea Seaport contest - NoamYakov's results

[1] Unnecessary MLOADs in for-each loops

There are many for loops that follows this for-each pattern:

for (uint256 i = 0; i < array.length; i++) {
        // do something with `array[i]`
}

In such for loops, the array.length is read on every iteration, instead of caching it once in a local variable and read it from there. Memory reads are a bit more expensive than reading local variables.

Recommendation

Read these values from memory once, cache them in local variables and then read them again from the local variables. For example:

uint256 length = array.length;
for (uint256 i = 0; i < length; i++) {
        // do something with `array[i]`
}

Code References

• OrderCombiner.sol: lines 291, 598, 621
• OrderFulfiller.sol: lines 217, 306

[2] Unnecessary CALLDATALOAD in BasicOrderFulfiller._validateAndFulfillBasicOrder()

The following assembly code from BasicOrderFulfiller._validateAndFulfillBasicOrder() invokes the CALLDATALOAD opcode twice using the same argument:

// Mask all but 2 least-significant bits to derive the order type.
orderType := and(calldataload(BasicOrder_basicOrderType_cdPtr), 3)

// Divide basicOrderType by four to derive the route.
route := div(calldataload(BasicOrder_basicOrderType_cdPtr), 4)
}

Recommendation

Optimize by invoking that opcode only once and caching the result in a local variable:

let basicOrderType := calldataload(BasicOrder_basicOrderType_cdPtr)

// Mask all but 2 least-significant bits to derive the order type.
orderType := and(basicOrderType, 3)

// Divide basicOrderType by four to derive the route.
route := div(basicOrderType, 4)
}

[3] Unused named returns can be removed

Removing unused named return variables can reduce gas usage and improve code clarity. For example:

function foo() external returns (uint256 x) {
        return goo();
}

can be optimized to:

function foo() external returns (uint256) {
        return goo();
}

Code References

• Consideration.sol: lines 225, 311, 352, 402, 522-525, 561-563
• OrderCombiner.sol: lines 135, 547, 665, 708, 791
• OrderValidator.sol: lines 113-114, 422-425

[4] Use smaller unsigned integer types for timestamp fields in structures

The structures OrderComponents, BasicOrderParameters and OrderParameters contains fields called startTime and endTime of type uint256. These fields holds timestamp values which are much smaller than type(uint40).max for example. Therefore, changing the type of these fields to a smaller unsigned integer type (like uint40) can save significant amount of gas because in such case, these two fields will share the same 32-byte slot and the entire structure will use one less slot.

[5] Default value initialization

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc. depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example:

uint256 x = 0;

can be optimized to:

uint256 x;

[6] Place enums next to addresses in structs

The structures OrderComponents, BasicOrderParameters and OrderParameters contains fields of types OrderType and BasicOrderType (enums), as well as fields of type address. These fields should be places next to each other so they can share the same 32-byte slot and the entire structure will use one less slot. This is because the enum type don't use more than 12 bytes.

[7] Use strict comparisons when comparing timestamps

The function Verifiers._verifyTime() validates that the current time doesn't exceed a given range of timestamps:

if (startTime > block.timestamp || endTime <= block.timestamp) {
        // ...
}

The EVM doesn't have an opcode for <= comparison, and therefore it performs that comparison by splitting it to a < comparison AND a == comparison.

Recommendation

Change that if condition to:

if (startTime > block.timestamp || endTime < block.timestamp) {
        // ...
}

This won't change the functionality because we deal with timestamps. One second won't affect the protocol.

[8] Don't use named returns when it isn't being used as a variable

Explicit returns use less gas than implicit returns (named returns). Therefore, whenever you don't use the benefits of implicit returns (the use of the return variables), you should change the function to use explicit return instead.

Take Consideration.fulfillBasicOrder as an example:

function fulfillBasicOrder(BasicOrderParameters calldata parameters)
        external
        payable
        override
        returns (bool fulfilled)
{
        // Validate and fulfill the basic order.
        fulfilled = _validateAndFulfillBasicOrder(parameters);
}

Here you don't use fulfilled as a variable, but only assigns it the return value. It can easily be optimized to:

function fulfillBasicOrder(BasicOrderParameters calldata parameters)
        external
        payable
        override
        returns (bool)
{
        // Validate and fulfill the basic order.
        return _validateAndFulfillBasicOrder(parameters);
}

[9] Unnecessary MLOAD in OrderCombiner._validateOrdersAndPrepareToFulfill()

The following code can be optimized by reading a local variable (endAmount) instead of a struct field located on memory (offerItem.endAmount):

// Update end amount in memory to match the derived amount.
offerItem.endAmount = endAmount;

// Adjust offer amount using current time; round down.
offerItem.startAmount = _locateCurrentAmount(
        offerItem.startAmount,
        offerItem.endAmount,
        elapsed,
        remaining,
        duration,
        false // round down
);

The optimized code is as follows:

// Update end amount in memory to match the derived amount.
offerItem.endAmount = endAmount;

// Adjust offer amount using current time; round down.
offerItem.startAmount = _locateCurrentAmount(
        offerItem.startAmount,
        endAmount,
        elapsed,
        remaining,
        duration,
        false // round down
);

The same optimization can be applied to the code bellow as well (from the same function):

// Update end amount in memory to match the derived amount.
considerationItem.endAmount = endAmount;

// Adjust consideration amount using current time; round up.
considerationItem.startAmount = (
        _locateCurrentAmount(
                considerationItem.startAmount,
                considerationItem.endAmount,
                elapsed,
                remaining,
                duration,
                true // round up
)
);

[10] Prefix decrements are cheaper than postfix decrements

Use prefix decrements (--x) instead of postfix decrements (x--).

Recommendation

Change all postfix decrements to prefix decrements.

Code References

• OrderCombiner.sol: line 229

[11] Unnecessary array boundaries check when loading an array element twice

Some functions loads the same array element (e.g., orderHashes[i]) more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundant second array boundaries check and saving gas.

Recommendation

Load the array elements once, cache them in local variables and then read them again using the local variables. For example:

uint256 item = array[i];
// do something with `item`
// do some other thing with `item`

Code References

• CriteriaResolution.sol: lines 168+177 (advancedOrders[i])
• OrderCombiner.sol: lines 375+386 (orderHashes[i])

[12] Use if instead of else if whenever possible

if statements are cheaper than else if statements.

Recommendation

Modify any else if statement to if statement when it doesn't affect functionality (when the conditions can pass together).

Code References

• Conduit.sol: lines 179, 192
• ConduitController.sol: line 149
• BasicOrderFulfiller.sol: lines 220, 242
• Executor.sol: lines 64, 74

[13] ++ is cheaper than += 1

Use prefix increments (++x) instead of increments by 1 (x += 1).

Recommendation

Change all increments by 1 to prefix increments.

Code References

• OrderCombiner.sol: lines 490, 515, 768