Infinity NFT Marketplace contest - KIntern's results

Lines of code

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

Vulnerability details

Impact

Function InfinityStaker.rescueETH() should rescue any ETH accidentally sent to the contract. But because of using msg.value, instead of transfering ETH of contract, it just transfers ETH of caller to destination.

Proof of Concept

1. Alice accidentally sends 1 ETH into InfinityStaker
2. Bob is owner of InfinityStaker and he try to rescue Alice’s ETH by calling rescueETH with destination = address(Alice) and msg.value = 1 ETH.
3. Alice will receive 1 ETH back, but 1 ETH still stuck in InfinityStaker contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Add an amount parameter in rescueETH function and remove payable tag.

Lines of code

https://github.com/infinitydotxyz/exchange-contracts-v2/blob/c51b7e8af6f95cc0a3b5489369cbc7cee060434b/contracts/core/InfinityOrderBookComplication.sol#L205

Vulnerability details

Impact

Function matchOrders uses custom constraints to make the matching more flexible, allow seller/buyer to specify maximum/minimum number of NFTs they want to sell/buy. This function first does some checks and then execute matching.

But in function areNumItemsValid(), there is a wrong checking will lead to wrong logic in matchOrders() function.

Instead of checking if numConstructedItems <= sell.constraints[0] or not, function areNumItemsValid() check if buy.constraints[0] <= sell.constraints[0]. It will lead to the scenario that numConstructedItems > sell.constraints[0] and make the seller sell more number of nfts than he/she allow.

Proof of concept

Consider the scenario

1. Alice create a sell order to sell maximum 2 in her 3 BAYC with ids [1, 2, 3]
2. Bob create a buy order to buy mimimum any 2 BAYC with id in list [1, 2, 3]
3. Match executor call matchOrders() to match Alice's order and Bob's one with parameter constructs = [1, 2, 3]
4. Function matchOrders will transfer all NFT in construct list (3 NFTs 1, 2, 3) from seller to buyer even though seller only want to sell maximum 2 NFTs.

For more information, please check this PoC. https://gist.github.com/minhquanym/a95c8652de8431c5d1d24aa4076a1878

Tools Used

hardhat, chai

Recommended Mitigation Steps

Replace check buy.constraints[0] <= sell.constraints[0] with numConstructedItems <= sell.constraints[0]

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L128-L143

Vulnerability details

Impact

• Wrong function logic: 2 orders both contain constructedItem but can't be match.

Proof of Concept

Function canExecMatchOrder() will check whether match orders with a higher order intent can be executed as in the comment. It can be done by checking buyOrder and sellOrder contain constructedItem or not.

But this function contains a redundant check doItemsIntersect(sell.nfts, buy.nfts). This check will make the whole logic of function wrong.

For example:

• sellOrder = [1, 2, 3]
• buyOrder = [2, 3, 5]
• constructedItem = [2, 3]

In example above, sellOrder and buyOrder are both contain constructedItem, but doItemsIntersect(sellOrder, buyOrder) will return false and make the whole function return false.

Tools Used

Manual review

Recommended Mitigation Steps

remove check in line 140

Should return false instead of require because of function's purpose (InfinityOrderBookComplication.sol)

Affected code

• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L255

Proof of concept

• function doItemsIntersect will return true if 2 item is intersected, return false if not. But in line 255, if not tokendIdsIntersect, it will revert the transaction.

Recommend mitigation step

• Instead of using require statement, we can return false in line 255

if (!tokenIdsIntersect) {
    return false;
}

Inconsistent naming of variables in InfinityStaker.unstake

• In the InfinityStaker.unstake function, variables are following camel case naming convention (e.g vestedThreeMonths and vestedTwelveMonths, but vestedsixMonths is not.

Proof of concept

• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L120

Recommended Mitigation Steps

• Change vestedsixMonths to vestedSixMonths

Immutable variable do not need storage slot

• In the InfinityExchange, variable WETH is immutable. The compiler does not reserve a storage slot for this variable, and every occurrence is replaced by the respective value.
• So caching this variable actually doesn't save gas like the comment said

Affected code

• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L147
• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L198
• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L271

typo comment

• https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L60 @notice Checks whether one to matches matches can be executed -> @notice Checks whether one to many matches can be executed