Infinity NFT Marketplace contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L205

Vulnerability details

Impact

sell.constraint[0], representing the max number of nft items, can be simply bypassed due to a missing condition. Hence order will process more nft than max limit and hence would cause loss to seller whose nft would be traded without his permission

Proof of Concept

1. Assuming below parameters:

sell has set constraint[0] as 2 (max number of item allowed)
buy has set constraint[0] as 1 (min number of item allowed)

2. Now canExecMatchOrder is called to check Order validity with constructedNFTs as {A,B,C}

3. areNumItemsValid function is called which makes below check:

return numConstructedItems >= buy.constraints[0] && buy.constraints[0] <= sell.constraints[0];

4. This will return true since 3>=1 && 1<=2

5. But this is incorrect since this order is simply bypassing the sell order max nft constraint which was 2. With this order processed, all 3 nfts will be processed even though only 2 should have been allowed

Recommended Mitigation Steps

Add a condition to verify that constructed nft items are not larger than sell constraints

return numConstructedItems >= buy.constraints[0] && buy.constraints[0] <= sell.constraints[0] && numConstructedItems<= sell.constraints[0];

Lines of code

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

Vulnerability details

Impact

There is no max cap on the fees in setProtocolFee function. Admin can have fees as 100_00 which is 100%. Now _transferFees function will make sure that seller gets nothing as remainingAmount becomes 0 and contract will get the whole amount.

Proof of Concept

1. Owner calls setProtocolFee and set _protocolFeeBps as 100_00 which is 100%

2. Now whenever NFT are transferred, _transferFees is called

3. This calculates the fees over the amount. Since fees is 100% so contract receives full amount and seller receives 0 amount

Recommended Mitigation Steps

Set a max fee cap on PROTOCOL_FEE_BPS so that only reasonable fees is deducted

function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
require(_protocolFeeBps<=MAX_FEES, "Exceeded max fees");
    PROTOCOL_FEE_BPS = _protocolFeeBps;
    emit NewProtocolFee(_protocolFeeBps);
  }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L140

Vulnerability details

Impact

canExecMatchOrder is having an incorrect check which makes a valid order as invalid. doItemsIntersect function is also checked on sell.nfts, buy.nfts which is incorrect. doItemsIntersect should only be checked in reference to constructedNfts

Proof of Concept

1. Assume buy has nfts {A,B,C}, sell has nft {A,B}, constructedNfts has nft {A}, buy.constraints[0]/sell.constraints[0]/numConstructedItems is 1

2. Ideally this order should match since constructedNfts {A} is present in both buy and sell

3. But this will not match since doItemsIntersect(sell.nfts, buy.nfts) will fail because of item C which is not present in sell

Recommended Mitigation Steps

Remove doItemsIntersect(sell.nfts, buy.nfts) from InfinityOrderBookComplication.sol#L140

Lines of code

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

Vulnerability details

Impact

In updatePenalties function, THREE_MONTH_PENALTY/SIX_MONTH_PENALTY/TWELVE_MONTH_PENALTY are not having any upper bound, nor any lower bound

1. Admin can set it to really high value which will force user to remain staked and never use rage quit.
2. Admin can disallow rageQuit by setting THREE_MONTH_PENALTY, SIX_MONTH_PENALTY, TWELVE_MONTH_PENALTY to 0

Proof of Concept

1. Admin uses updatePenalties to set THREE_MONTH_PENALTY, SIX_MONTH_PENALTY, TWELVE_MONTH_PENALTY to 0

2. Now rage quit always fail since denominator becomes 0 at InfinityStaker.sol#L195 while calculating totalToUser in getRageQuitAmounts

3. This forces users to remain staked and would not be able to leave the contract

4. Same can also be done if owner sets insane high penalty forcing user to stay in system

Recommended Mitigation Steps

Add below check

require(threeMonthPenalty<=ThreeMonthPenaltyMAX && sixMonthPenalty<=SixMonthPenaltyMAX && twelveMonthPenalty<=TwelveMonthPenaltyMAX);
require(threeMonthPenalty!=0 && sixMonthPenalty!=0 && twelveMonthPenalty!=0);