Infinity NFT Marketplace contest - saian's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1229 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L345

Vulnerability details

Impact

The InfinityStaker#rescueETH and InfinityExchange#rescueETH payable functions sends msg.value to the destination and not the ether in the contract, so the fees and accidentally transferred ether is not sent to the destination address

Proof of Concept

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L345

  function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}(''); 
    require(sent, 'Failed to send Ether');
  }

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

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

Tools Used

Manual review

Recommended Mitigation Steps

msg.value can be replaced with address(this).balance and payable can be removed

Low severity findings

require condition always returns true

require statement in InfinityStaker#getRagequitAmounts executes condition >= 0 which always returns true for unsigned integers, it can be changed to != 0

Proof of concept

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L193

    require(totalStaked >= 0, 'nothing staked to rage quit');

Non-critical findings

Missing natspec comments

Some functions in the contracts have missing natspec comments

Proof of concept

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/InfinityToken.sol

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/token/TimelockConfig.sol

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

Incomplete natspec comments

Some function have comments that are incomplete

Proof of concpet

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

missing @params

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

  /// @dev checks whether the orders are active and not expired
  function isTimeValid(OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy)
    public
    view
    returns (bool)

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

  /// @dev checks whether the price is valid; a buy order should always have a higher price than a sell order
  function isPriceValid(OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy)
    public
    view
    returns (bool, uint256)

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

  /// @dev sanity check to make sure that a taker is specifying the right number of items
  function areTakerNumItemsValid(OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems)
    public
    pure
    returns (bool)

missing @returns

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

  /**
   * @notice Verifies the validity of the order
   * @dev checks whether order nonce was cancelled or already executed, 
          if signature is valid and if the complication and currency are valid
   * @param order the order
   * @param orderHash computed hash of the order
   */
  function isOrderValid(OrderTypes.MakerOrder calldata order, bytes32 orderHash) public view returns (bool) {
      

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

  /// @notice returns the number of complications supported by the exchange
  function numComplications() external view returns (uint256) {

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

  /// @notice returns the complication at the given index
  function getComplicationAt(uint256 index) external view returns (address) {

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

  /// @notice returns whether a given complication is valid
  function isValidComplication(address complication) external view returns (bool) {

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

  /// @notice returns the number of currencies supported by the exchange
  function numCurrencies() external view returns (uint256) {

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

 /// @notice returns the currency at the given index
  function getCurrencyAt(uint256 index) external view returns (address) {

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

  /// @notice returns whether a given currency is valid
  function isValidCurrency(address currency) external view returns (bool) {

Events missing indexed fields

events should use three indexed fields if there are three or more fields

Proof of concept

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

  event MatchOrderFulfilled(  
    bytes32 sellOrderHash,
    bytes32 buyOrderHash,
    address seller,
    address buyer,
    address complication, // address of the complication that defines the execution
    address currency, // token address of the transacting currency
    uint256 amount // amount spent on the order
  );

  event TakeOrderFulfilled(
    bytes32 orderHash,
    address seller,
    address buyer,
    address complication, // address of the complication that defines the execution
    address currency, // token address of the transacting currency
    uint256 amount // amount spent on the order
  );