Lines of code
<https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L117-L148&gt;

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

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

   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;
   

   }

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

   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&gt;

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.

The text was updated successfully, but these errors were encountered:

All reactions