Infinity NFT Marketplace contest - 0xkowloon's results

Lines of code

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

Vulnerability details

Impact

The function rescueETH is supposed to withdraw ETH stored in the contracts, but instead they transfer msg.value sent from the msg.sender to the destination address. Any ETH held by the 2 contracts are locked forever.

Recommended Mitigation Steps

msg.value should be replaced with address(this).balance and the function itself does not have to be payable as it should not require any payments.

1. Summary: Typo in OrderTypes.sol Location: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/libs/OrderTypes.sol#L39 Issue: ordes -> orders

2. Summary: Typo in InfinityExchange.sol Location: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L58 Issue: adress -> address

3. Summary: storage variable name is not properly camelcased Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L23 Issue: the storage variable userstakedAmounts should be renamed to userStakedAmounts because user and staked are two words.

4. Summary: ReentrancyGuard in InfinityStaker seems unnecessary. Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L15 Issue: ReentrancyGuard does not seem to be useful for InfinityStaker because the only token being transferred is INFINITY_TOKEN, which is an ERC-20 token deployed by the team and not an ERC-777 token with callback hooks after each transfer. It can reduce code size and save gas by removing it.

5. Summary: There should be a ceiling to PROTOCOL_FEE_BPS Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266 Issue: There should be a hard coded ceiling for PROTOCOL_FEE_BPS, in case the contract owner is compromised (even if it is a multi-sig or a token governed time-locked contract).

1. Summary: userMinOrderNonce[msg.sender] should be cached in memory Location: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L380 Issue: The function is reading from the storage the value userMinOrderNonce[msg.sender] twice inL380 and L381. The value should be cached in a memory variable to reduce gas cost. Based on hardhat-gas-report, the average gas used for cancelAllOrders was 39017 and after the optimization the average gas used is 38855.

2. Summary: The size of isOrderValid can be reduced by re-writing the code Location: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L520 Issue: The logic in L520

if (
  orderExpired ||
  !sigValid ||
  order.signer == address(0) ||
  !_complications.contains(order.execParams[0]) ||
  !_currencies.contains(order.execParams[1])
) {
  return false;
}
return true;

can be re-written as

return (
  !orderExpired &&
  sigValid &&
  order.signer != address(0) &&
  _complications.contains(order.execParams[0]) &&
  _currencies.contains(order.execParams[1])
);

According to hardhat, the contract size is 0.025 KB smaller this way.

3. Summary: The function getUserStakeLevel can be re-written to reduce number of checks as well as code size. Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L210 Issue: Instead of checking a user’s stake level from bronze to platinum, checking the user’s stake level from platinum to bronze will reduce the number of checks and still achieve the same result.

if (totalPower > PLATINUM_STAKE_THRESHOLD) {
  return StakeLevel.PLATINUM;
} else if (totalPower > GOLD_STAKE_THRESHOLD) {
  return StakeLevel.GOLD;
} else if (totalPower > SILVER_STAKE_THRESHOLD) {
  return StakeLevel.SILVER;
} else if (totalPower > BRONZE_STAKE_THRESHOLD) {
  return StakeLevel.BRONZE;
} else {
  return StakeLevel.NONE;
}

4. Summary: getVestedAmount should not read the amount variable pre-maturely Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L261 Issue: Instead of reading the amount storage variable in the first line of the function, defer the read until the very last line like this

return secondsSinceStake >= durationInSeconds ? userStakedAmounts[user][duration].amount : 0;

secondsSinceStake can be less than durationInSeconds and thus an
unnecessary expensive operation can be avoided. The average gas spent for rageQuit and unstake before the optimization are 88626 and 71345, and after the optimization are 88378 and 66758.

5. Summary: Storage variables in InfinityStaker.sol can be packed. Location: https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L25 Issue: the storage variable INFINITY_TOKEN can be moved to below the storage variable TWELVE_MONTH_PENALTY to save 1 storage slot. Currently the contract uses 5 storage slots according to hardhat-storage-layout. By moving INFINITY_TOKEN, only 4 storage variables are required.