Infinity NFT Marketplace contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

One can take orders which do not match. Possibly take some items which was not meant to be taken.

Proof of Concept

gist: describe("DuplicatesInOrder") This only shows the pard canExecTakeOrder returns false information. Due to limited time, could not write full PoC.

Tools Used

Hardhat

Recommended Mitigation Steps

The logic in doTokenIdsIntersect and doItemIntersect should be re-written to consider duplicated items.

Detail

The doTokenIdsIntersect and doItemsIntersect may give false positive in the case of duplicated items. Since the InfinityExchange supports ERC1155 standard, duplicated items in an order should be take into account. The test case DuplicatesInOrder in this gist demonstrates that canExecTakeOrder will return true even though the orders do not match. One scenario to exploit this is following. Alice makes a sell order of ERC1155 item 1 and 2 (Let's say item 1 is more valuable). She has multiple of item 1, and she approves multiple of itme 1, for convenience, because she made other orders including item 1. An attacker Bob sees this and takeOrders with list of item 1 and 1. Even though these orders do not match, it will pass the canExecTakeOrder and The transfer of NFT will be done based on the taker's order. As the result, Bob could get two of item 1 instead of item 1 and item2. Due to time constraint, I could not write PoC, but based on the code it seems possible. Also, the same logic is used for other match functions as well, so there could be also potential exploits.

Lines of code

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

Vulnerability details

Impact

When the NFT contract fails to follow ERC162 standard, it is possible to get paid without transferring the order item.

Proof of Concept

example for nonstandard NFT test file: NonstandardNFT The nonstandardNFT is OpenZeppelin ERC721 implementation, except supportsInterface is overriden. The test shows, even though no item is transferred, the currency is still paid.

Tools Used

Hardhat

Recommended Mitigation Steps

Add else to revert when the item gives false from supportsInterface.

Detail

The _transferNFTs does not check for non ERC165 standard case. Although ERC721 and ERC1155 must implement ERC165's supportsInterface function, this contract may be used with non-ERC165 contract with or without malicious intent. It maybe possible some NFTs simply fail to implement supportsInterface function or some evil entity might tailor NFT contract to abuse this function. Also old NFTs predate ERC721 might not have this function (for example CryptoPunk v1). If a person can convince a victim to buy this non-ERC165, the NFT will not be transferred, yet the agreed currency will be transferred.

Infinity NFT QA report

Summary

• using array instead of struct makes it harder to understand the code and error prone, since it is easy to make a mistake with indices.

Low

isNonceValid returns wrong value in InfinityExchange

• InfinityExchange.sol:413
• InfinityExchange.sol:516
• gist link to a test (see TestIsNonceValid) When the nonce is equal to userMinOrderNonce, the isNonceValid returns false. However, isOrderValid will consider the order valid and the order will go through. The isNonceValid function is external, so it is not used within this contract. But if it is used off-chain, It will create discrepancy between this contract and whoever using isNonceValid function and potential vulnerability.

Lack of zero address check for INFINITY_TREASURY in InfinityStaker

• InfinityStaker.sol:51
• InfinityStaker.sol:376
• diff to the test/staker.js Setting INFINITY_TREASURY to zero address will make rageQuit impossible. Currently, the INFINITY_TREASURE can be set to zero address in the constructor as well as updateInfinityTreasury. This will result to revert upon rageQuit, as the safeTransfer to INFINITY_TREASURY will revert, upon transferring to a zero address. If it is meant to be a security feature - to be able to pause the rageQuit, then it should be implemented explicitly, as the current implementation will mislead users to believe that they can withdraw at least some amount of money when they want. Impact: The owner of the contract can make impossible to rageQuit Mitigation: Add zero address check for updateInfinityTreasury

Lack of zero value check for penalties in InfinityStaker

• InfinityStaker.sol:369-371
• InfinityStaker.sol:196
• diff to the test/staker.js Setting penalty values to zero will make rageQuit impossible. The variable THREE_MONTH_PENALTY, SIX_MONTH_PENALTY and TWELVE_MONTH_PENALTY can be set to zero. It will result to revert in the line 196 for division by zero. If it is meant to be a security feature - to be able to pause the rageQuit, then it should be implemented explicitly, as the current implementation will mislead users to believe that they can withdraw at least some amount of money when they want. Impact: The owner of the contract can make impossible to rageQuit Mitigation: Add zero value check for penalties in updatePenalties function

Timestamp when amount is zero in InfinityStaker

• InfinityStaker.sol:313-323
• diff to the test/staker.js When the unstaking amount matches exactly to the staked value, the amount is set correctly to zero, but not the timestamp.

Non-critical

Naming of currentEpochTimestamp in InfinityToken

• InfinityToken.sol:73 In the current code, the currentEpochTimestamp is set to the contract's deployed timestamp in the constructor and is seemingly never updated. The naming is therefore confusing, since the currentEpoch is updated as epoch is updated. Also previousEpochTimestamp is updated when advanceEpoch is called, which makes the previousEpochTimestamp will be advanced than currentEpochstamp. Since it is also a public variable, somebody querying the currentEpochTimestamp might misunderstand the meaning.

Missing Zero Address check for admin in TimelockConfig

• TimelockConfig.sol:44

  constructor(address admin, uint256 timelock) {
    _setRawConfig(ADMIN, uint256(uint160((admin))));
    _setRawConfig(TIMELOCK, timelock);
  }

• TimelockConfig.sol:118 requestChange
• TimelockConfig.sol:50 confirmChange
• gist link to the diff from test/token.js There is no zero address check upon updating the admin address via confirmChange and requestChange. The admin address cannot be updated without access to the admin account. Therefore, the config of InfinityToken cannot be changed once the admin address is set to zero. If it is meant to be a way to renounce the admin account, to fix the given config set, it is safer to have a separate function to prevent accidents. Impack: potentially lose the admin access forever, which means no more config changes possible. Mitigation: when the config id is ADMIN, check for zero address. Add a separate function for renounce admin. To be really sure, implement two step for transferring admin (nomination from the current admin, confirming then nomination from the new admin).

Missing Zero Address check for INFINITY_TOKEN in InfinityStaker

• InfinityStaker.sol:50 There is no check when setting the _tokenAddress in the constructor. If a wrong address is set, many functionality of this contract cannot be used. Since there is no way to change the INFINITY_TOKEN address, it is safe to check for the zero address. Also the token address will not be changed, it can also be immutable to save gas. If it should be always deployed with the InfinityToken, checking codehash of the input address and compare it to the InfinityToken contract's hash will make sure of that.

Misleading revert message, Typo in comment in InfinityStaker

• InfinityStaker.sol:111-117

  /**
   * @notice Untake tokens
   * @dev Storage updates are done for each stake level. See _updateUserStakedAmounts for more details
   * @param amount Amount of tokens to unstake
   */
  function unstake(uint256 amount) external override nonReentrant whenNotPaused {
    require(amount != 0, 'stake amount cant be 0');

• Untake -> Unstake on line 112
• line 117, 'stake amount cant be 0' reads that the revert happens in during stake process

Lack of protocol fee bps check in InfinityExchange

• InfinityExchange.sol:1267

  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    PROTOCOL_FEE_BPS = _protocolFeeBps;
    emit NewProtocolFee(_protocolFeeBps);
  }

If the PROTOCOL_FEE_BPS is set to be higher than 10000, trabsferFees will revert. It is safer the check upon setting the value, to prevent accidental downtime.

Typo in comment in InfinityExchange

• InfinityExchange.sol:58

  /// @dev This is the adress that is used to send auto sniped orders for execution on chain

adress should be address