Infinity NFT Marketplace contest - VAD37's results

Judge has assessed an item in Issue #270 as Medium risk. The relevant finding follows:

6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user

With PROTOCOL_FEE_BPS > 10000 (more than 100%), the exchange can steal user WETH who might approve max WETH allowance for InfinityExchange.sol To provide safety for user and in case of compromised both admin and MATCH_EXECUTOR bot server. Protocol should prevent potential further damage that might steal WETH from all user approve Exchange before.

function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    require(_protocolFeeBps <= 1000); //max 10%
    PROTOCOL_FEE_BPS = _protocolFeeBps; 
    emit NewProtocolFee(_protocolFeeBps);
  }

With maximum amount of WETH_TRANSFER_GAS_UNITS is 4_294_967_296 and default value is 50_000, basically exchange can take any amount of WETH from user if gas price set high enough.

  function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
    require(_wethTransferGasUnits <= 200_000); // for deflation token or voting token transfer cannot really be more than 200000 gas per transaction
    WETH_TRANSFER_GAS_UNITS = _wethTransferGasUnits;
    emit NewWethTransferGasUnits(_wethTransferGasUnits);
  }

QA

1. QA
   1. 1.N- totalStaked >= 0 is always true
   2. 2.L- admin config updateStakeLevelThreshold() should call with array input and not spread out into multiple transactions
   3. 3.L- admin config updatePenalties() missing input check
   4. 4.L- Missing admin Zero-address check for updateInfinityTreasury()
   5. 5.N- Missing admin event in general
   6. 6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user
   7. 7.L- Not all NFT implement IERC165 interface and _transferNFTs() revert for case like this

1.N- totalStaked >= 0 is always true

Slither tautology error here. uint256 always >= 0. I think the dev here mean totalStaked > 0.

2.L- admin config updateStakeLevelThreshold() should call with array input and not spread out into multiple transactions

Since there is no implementation for user staking power yet. Admin should be aware of user might use protocol when admin changing staking level.

If admin update stake level of Bronze,Silver,Gold,Plat one by one between the average of 4 blocks (assuming using JS script). There is very small chance of user do something not intended as Threshold is changing.

    function _updateStakeLevelThreshold(StakeLevel stakeLevel, uint16 threshold) internal {
        if (stakeLevel == StakeLevel.BRONZE) {
        BRONZE_STAKE_THRESHOLD = threshold;
        } else if (stakeLevel == StakeLevel.SILVER) {
        SILVER_STAKE_THRESHOLD = threshold;
        } else if (stakeLevel == StakeLevel.GOLD) {
        GOLD_STAKE_THRESHOLD = threshold;
        } else if (stakeLevel == StakeLevel.PLATINUM) {
        PLATINUM_STAKE_THRESHOLD = threshold; //@audit missing input check
        }
    }

    function updateStakeLevelThreshold(StakeLevel[] stakeLevel, uint16[] threshold) external onlyOwner {
        for (uint8 i = 0; i < stakeLevel.length; i++) {
        _updateStakeLevelThreshold(stakeLevel[i], threshold[i]);
        }
        // emit event    
    }

    // Or this one is much better as there are fixed amount of enum and threshold 
    function updateStakeLevelThreshold(uint16 bronzeThreshold,uint16 silverThreshold, uint16 goldThresHold, uint16 platThresHold ) external onlyOwner 
    {
      require (silverThreshold >= bronzeThreshold, "Silver threshold must be greater than Bronze threshold");
      require (goldThresHold >= silverThreshold, "Gold threshold must be greater than Silver threshold");
      require (platThresHold >= goldThresHold, "Platinum threshold must be greater than Gold threshold");
      if(BRONZE_STAKE_THRESHOLD != bronzeThreshold) BRONZE_STAKE_THRESHOLD = bronzeThreshold;
      if(SILVER_STAKE_THRESHOLD != silverThreshold) SILVER_STAKE_THRESHOLD = silverThreshold;
      if(GOLD_STAKE_THRESHOLD != goldThresHold) GOLD_STAKE_THRESHOLD = goldThresHold;
      if(PLATINUM_STAKE_THRESHOLD != platThresHold) PLATINUM_STAKE_THRESHOLD = platThresHold;
      // emit event
    }

3.L- admin config updatePenalties() missing input check

Solidity throw error when divided by 0. Admin should prevent this so user wont be locked out of ragequit() early.

  function updatePenalties(
    uint16 threeMonthPenalty,
    uint16 sixMonthPenalty,
    uint16 twelveMonthPenalty
  ) external onlyOwner {
    require(threeMonthPenalty >= 1, 'Three month penalty must be greater than 1');
    require(sixMonthPenalty >= threeMonthPenalty, 'Six month penalty must be greater than three month penalty');
    require(twelveMonthPenalty >= sixMonthPenalty, 'Twelve month penalty must be greater than six month penalty');
    THREE_MONTH_PENALTY = threeMonthPenalty;
    SIX_MONTH_PENALTY = sixMonthPenalty;
    TWELVE_MONTH_PENALTY = twelveMonthPenalty;
    // emit event
  }

4.L- Missing admin Zero-address check for updateInfinityTreasury()

Cant have user send token to burn address thanks to javascript.

  function updateInfinityTreasury(address _infinityTreasury) external onlyOwner {
    require(_infinityTreasury != address(0), 'Invalid address');
    INFINITY_TREASURY = _infinityTreasury;
    // emit event
  }

5.N- Missing admin event in general

With admin event, it is for easy tracking from off chain for rare case like compromised admin key. It just simply look much better with it.

Missing event all of these functions:

• updateInfinityTreasury

• updatePenalties

• updateStakeLevelThrehold

• updateMatchExecutor

• complication and currency update

6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user

With PROTOCOL_FEE_BPS > 10000 (more than 100%), the exchange can steal user WETH who might approve max WETH allowance for InfinityExchange.sol To provide safety for user and in case of compromised both admin and MATCH_EXECUTOR bot server. Protocol should prevent potential further damage that might steal WETH from all user approve Exchange before.

function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    require(_protocolFeeBps <= 1000); //max 10%
    PROTOCOL_FEE_BPS = _protocolFeeBps; 
    emit NewProtocolFee(_protocolFeeBps);
  }

With maximum amount of WETH_TRANSFER_GAS_UNITS is 4_294_967_296 and default value is 50_000, basically exchange can take any amount of WETH from user if gas price set high enough.

  function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
    require(_wethTransferGasUnits <= 200_000); // for deflation token or voting token transfer cannot really be more than 200000 gas per transaction
    WETH_TRANSFER_GAS_UNITS = _wethTransferGasUnits;
    emit NewWethTransferGasUnits(_wethTransferGasUnits);
  }

7.L- Not all NFT implement IERC165 interface and _transferNFTs() revert for case like this

All NFT transfer goes through _transferNFTs(). But if-else only check for NFT contract support ERC721 or ERC1155. When nonstandard NFT does not have function to return support interface, the transaction revert.

Either add final else case as fail-safe call ERC721 anyway or throw error to prevent any malicious NFT from entering the exchange.