Infinity NFT Marketplace contest - codexploder's results

Lines of code

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

Vulnerability details

Impact

The rescueETH function is implemented incorrectly and will always fail This is sending msg.value to the destination contract instead of address(this).balance

Note

Same need to be fixed at InfinityExchange.sol#L1229

Proof of Concept

1. Let's observe the rescueETH function

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

2. As we can see this is resuing funds using the passed msg.value instead of using contracts fund

Recommended Mitigation Steps

Change the rescueETH function like below:

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

Lines of code

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

Vulnerability details

Impact

Any extra ETH passed while calling takeOrders function will not be returned by the contract causing user to loose funds

Proof of Concept

1. Let us see the takeOrders function

  function takeOrders(OrderTypes.MakerOrder[] calldata makerOrders, OrderTypes.OrderItem[][] calldata takerNfts)
    external
    payable
    nonReentrant
  {

    .......

      uint256 execPrice = _getCurrentPrice(makerOrders[i]);
      totalPrice += execPrice;
      _takeOrders(makerOrders[i], takerNfts[i], execPrice);
      unchecked {
        ++i;
      }
    }
    // 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');
    }
  }

2. Notice the total cost for this order is calculated as totalPrice

3. So if user has passed a msg.value > = totalPrice, this is executed successfully

4. But what about the extra ETH msg.value - totalPrice, which is simply kept by contract and not refunded back to the user

Note

Also need to be fixed for takeMultipleOneOrders function

Recommended Mitigation Steps

Return the excess funds of msg.value - totalPrice back to user

Zero address checks missing

Contract: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L375

Issue: In updateInfinityTreasury function it is not verified that _infinityTreasury is not address(0) else rageQuit balance penality will be lost

Recommendation: Add a check to see _infinityTreasury is not address(0)

require(_infinityTreasury!=address(0), "Incorrect address");

Verify receiving end is contract

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

Issue: In _transferNFTsAndFees function, it is not checked whether recipient is a contract. If receiver is an EOA not supporting ERC777/ERC1155 , NFT can be lost forever

Recommendation: Check whether recipient is a valid contract by chekcing for extcodesize>0

Missing Index validation

Contract: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/TimelockConfig.sol#L88

Issue: In getConfigByIndex, it is not verified if provided index is valid. Same fix is required for getPendingByIndex function as well

Recommendation: Add a check to see that provided index is valid.

require(index>=0 && index<getConfigCount(), "Invalid index");

If not required - Amount can never be greater than vestedTwelveMonths

Contract: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L310

Issue: In _updateUserStakedAmounts function, amount can never be greater than vestedTwelveMonths otherwise user is trying to unstake amount higher than deposit which is not possible due to require check in unstake

Recommendation: Change the implementation like below:

OLD

if (amount > vestedTwelveMonths) {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
            userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
          } else {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
          }

New

if (amount > 0)  {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
			
          }

Zero amount checks

Contract: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L260

Issue: Short circuit if user staked amount =0 in getVestedAmount function

Recommendation: Add below check in getVestedAmount function

require(amount!=0, "Insufficient amount");

No need to check complications

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

Issue: In matchOneToManyOrders function, No need to check complications.contains(makerOrder.execParams[0]) at InfinityExchange.sol#L184, isUserOrderNonceExecutedOrCancelled at InfinityExchange.sol#L216 as this is already done in isOrderValid call at InfinityExchange.sol#L190 and _matchOneMakerSellToManyMakerBuys call at InfinityExchange.sol#L203

Recommendation: Remove extra complications check and isUserOrderNonceExecutedOrCancelled assignation

Unrequired call made to getVestedAmount

Contract: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L168

Issue: In getUserTotalVested function, For noVesting there is no need to call getVestedAmount as this will always be userstakedAmounts[user][Duration.NONE].amount

Recommendation: Directly assign userstakedAmounts[user][Duration.NONE].amount instead of calling getVestedAmount function for noVesting condition