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

1 stars 0 forks source link

Gas Optimizations #193

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

GAS 1. Title: Passing standardTransfer[i] directly to _transfer() https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L68-L71 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L132-L135

Instead of storing standardTransfer[i] value into standardTransfer var. Passing it directly can save gas per loop

    function executeWithBatch1155(
        ConduitTransfer[] calldata standardTransfers,
        ConduitBatch1155Transfer[] calldata batchTransfers
    ) external override returns (bytes4 magicValue) {
        // Ensure that the caller has an open channel.
        if (!_channels[msg.sender]) {
            revert ChannelClosed();
        }

        // Retrieve the total number of transfers and place on the stack.
        uint256 totalStandardTransfers = standardTransfers.length;

        // Iterate over each standard transfer.
        for (uint256 i = 0; i < totalStandardTransfers; ) {

            // Perform the transfer.
            _transfer(standardTransfers[i]); // @audit-info: Pass the value directly here<-------------

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

2. Title: Using assembly for checking open channel, can save some gas

Occurrences: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L58 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L96 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L122

by replacing these check with assembly can save some gas

contract testing20 {

    uint public counter = 10;

    mapping(address => bool) public channels ; 

    function setChanel()public {
        channels[msg.sender] = true;
    }

    modifier checkChannel { 
        assembly {
            mstore(0, caller())
            mstore(32, channels.slot)
            if iszero(sload(keccak256(0, 64))) {revert(0, 0)}// sload directly from storage location
        }
        _;
    }

    function exec()public checkChannel{

        counter++;
    }
    function exec2()public checkChannel{

        counter++;
    }
    function exec3()public checkChannel{

        counter++;
    }
    // 28629 gas without modifier, 313165 gas deploy, 28551 gas optimization ON
    // 28572 gas OPTIMIZOOORRRRR,  274920 gas deploy, 28545 gas optimization ON, 28539 gas using iszero 
HardlyDifficult commented 2 years ago

Passing standardTransfer[i] directly to _transfer()

I ran the recommended changes here and didn't get great results. Some calls got a tiny bit more efficient, others a bit worse. ~ +/- 10 gas and not a consistent win.

Using assembly for checking open channel, can save some gas

I tried this one as well, using the hardhat profile report instead of the simplified example above. Again got some mixed results, best case was a savings of ~40 gas.

Given that neither change provides a clear win, I'm closing this as invalid.

rfart commented 1 year ago

Hi there, im afraid i have to disagree with the decision on this one since the changes that made into the seaportV1.1, they made some changes related to this issue https://github.com/ProjectOpenSea/seaport/blob/main/contracts/conduit/Conduit.sol#L104 for the 1 gas opt https://github.com/ProjectOpenSea/seaport/blob/main/contracts/conduit/Conduit.sol#L40-L68 for the 2 gas opt

for reference you can take a look at the etherscan https://etherscan.io/address/0x00000000006c3852cbef3e08e8df289169ede581#code on the conduit.sol, those changes was made based on these reports. and since on the contest page, there is nothing that stated a limit on what is the minimum gas, that was accepted, i don't think this report should go to invalid.

Thank you

HardlyDifficult commented 1 year ago

That's a fair point. Thanks for raising this.

Re-opening as valid.