Caviar Private Pools - Koolex's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L152 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L219 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L270

Vulnerability details

Impact

in ETHRouter, on sell, deposit and change methods, ETHRouter approves the pool to transfer NFTs from the ETHRouter. For example:

	// approve the pair to transfer NFTs from the router
	ERC721(sells[i].nft).setApprovalForAll(sells[i].pool, true);

So it approves the pool for all token Ids under the NFT. However, it doesn't revoke the approval after the operation is finished. Now this might seem not risky on the surface. However, if any NFTs get stuck in (or sent to) ETHRouter, and because the pool is approvalForAll by ETHRouter, the pool owner could withdraw them via PrivatePool.execute. Please note that it is easy to get the approval of ETHRouter by doing the following:

1. Create a private pool with the targeted NFT
2. Deposit at least one token only via ETHRouter. This will execute setApprovalForAll for the targeted NFT.

In short, the ETHRouter should be permission-less and no contract should have the authority to withdraw tokens from it to mitigate any unknown exploits scenarios that could possibly occur in future.

Proof of Concept

See sell, deposit and change methods of EthRouter. they don't revoke the approval after the operation is finished.

Tools Used

Manual analysis

Recommended Mitigation Steps

Revoke the ERC721's approvalForAll of the pool after the operation (e.g. deposit, sell or change) is finished

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L733

Vulnerability details

Impact

PrivatePool.change method is used to change a set of NFTs that the caller owns for another set of NFTs in the pool. The caller has to pay the pool a change fee. To calculate the change fee, PrivatePool.changeFeeQuote is used. In the first line of the method, it deducts 4 from baseToken's decimals

	// multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
	uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;

Thus, getting the exponent which is used to calculate the feePerNft. The issue is that it doesn't take into account baseTokens with decimals less than 4. If the baseToken is less than 4, changeFeeQuote will always revert. This makes the PrivatePool.change method is unusable as it will always revert when calculating the fee.

Proof of Concept

• PrivatePool.changeFeeQuote method

	function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
		// multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
		uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
		uint256 feePerNft = changeFee * 10 ** exponent;

		feeAmount = inputAmount * feePerNft / 1e18;
		protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
	}

• PrivatePool.change method at line 416.

	// calculate the fee amount
	(feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider setting the exponent to a fixed value if the decimals is less than 4.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L211

Vulnerability details

Impact

on PrivatePool.buy, total royalty fee amount is calculated by adding the royaltyFee for all tokenIds.

	for (uint256 i = 0; i < tokenIds.length; i++) {
		// transfer the NFT to the caller
		ERC721(nft).safeTransferFrom(address(this), msg.sender, tokenIds[i]);

		if (payRoyalties) {
			// get the royalty fee for the NFT
			(uint256 royaltyFee,) = _getRoyalty(tokenIds[i], salePrice);

			// add the royalty fee to the total royalty fee amount
			royaltyFeeAmount += royaltyFee;
		}
	}

After that, it adds the royalty fee amount to the net input amount

	netInputAmount += royaltyFeeAmount;

Then after transferring the base token from the caller to the contract, It loops through tokenIds again to transfer the royalty fees to their respective recipients only If the recipient's address is not zero

	if (royaltyFee > 0 && recipient != address(0)) {
		if (baseToken != address(0)) {
			ERC20(baseToken).safeTransfer(recipient, royaltyFee);
		} else {
			recipient.safeTransferETH(royaltyFee);
		}
	}

The issue is that the buyer will pay extra ether (to the pool) if at least one recipient's address is zero.

Proof of Concept

When calculating the total royalty fee amount, it doesn't skip the royalty fee for zero recipient's addresses. However, only when transferring the fee, it does. An example:

1. Assume we have two tokens 1 and 3.
2. Token 1 has a royalty fee 0.5 ether with a recipient set to a non-zero address.
3. Token 3 has a royalty fee 0.5 ether with a recipient set to zero.
4. total royalty fee amount now is 1 ether.
5. 0.5 ether will be transferred to royalty recipient of token 1 and the other 0.5 ether will not be sent although the buyer pays it.

Note: this is applicable for ERC20 baseTokens as well.

Tools Used

Manual analysis

Recommended Mitigation Steps

Skip adding the fee to the total if recipient's address is zero.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L301

Vulnerability details

Impact

PrivatePool.sell method is used to sell NFTs into the pool and transfers base tokens to the caller. The royalty recipient receives royalty fee on sale. However, in case the baseToken is ETH, the royalty recipient could censor (i.e. block) the selling operation to certain pools. Therefore, not allowing the NFT owner to sell his token to those pools.

Proof of Concept

in PrivatePool.sell, the following code transfer the royalty fee to the recipient.

	// transfer the royalty fee to the recipient if it's greater than 0
	if (royaltyFee > 0 && recipient != address(0)) {
		if (baseToken != address(0)) {
			ERC20(baseToken).safeTransfer(recipient, royaltyFee);
		} else {
			recipient.safeTransferETH(royaltyFee);
		}
	}

If the recipient is a contract, it could simply revert upon receiving ether in receive() method of the recipient's contract.

Tools Used

Manual analysis

Recommended Mitigation Steps

Favor pull over push. for example:

• Track royalty fees balances (e.g. in a mapping) for their recipients.
• Add a method so the recipients can use to withdraw their royalty balances.