Caviar Private Pools - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

The private pool does not have any restriction about the base token, so the creator can initialize the private pool with any token. The problem is the the private pool can be initialized with tokens of 3 decimals or less causing reverts in some functions.

The changeFeeQuote() function will be reverted by an arithmetic error causing that all trades via change() function will fail.

Proof of Concept

I created a test where a private pool is created with a 3 decimals token, then the changeFeeQuote() function will be reverted by an arithmeticError causing that all the trades via change() function will fail.

File: Change.t.sol
324:     function test_RevertIfWithThreeDecimalsToken() public {
325:         // PrivatePools does not support tokens with decimals less than four.
326:         // 1. Create a private pool with 3 decimals token.
327:         // 2. The changeFeeQuote() function will fail due an arithmeticError causing that
328:         //    all the trades via change() function will fail.
329:         //
330:         // 1. Create a private pool with 3 decimals token.
331:         //
332:         privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
333:         privatePool.initialize(
334:             address(threeDecimalsToken),
335:             nft,
336:             virtualBaseTokenReserves,
337:             virtualNftReserves,
338:             changeFee,
339:             feeRate,
340:             merkleRoot,
341:             true,
342:             false
343:         );
344:         //
345:         // 2. The changeFeeQuote() function will fail due an arithmeticError, causing that
346:         //    all the trades via change() function will fail.
347:         //
348:         vm.expectRevert(stdError.arithmeticError);
349:         privatePool.changeFeeQuote(1);
350:     }

Tools used

VScode/Foundry

Recommended Mitigation Steps

In the private pool creation validates that the base token is 4 decimals places or more.

1 - There is a discrepancy between the documentation and the code in the PrivatePool.change() function

The documentaion about the change() function says: The sum of the caller's NFT weights must be less than or equal to the sum of the output pool NFTs weights. Additionally in the next code comment it says: The sum of the caller's NFT weights must be less than or equal to the sum of the output pool NFTs weights.

The problem is that the code is totally the opposite, the code will revert if the sum of the caller's NFT weights is less.

if (inputWeightSum < outputWeightSum) revert InsufficientInputWeight();

Fix the discrepancy between the code and the documentation.

2 - The flashLoan fee token should use the flashFeeToken() to get the fee token

The flash loan fee token used in the next line:

if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

It uses the baseToken for the fee token but it should use the flashFeeToken() function instead to get the flash fee token.

3 - Add a function that helps to update the changeFee

The changeFee is initialized in the beginning and it can not be updated in the future by the private pool owner.

Add a updateChangeFee() function that helps to update the changeFee if it is necessary.

4 - In the Factory.create() function the PrivatePool.deposit() can be used instead, for the base token and NFTs deposit.

The Factory.create() function uses the next code that helps to deposit the baseToken and the NFTs. It can use the privatePool.deposit() instead and the code will be cleaner.

5 - The price() function does not represent the exact price in 18 decimals as expected

The price() function mention the next comment: Returns the price of the pool to 18 decimals of accuracy. but it returns the price based on the baseToken decimals which is incorrect.

uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());
return (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;

Please see the next situation:

• The virtualBaseTokenReserves is 10e18. The value is normalized with 18 decimals.
• The virtualNftReserves is 2e18. The value is normalize with 18 decimals.
• The baseToken is a token with 6 decimals.

Accordly to the formula, the result will be 5e30 which is incorrect because the real price should be 5e18 tokens for each NFT. Instead the formula should only return virtualBaseTokenReserves / virtualNftReserves since both values are 18 decimals normalized.