Liquidity providers may lose funds when adding liquidity

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L421-L423

Vulnerability details

Impact

Liquidity providers may lose a portion of provided liquidity in either of the pair tokens. While the minLpTokenAmount protects from slippage when adding liquidity, it doesn't protect from providing liquidity at different K.

Proof of Concept

The Pair contract is designed to receive liquidity from liquidity providers (Pair.sol#L63). First liquidity provider in a pool may provide arbitrary token amounts and set the initial price (Pair.sol#L425-L426), but all other liquidity providers must provide liquidity proportionally to current pool reserves (Pair.sol#L420-L423). Since a pool is made of two tokens and liquidity is provided in both tokens, there's a possibility for a discrepancy: token amounts may be provided in different proportions. When this happens, the smaller of the proportions is chosen to calculate the amount of LP tokens minted (Pair.sol#L420-L423):

// calculate amount of lp tokens as a fraction of existing reserves
uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
return Math.min(baseTokenShare, fractionalTokenShare);

As a result, the difference in proportions will create an excess of tokens that won't be redeemable for the amount of LP tokens minted. The excess of tokens gets, basically, donated to the pool: it'll be shared among all liquidity providers of the pool. While the minLpTokenAmount argument of the add function (Pair.sol#L63) allows liquidity providers to set the minimal amount of LP tokens they want to receive, it doesn't allow them to minimize the disproportion of token amounts or avoid it at all.

// test/Pair/unit.Add.t.sol

function testLockOfFunds_AUDIT() public {
    address alice = address(0x31337);
    address bob = address(0x12345);
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    deal(address(usd), alice, 100e18, true);
    deal(address(usd), bob, 100e18, true);
    deal(address(p), alice, 100e18, true);
    deal(address(p), bob, 100e18, true);

    // Alice is the first liquidity provider.
    vm.startPrank(alice);
    usd.approve(address(p), type(uint256).max);
    p.add(10 ether, 10 ether, 0);
    vm.stopPrank();

    // Bob provides liquidity to the pool and sets the minimal LP amount.
    // The token amounts are deposited in different proportions, thus the smaller
    // one will be chosen to calculate the amount of LP tokens Bob will receive.
    vm.startPrank(bob);
    usd.approve(address(p), type(uint256).max);
    uint256 minLPAmount = 1e18;
    uint256 bobLPAmount = p.add(1.2 ether, 1 ether, minLPAmount);
    vm.stopPrank();

    // Bob has received the minimal LP amount he wanted.
    assertEq(bobLPAmount, minLPAmount);

    // However, after removing all his liquidity from the pool...
    (uint256 bobUSDBefore, uint256 bobFracBefore) = (usd.balanceOf(bob), p.balanceOf(bob));
    vm.prank(bob);
    p.remove(minLPAmount, 0, 0);
    (uint256 bobUSDAfter, uint256 bobFracAfter) = (usd.balanceOf(bob), p.balanceOf(bob));

    // ... Bob received less USD than he deposited.
    assertEq(bobUSDAfter - bobUSDBefore, 1.018181818181818181 ether);
    assertEq(bobFracAfter - bobFracBefore, 1.000000000000000000 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

In the add function, consider calculating optimal token amounts based on the amounts specified by user, current pool reserves, and the minimal LP tokens amount specified by user. As a reference, consider this piece from the Uniswap V2 Router: UniswapV2Router02.sol#L45-L60.

[Shungy]

Dup #90

[c4-judge]

berndartmueller marked the issue as primary issue

[c4-sponsor]

outdoteth marked the issue as sponsor confirmed

[outdoteth]

Fixed in: outdoteth/caviar#2

By allowing a user to specify a minPrice and maxPrice that they are willing to LP at along with the minLpTokenAmount that they would like to receive. The price calculation is based on this: https://github.com/outdoteth/caviar/blob/main/src/Pair.sol#L471

[Shungy]

I have to point out I think this fix is insufficient.
It fixes the improper slippage check mentioned in #108
It does not fix extra input given as slippage tolerance refunded.
So with this users have to either set 0% slippage and they can get DOSed, or they can provide some slippage tolerance and unnecessarily lose tokens.

So with the current fix, any price change within the slippage tolerance will always result in more tokens than necessary being withdrawn from the user.

Consider checking the recommended fix in #90

[c4-judge]

berndartmueller marked the issue as selected for report