Swaps affect LP token mint/burn during liquidity addition/removal

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L463-L490

Vulnerability details

Impact

The LP token removal/addition forces a recalculation of the bonding curve, and the utility of the curve. The utility curve in proteus looks like the graph below, where the point A represents a certain composition of the pool.

If we try to remove add/remove liquidity form the pool, the new utility value needs to be calculated, and the bonding curve basically shifts to accomodate the new state of the pool. Here the new state is shown by point B and the curve in blue.

The issues is that if the pool is at a different price C, and the new state goes to D due to the same amount of liquidity removal, the new point D is not on the bblue bonding curve. This shows that the final bonding curve due to liquidity addition/removal depends on the price in the AMM.

Thus users can manipulate the price in the AMM, and this affects how many tokens other users will get when they add/remove liquidity.

Proof of Concept

In this POC, a user adds liquidity of 1 ETH. The same action is repeated, but at a different price after a large swap.

It is shown that after the swap, the user gets less LP tokens for the same amount of ETH added. Thus attackers can frontrun large LP additions/removals to cause losses in this manner, essentially a form of sandwich attack. In protocols like Uni V3 this isnt possible since the users get to choose the exact price ticks they want to supply to.

function testAttack() public {
    uint256 x0 = 100e18;
    uint256 y0 = 100e18;
    uint256 s0 = 100e18;
    uint256 depositedAmount = 1e18;

    SpecifiedToken depositedToken = SpecifiedToken.X;
    uint256 minted1 = DUT.depositGivenInputAmount(
        x0,
        y0,
        s0,
        depositedAmount,
        depositedToken
    );

    emit log_named_uint("minted1", minted1);

    //swap
    uint256 swapvalX = 100e18;
    uint256 swapvalY = DUT.swapGivenInputAmount(
        x0,
        y0,
        swapvalX,
        SpecifiedToken.X
    );

    // New values
    uint256 x1 = x0 + swapvalX;
    uint256 y1 = y0 - swapvalY;

    uint256 minted2 = DUT.depositGivenInputAmount(
        x1,
        y1,
        s0,
        depositedAmount,
        depositedToken
    );

    emit log_named_uint("minted2", minted2);
}

The output shows:

Running 1 test for src/test/EvolvingProteusProperties.t.sol:EvolvingProteusProperties
[PASS] testAttack() (gas: 140259)
Logs:
  minted1: 6801162225772413
  minted2: 6718681842515858

This shows that the user got lesser amount of tokens in the second minting.

Tools Used

Foundry

Recommended Mitigation Steps

Develop a price resistant way of calculating LP tokens.

Assessed type

MEV

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[viraj124]

the pool behaviour depends on the price configuration in the pool that is set at deployment so the price transitions after different operations also depend on that & if we have a scenario where the market price of an asset varies too much from the price configuration that was set then we can add a new evolving proteus implementation as we use a minimal proxy mechanism https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/LiquidityPoolProxy.sol#L48 so this is a known/expected behaviour

https://github.com/code-423n4/2023-08-shell-findings/issues/260#issuecomment-1701889491 for some more context

[c4-sponsor]

viraj124 (sponsor) disputed

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid