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

1 stars 0 forks source link

Gas Optimizations #178

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L137-L152

    // Determine whether the updated channel is already tracked as open.
    bool channelPreviouslyOpen = channelIndexPlusOne != 0;

    // If the channel has been set to open and was previously closed...
    if (isOpen && !channelPreviouslyOpen) {
        // Add the channel to the channels array for the conduit.
        conduitProperties.channels.push(channel);

        // Add new open channel length to associated mapping as index + 1.
        conduitProperties.channelIndexesPlusOne[channel] = (
            conduitProperties.channels.length
        );
    } else if (!isOpen && channelPreviouslyOpen) {
        // Set a previously open channel as closed via "swap & pop" method.
        // Decrement located index to get the index of the closed channel.
        uint256 removedChannelIndex = channelIndexPlusOne - 1;

channelPreviouslyOpen is true means that channelIndexPlusOne != 0, and channelIndexPlusOne is uint256, then channelIndexPlusOne - 1 will never underflow.

Setting uint256 variables to 0 is redundant

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

    uint256 extraCeiling = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

Other examples include:

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

    uint256 totalFilteredExecutions = 0;

Using constant value instead this.[functionName].selector is more gas efficient

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L79-L80

    // Return a magic value indicating that the transfers were performed.
    magicValue = this.execute.selector;

Recommendation

Change to:

uint256 constant execute_selector_signature = (
    0x4ce34aa200000000000000000000000000000000000000000000000000000000
);

bytes4 constant execute_selector = bytes4(
    bytes32(execute_selector_signature)
);

...

// Return a magic value indicating that the transfers were performed.
magicValue = execute_selector;

Based on our tests, this change can save 21 gas.

Other examples include:

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/Conduit.sol#L112-L113

    // Return a magic value indicating that the transfers were performed.
    magicValue = this.executeBatch1155.selector;
HardlyDifficult commented 2 years ago

Adding unchecked directive can save gas

This seems valid and could provide a small savings, but updating channels is not common and does not impact end-users so this is not a critical path.

Setting uint256 variables to 0 is redundant

There are a lot of instances of this, but in my testing the optimizer seems to mostly take care of this automatically. I saw very little impact from changes like this.

Using constant value instead this.[functionName].selector is more gas efficient

It's unfortunate that the optimizer does not handle this automatically, but given there is small savings here it may be worth including despite the impact to readability. Using immutable may be nice so that the values used are programmatically assigned.