Infinity NFT Marketplace contest - obtarian's results

Lines of code

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

Vulnerability details

User called takeOrders() and takeMultipleOneOrders() functions accumulate native token fees over time. These fees end up being frozen on the contract balance. There is only one way for an owner to transfer them, a rescueETH() function, that isn’t able to access ETH balance of the contract, transferring over call’s msg.value instead.

Impact

ETH revenue of the InfinityExchange is lost as there are no means to retrieve ETH from the balance.

Proof of Concept

ETH denominated fees are left with the InfinityExchange in _transferFees’s logic:

  function _transferFees(
    address seller,
    address buyer,
    uint256 amount,
    address currency
  ) internal {
    // protocol fee
    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;
    uint256 remainingAmount = amount - protocolFee;
    // ETH
    if (currency == address(0)) {
      // transfer amount to seller
      (bool sent, ) = seller.call{value: remainingAmount}('');
      require(sent, 'failed to send ether to seller');

rescueETH() method is currently the only way for an owner to retrieve ETH funds from the contract:

  /// @dev used for rescuing exchange fees paid to the contract in ETH
  function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}('');
    require(sent, 'failed');
  }

There msg.value is used as the transfer amount: only amount attached to the call is transferred over.

Recommended Mitigation Steps

Use the contract balance instead:

function rescueETH(address destination) external payable onlyOwner {
  (bool sent, ) = destination.call{value: address(this).balance}('');
  require(sent, 'failed');
}

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L323-L327 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L359-L363

Vulnerability details

takeOrders() and takeMultipleOneOrders() are the main user facing functionality of the protocol. Both require currency to be fixed for the call and can have it either as a ERC20 token or ETH. This way, the probability of a user sending over a ETH with the call whose currency is a ERC20 token isn't negligible. However, in this case ETH funds of a user will be permanently lost.

Setting the severity to medium as this is permanent fund freeze scenario conditional on a user mistake, which probability can be deemed high enough as the same functions are used for ETH and ERC20 orders.

Proof of Concept

Both takeOrders() and takeMultipleOneOrders() only check that ETH funds are enough to cover the order's totalPrice:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L323-L327

    // check to ensure that for ETH orders, enough ETH is sent
    // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
    if (isMakerSeller && currency == address(0)) {
      require(msg.value >= totalPrice, 'invalid total price');
    }

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L359-L363

    // check to ensure that for ETH orders, enough ETH is sent
    // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
    if (isMakerSeller && currency == address(0)) {
      require(msg.value >= totalPrice, 'invalid total price');
    }

When currency is some ERC20 token, while msg.value > 0, the msg.value will be permanently frozen within the contract.

Recommended Mitigation Steps

Consider adding the check for msg.value to be zero for the cases when it is not utilized:

    // check to ensure that for ETH orders, enough ETH is sent
    // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
    if (isMakerSeller && currency == address(0)) {
      require(msg.value >= totalPrice, 'invalid total price');
    } else {
      require(msg.value == 0, 'non-zero ETH value');
    }