Dangerous use of setVirtualReserves(), withdraw(), and execute() leads to incorrect configuration of PrivatePool

[code423n4]

Lines of code

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

Vulnerability details

Impact

The function setVirtualReserves() allows arbitrary changes to the values of virtualBaseTokenReserves and virtualNftReserves, which determine the price of the NFT trade in the pool. However, the real balance of tokens or ether in the pool is not checked, so in reality, virtual reserves may not correspond to reality. Setting incorrect configuration can be front-run by a hacker to make a more advantageous deal, even if in reality the balances do not support the correctness of the deal.

For example, if the owner of a private pool accidentally sets very low token reserves, a hacker can buy all NFTs from the pool at a fraction of their value.

Incorrect configuration can also be used by the owner intentionally for slippage attacks on traders.

Even if the pool owner initially created the pool correctly and set the values of virtualBaseTokenReserves and virtualNftReserves correctly, this configuration can be accidentally or intentionally violated when the owner calls withdraw(), or if they call execute() which calls baseToken.transfer(...). This will result in there not being enough funds in the pool to make trades, even though virtual reserves will suggest otherwise.

Recommended Mitigation Steps

It is recommended to change the balances and set them in one transaction. Similarly, setMerkleRoot() can be combined with setVirtualReserves(). EthRouter.deposit() function can also be combined with setVirtualReserves() so that they are executed in one transaction.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[outdoteth]

It's intentional that the virtual reserves may not match up with the actual reserves. This allows for concentrated liquidity. This is documented and also indicated in the name itself; "virtual".

[c4-sponsor]

outdoteth marked the issue as sponsor disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[GalloDaSballo]

VirtualReserves allow for custom pricing, no vulnerability was demonstrated

If you have further proof, please follow up with a coded POC