code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L379 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L390

Vulnerability details

Impact

Detailed description of the impact of this finding.

I noticed that the nonReentrant modifier only adds some of the functions that generate state changes to the InfinityExchange.sol contract, which provides the feasibility of reentrancy between multiple functions in a single contract, so I recommend adding the nonReentrant modifier to all functions that generate state changes.

  function transferMultipleNFTs(address to, OrderTypes.OrderItem[] calldata items) external nonReentrant {
    _transferMultipleNFTs(msg.sender, to, items);
  }

  function cancelAllOrders(uint256 minNonce) external {
    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
    userMinOrderNonce[msg.sender] = minNonce;
    emit CancelAllOrders(msg.sender, minNonce);
  }

  function cancelMultipleOrders(uint256[] calldata orderNonces) external {
    uint256 numNonces = orderNonces.length;
    require(numNonces > 0, 'cannot be empty');
    for (uint256 i = 0; i < numNonces; ) {
      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
      isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]] = true;
      unchecked {
        ++i;
      }
    }
    emit CancelMultipleOrders(msg.sender, orderNonces);
  }

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

I recommend adding the nonReentrant modifier to all functions that generate state changes.

pmerkleplant commented 2 years ago

There is no PoC provided indicating an issue through reentrancy. State modification does not automatically make functions vulnerable to reentrancy.

For a medium severity issue, I would expect a more detailed explanation of the vulnerability.

nneverlander commented 2 years ago

The Cancel* orders are not calling any external contracts. So not sure if adding nonReentrant modifier to them makes sense.

HardlyDifficult commented 2 years ago

Using nonReentrant on all functions that may change state is unnecessary. Without an indication on where this is required, the report is invalid.

code-423n4 / 2022-06-infinity-findings