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

1 stars 0 forks source link

Prevent Out of Gas error #54

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/ConduitController.sol https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/interfaces/ConduitControllerInterface.sol

Vulnerability details

Impact

=> User can use getChannels function if user wants to evaluate all his open channels for approvals

=> But getChannels function will revert with out-of-gas error for a conduit with many channels.

=> Using getChannel(address conduit, uint256 channelIndex) is even a non effective way since there will be too many channel indexes to go one by one.

=> By the time user evaluate a channel approval, it might be too late (channel with incorrect approval might drain user holdings) and could cause possible fund loss.

Recommended Mitigation Steps

Add a new function which allows to retrieve channels from start to end Index, thus preventing Out of Gas error and giving a convenient way to retrieve channels

function getChannel(address conduit, uint256 startIndex, uint256 endIndex)
        external
        view
        override
        returns (address[] memory channels)
    {
        // Ensure that the conduit in question exists.
        _assertConduitExists(conduit);

        // Retrieve the total open channel count for the conduit in question.
        uint256 totalChannels = _conduits[conduit].channels.length;

        // Ensure that the supplied index is within range.
        if (startIndex >= totalChannels || endIndex >= totalChannels) {
            revert ChannelOutOfRange(conduit);
        }

for(uint256 i=startIndex;i<=endIndex;i++){
        // Retrieve the channel at the given index.
        channels[i] = _conduits[conduit].channels[i];
}
    }
0age commented 2 years ago

This is incorrect; getChannel can still be called in a loop or multicall and any conduit that has enough channels to cause an out-of-gas error is almost certainly already behaving maliciously and should be revoked immediately. Also out-of-scope as we explicitly state that for the purposes of the review only a single conduit (Seaport) is added as a channel.

0xleastwood commented 2 years ago

As per the README:

As such, this is invalid.