Owner's ability to set any arbitrary virtualReserves allowes him to steal users funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

The owner of PrivatePool has the ability to set the virtualReserve to any arbitrary value he wants. There is no upper or lower limit.

When the user makes a call to buy() or sell() of PrivatePool, the owner can sandwich the user's transaction and set the virtualReserves in such a way that,
-1. The user might have to pay a very big amount of baseToken to buy the NFTs
-2. The user might not receive any baseToken in return while selling the NFTs

In EthRouter.sol, there is slippage protection for buy() and sell(), but the owner can sandwich attack the user's transaction, making sure the user will receive or pay the limit he provided in the argument.

Proof of Concept

Assume,
Current virtualBaseTokenReserves = 100e18, virtualNftReserves = 40e18
-1. Alice calls buy() of PrivatePool with tokenWeights = 10e18,
-2. The attacker front-run her transaction and calls setVirtualReserves(), with a very high value of virtualBaseTokenReserves or a very low value of virtualNftReserves or both. There is no limit to function.
- virtualBaseTokenReserves = 1000e18
- virtualNftReserves = 11e18
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L538-L545
-3. Alice's transaction goes through
- In buy, inputAmount is calculated using the following formula,
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L694-L706
- inputAmount = (10e18 * 1000e18) / (11e18 - 10e18)
- inputAmount = 10000e18
- So, inputAmount would be very high instead it should have been 33.33e18 according to actual virtualreserve
- Remember, Owner can always check user's balance and manipulate the price accordingly
-4. Owner set the virtualReserve to normal range

During sell(), the Owner can manipulate the reserve in such a way that the user will not receive any baseToken in return for their NFTs by lowering the virtualBaseTokenReserves or increasing the virtualNftReserves
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L713-L724

Tools Used

Manual Review

Recommended Mitigation Steps

There should be some limit by which the owner can manipulate the reserve. Ideally it should follow xy=k

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[outdoteth]

Sandwich attack is a known risk and is documented in the code with a comment that even warns against it: https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L204. The slippage check in the router contract serves to protect against this. Sandwiching is an issue in almost all AMM designs. I'm not sure if this is valid or not.

[c4-sponsor]

outdoteth requested judge review

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[c4-sponsor]

outdoteth marked the issue as sponsor disputed

[GalloDaSballo]

The router protects against this

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Invalid