Infinity NFT Marketplace contest - k's results

Lines of code

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

Vulnerability details

Impact

The purpose of the doItemsIntersect() function is to determine whether the buy and sell orders match. No duplication check is performed, and as a result qualifying items can be included multiple times in a sell order. As such, the following is possible:

1. A buyer creates an order for 3 items with the following requirements:

• 1 item from collection A (ERC-721)
• 1 item from collection B (ERC-721)
• 1 item of token ID #10 from collection C (ERC-1155)

2. The expected behavior is that in order to fulfill this order, a seller will need to have each of these items.
3. However, if a seller has a balance of 3 token ID #10 from collection C, it's possible to fill this order by submitting a sell order comprising of only collection C tokens as the amount of tokens will match the required 3 and the collection will also correctly validate.

This will generally not be possible for ERC-721 tokens as they are non-fungible. When trying to perform the second transfer the call will fail as the seller will no longer be the owner of that NFT. It's likely still possible to bypass this by using afterTransferHook or onERC721Received hook, but no risk was observed in this path.

Proof of Concept

https://gist.github.com/kylriley/4db98ba6c67035c91dc35f5a5560f56d

Tools Used

N/A

Recommended Mitigation Steps

Validation should be added to order matching to ensure that there are no duplicate token IDs present.

Lines of code

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

Vulnerability details

Impact

If an NFT is sold that does not specify support for the ERC-721 or ERC-1155 standard interface, the sale will still succeed. In doing so, the seller will receive funds from the buyer, but the buyer will not receive any NFT from the seller. This could happen in the following cases:

1. a token that claims to be ERC-721/1155 compliant, but fails to implement the supportsInterface() function properly.
2. an NFT that follows a standard other than ERC-721/1155 and does not implement their EIP-165 interfaces.
3. a malicious contract that is deployed to take advantage of this behavior.

Proof of Concept

https://gist.github.com/kylriley/3bf0e03d79b3d62dd5a9224ca00c4cb9

Tools Used

N/A

Recommended Mitigation Steps

If neither the ERC-721 nor the ERC-1155 interface is supported the function should revert. An alternative approach would be to attempt a transferFrom and check the balance before and after to ensure that it succeeded.

1. To follow standard practices, the MATCH_EXECUTOR, WETH_TRANSFER_GAS_UNITS and PROTOCOL_FEE_BPS variables should use camel case as they are not constants. Using caps case gives the incorrect impression that they are immutable.
2. The setProtocolFee() function should validate that the _protocolFeeBps parameter is less than 10,000.
3. The Solidity version of 0.8.14 should be bumped to 0.8.15. Note: the bug does not seem to affect the code in scope as assembly is not used.
4. Some typos:

• The updateWethTranferGas() function should be updateWethTransferGas()
• ordes should be orders
• * @notice Checks whether one to matches matches can be executed - the first "matches" should be "many"

Use custom errors instead of require messages

Replacing all require statements with custom errors will reduce the gas cost of deployment, as well as runtime costs when the revert condition is met.