Caviar Private Pools - yixxas's results

Lines of code

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

Vulnerability details

Impact

Base tokens with less than 4 decimals cannot utilise changeFeeQuote(). This means that such token is used, users of the pool cannot make a change() of NFT since a change fee is paid on each change(). However, buying and selling of NFTs to the pool is still allowed.

Proof of Concept

Base tokens with less than 4 decimals will revert when changeFeeQuote() is called. Calculation in exponent assumes that baseToken.decimals >= 4.

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;
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider allowing base tokens with less than 4 decimals to still be changable, considering that both buy and sell are still allowed.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L235-L249

Vulnerability details

Impact

User can reduce the royalty fee paid by purchasing a NFTs in a group with big difference in weight.

Proof of Concept

In the calculations of royalities, royalty fee is based on salePrice, which is averaged out across all NFTs even if they have different weights. This can lead to extremely wrong royality fees caculated.

Assuming virtualBaseTokenReserves = 100000, virtualNftReserves = 2000, royaltyFee = 10%, and user is buying 2 NFT, 1 with weight = 1 and the other with weight = 999

To calculate netInputAmount, we take the weight sum

(netInputAmount, feeAmount, protocolFeeAmount) = buyQuote(weightSum);

function buyQuote(uint256 outputAmount)
	public
	view
	returns (uint256 netInputAmount, uint256 feeAmount, uint256 protocolFeeAmount)
{
	// calculate the input amount based on xy=k invariant and round up by 1 wei
	uint256 inputAmount =
		FixedPointMathLib.mulDivUp(outputAmount, virtualBaseTokenReserves, (virtualNftReserves - outputAmount));

	protocolFeeAmount = inputAmount * Factory(factory).protocolFeeRate() / 10_000;
	feeAmount = inputAmount * feeRate / 10_000;
	netInputAmount = inputAmount + feeAmount + protocolFeeAmount;
}

For simplicity, we assume that there are no other fees.

outputAmount = weight = 1000

then netInputAmount = ceil(1000 * 100000 / (2000 - 1000)) = 100000

Total royaltyFeeAmount = (10% of (100000 / 2) ) * 2 = 10000

---

 

On the other hand, if NFT is bought separately instead,

first buy weight = 1 NFT,

netInputAmount = ceil(1 * 100000 / (2000 - 1)) = 51

new virtualBaseTokenReserves = 100000 + 51 = 100051 new virtualNftReserves = 2000 - 1 = 1999

royaltyFeeAmount = 10% of 51 = 5.1

next buys weight = 999 NFT,

netInputAmount = ceil(999 * 100051 / (1999 - 999)) = 99951 royaltyFeeAmount = 10% of 100961 = 10096.1

Total royaltyFeeAmount = 5.1 + 10096.1 = 10101.2

In this case, the user who is buying NFTs as a group is paying about 1% less in royalities. In fact, the bigger the weight difference in the buy token list and with more NFTs to average out, the difference would be larger.

For example, if a user buys NFTs with weight 1, 1, 1, 1, 1, 1, 1000000, the difference in fees paid would be much larger than 1%.

Tools Used

Manual Review

Recommended Mitigation Steps

We should consider the weight of NFT when calculating royalty fee instead of assuming all of them having the same price.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/Factory.sol#L92

Vulnerability details

Impact

Creation of new private pools can be frontrun by an adversary, preventing pools from being created.

Proof of Concept

create() uses cloneDeterministic as seen below.

privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

This clone pattern uses the CREATE2 opcode. This means that, if implementation address and salt used is the same, it will revert as the resulting contract is already existing. In our case, _salt is an user input freely choosable by users. create() is also a public function. This means that an adversary can scan the mempool, look for calls to create() and frontrun this transaction with the same salt. The original transaction would revert, preventing legitimate users from creating a pool.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using clone instead of cloneDeterministic which deploys a contract with the CREATE opcode.