Infinity NFT Marketplace contest - horsefacts's results

Lines of code

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

Vulnerability details

Protocol fees for orders priced in native ETH are paid in native ETH to the InfinityExchange contract, which includes payable methods in order to receive them. Additionally, the contract includes payable fallback and receive methods to accept direct transfers:

InfinityExchange#L119-L121

  fallback() external payable {}

  receive() external payable {}

The contract owner may call rescueETH to withdraw accrued fees in ETH from the contract. However, this payable function only transfers msg.value ether, i.e. the amount sent by the caller, to the specified destination address, rather than drawing from the contract balance:

InfinityExchange#rescueETH

  /// @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');
  }

This function will effectively send msg.value ether from the caller of rescueETH to the destination address through the InfinityExchange contract, rather than withdrawing accrued ether from the contract balance. Fees accrued in native ETH will be locked in the contract permanently.

Scenario:

• Alice creates a sell order for her token at price 1000, priced in native ETH.
• Bob fills the order and purchases the token.
• The protocol fee is set to the default of 250 BPS. InfinityExchange transfers 975 to Alice and retains 25 in the contract balance as a protocol fee.
• This fee is locked in the contract permanently.

Impact: Contract owner cannot withdraw protocol fees accrued in native ETH.

Recommendation: Add an amount parameter to this function and transfer the specified amount rather than msg.value. Note that this function does not need to be payable, since it only needs to send, not receive ether:

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

Lines of code

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

Vulnerability details

InfinityExchange accepts payments in native ETH, but does not return overpayments to the buyer. Overpayments are likely in the case of auction orders priced in native ETH.

In the case of a Dutch or reverse Dutch auction priced in native ETH, the end user is likely to send more ETH than the final calculated price in order to ensure their transaction succeeds, since price is a function of block.timestamp, and the user cannot predict the timestamp at which their transaction will be included.

In a Dutch auction, final price may decrease below the calculated price at the time the transaction is sent. In a reverse Dutch auction, the price may increase above the calculated price by the time a transaction is included, so the buyer is incentivized to provide additional ETH in case the price rises while their transaction is waiting for inclusion.

The takeOrders and takeMultipleOneOrders functions both check that the buyer has provided an ETH amount greater than or equal to the total price at the time of execution:

InfinityExchange#takeOrders

    // 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');
    }

InfinityExchange#takeMultipleOneOrders

    // 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');
    }

However, neither of these functions refunds the user in the case of overpayment. Instead, overpayment amounts will accrue in the contract balance.

Moreover, since there is a bug in rescueETH that prevents ether withdrawals from InfinityExchange, these overpayments will be locked permanently: the owner cannot withdraw and refund overpayments manually.

Scenario:

• Alice creates a sell order for her token with constraints that set up a reverse Dutch auction: start price 500, end price 2000, start time 1, end time 5.
• Bob fills the order at time 2. The calculated price is 875. Bob is unsure when his transaction will be included, so provides a full 2000 wei payment.
• Bob's transaction is included at time 3. The calculated price is 1250.
• Bob's additional 750 wei are locked in the contract and not refunded.

Suggestion: Calculate and refund overpayment amounts to callers.

Low

Malicious owner can frontrun rageQuit

A malicious owner can observe and frontrun calls to InfinityStaker#rageQuit to increase penalties and extract the quitter's full staked balance as a penalty. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock.

Scenario:

• Alice stakes her tokens for 12 months
• Alice later decides to ragequit and withdraw her staked tokens, and calls rageQuit
• The contract owner observes Alice's transaction and frontruns it with a call to updatePenalties, increasing the 12-month penalty to 1
• Alice's full balance is deducted as a penalty.

Malicious owner can frontrun trades

A malicious owner can observe and frontrun trades to increase the protocol fee and extract the full trade amount as a fee. To mitigate this risk and increase user trust, consider ensuring that the contract owner is a timelock. Additionally, consider adding and enforcing an upper bound on the protocol fee parameter.

Scenario:

• Alice calls takeOrders to fill an open order for her token
• The contract owner observes Alice's transaction and frontruns it with a call to setProtocolFee, increasing the fee amount to 100%
• The full amount of Alice's trade is extracted as a protocol fee

Noncritical

Emit events from protected functions

Protected functions in InfinityStaker update staking parameters like the treasury address, ragequit penalties, and level thresholds, but do not emit corresponding events. Consider adding events that log updates to these parameters. This enables end users to monitor changes to these parameters, and the protocol to monitor for potential malicious behavior.

• InfinityStaker#updateStakeLevelThreshold
• InfinityStaker#updatePenalties
• InfinityStaker#updateInfinityTreasury

Optimizer bug in Solidity 0.8.13 and 0.8.14

Note that Solidity versions 0.8.13 and 0.8.14 are vulnerable to a recently reported optimizer bug related to inline assembly. This project's current settings use the via-IR optimizer pipeline, which is not affected, and there is limited usage of assembly in this codebase and its dependencies, but it's worth being aware of this issue. Solidity 0.8.15 has been released with a fix.

QA

Duplication of _getCurrentPrice function

An identical _getCurrentPrice function is included in both InfinityOrderBookComplication and InfinityExchange:

  /// @dev Gets current order price for orders that vary in price over time (dutch and reverse dutch auctions)
  function _getCurrentPrice(OrderTypes.MakerOrder calldata order) internal view returns (uint256) {
    (uint256 startPrice, uint256 endPrice) = (order.constraints[1], order.constraints[2]);
    uint256 duration = order.constraints[4] - order.constraints[3];
    uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
    if (priceDiff == 0 || duration == 0) {
      return startPrice;
    }
    uint256 elapsedTime = block.timestamp - order.constraints[3];
    uint256 PRECISION = 10**4; // precision for division; similar to bps
    uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
    priceDiff = (priceDiff * portionBps) / PRECISION;
    return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;
  }

Consider extracting this shared function to a library: this removes duplication and ensures the two separate functions will not diverge and behave differently.

Missing event indexes

Event indexes simplify filtering for events by specific criteria, like user addresses.

Consider adding event indexes for:

• address user in CancelAllOrders
• address user in CancelMultipleOrders
• address seller and address buyer in MatchOrderFulfilled
• address seller and address buyer in TakeOrderFulfilled

Typos

• Storage in InfinityExchange#L77
• orders in OrderTypes#L39
• unstake in InfinityStaker#L112