code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L52-L81 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L117-L148

Vulnerability details

Impact

execute() and executeWithBatch1155() are external functions. Both functions run for loops, boundary of which are determined by the function arguments. Anytime there's a loop where the input comes from an external source there's the possibility of unbounded looping.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L52-L81

function execute(ConduitTransfer[] calldata transfers)
    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 = transfers.length;

    // Iterate over each transfer.
    for (uint256 i = 0; i < totalStandardTransfers; ) {     //@audit-issue totalStandardTransfers is determined by the caller. Susceptible to unbounded looping.
        // Retrieve the transfer in question.
        ConduitTransfer calldata standardTransfer = transfers[i];

        // Perform the transfer.
        _transfer(standardTransfer);

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

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

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L117-L148

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; ) {     //@audit-issue totalStandardTransfers is determined by the caller. Susceptible to unbounded looping.
        // Retrieve the transfer in question.
        ConduitTransfer calldata standardTransfer = standardTransfers[i];

        // Perform the transfer.
        _transfer(standardTransfer);

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

    // Perform 1155 batch transfers.
    _performERC1155BatchTransfers(batchTransfers);

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

Reference: https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/#dos-with-block-gas-limit

Tools Used

Manual review

Recommended Mitigation Steps

I suggest to limit the max number the loops can run. If the required iteration is greater than the limit, force it to take multiple transactions, or revert the function with a message indicating the reason.

0age commented 2 years ago

duplicate and already commented on

HardlyDifficult commented 2 years ago

The comment from 0age on the duplicate report:

This is not a valid finding; yes the number of transfers you can execute is capped by the block limit but setting an arbitrary cap would have the same effect (i.e. transaction would revert) but without as much headroom for additional transfers.