Caviar Private Pools - neumo's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L731-L738 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L416 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L421-L438

Vulnerability details

Impact

While the protocol takes fees when users buy/sell NFTs in the private pool with a rate set by the protocol owner in the Factory contract, when users change a set of NFTs for another set, the protocol fees depend on the value of changeFee, which can be set arbitrarily by the owner of the private pool when creating it. Protocol fees should be a percentage of the value of the NFTs that take part in the exchange, and not a percentage of the fixed fee set by the private pool owner.

Proof of Concept

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

If the owner sets the value of changeFee to zero, feePerNft and feeAmount would also be zero and so would be protocolFeeAmount, so the protocol would not receive any fees when users change NFTs.

Tools Used

Manual review

Recommended Mitigation Steps

A possible approach would be to separate the calculation of the fees to the pool owner from those to the protocol, by calculatig the protocol fees as the average of the buy and sell quotes (using sellQuote and buyQuote).