crimson-rat-reach - Deposit transactions lose funds to front-running when multiple fee tiers are available

[sherlock-admin]

crimson-rat-reach

high

Deposit transactions lose funds to front-running when multiple fee tiers are available

Summary

Deposit transactions lose funds to front-running when multiple fee tiers are available

Vulnerability Detail

The deposit transaction takes in minimum parameters for amount0 and amount1 of tokens that the user wishes to deposit, but no parameter for the minimum number of LP tokens the user expects to receive. A malicious actor can limit the number of LP tokens that the user receives in the following way:

A user Alice submits a transaction to deposit tokens into Multipool where (amount0Desired, amount0Min) > (amount1Desired, amount1Min)

A malicious actor Bob can front-run this transaction if there are multiple feeTiers:

• by first moving the price of feeTier1 to make tokenA very cheap (lots of tokenA in the pool)
• then moving the price of feeTier2 in opposite direction to make tokenB very cheap (lots of tokenB in the pool)

This results in reserves being balanced accross feeTiers, and the amounts resulting from _optimizeAmounts are balanced as well: https://github.com/sherlock-audit/2023-06-real-wagmi/blob/82a234a5c2c1fc1921c63265a9349b71d84675c4/concentrator/contracts/Multipool.sol#L780-L808

So the minimum amounts checks pass and but results as less LP tokens minted, because even though the reserves are balanced, they are also overinflated due to the large swap, and the ratio: https://github.com/sherlock-audit/2023-06-real-wagmi/blob/82a234a5c2c1fc1921c63265a9349b71d84675c4/concentrator/contracts/Multipool.sol#L468-L470

becomes a lot smaller than before the large swap

Impact

The user loses funds as a result of maximum slippage.

Code Snippet

Tool used

Manual Review

Recommendation

Add extra parameters for the minimum number of LP tokens that the user expects, instead of just checking non-zero amount: https://github.com/sherlock-audit/2023-06-real-wagmi/blob/82a234a5c2c1fc1921c63265a9349b71d84675c4/concentrator/contracts/Multipool.sol#L473

[SergeKireev]

Escalate

This should not be a duplicate of #180 but a unique high (see my escalation on #180 on why similar looking issues are invalid)

[sherlock-admin]

Escalate

This should not be a duplicate of #180 but a unique high (see my escalation on #180 on why similar looking issues are invalid)

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ctf-sec]

the issue still talk about the missing slippage when depositing / minting

in fact, the recommendation in this report is the same as the issue 180

duplicate of #180

[SergeKireev]

the issue still talk about the missing slippage when depositing / minting

in fact, the recommendation in this report is the same as the issue 180

duplicate of #180

Respectfully, the recommendation in this issue is absolutely different from the one in #180.

I summarized in my escalation on issue #180 why #180 should be invalid. Slippage protection works correctly unless multiple pools are involved

[fann95]

FIXED: We simplified the slip check and added the minimum acceptable amount of liquidity tokens to be minted. https://github.com/RealWagmi/concentrator/blob/fbccf1caf28edbb23db8c7e0409d0c40c3a56461/contracts/Multipool.sol#L415C19-L415C19

[hrishibhat]

Result: High Unique Considering this issue as a valid high since it opens a user to sandwiching and the funds at loss are unbounded.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• crimson-rat-reach: accepted

[huuducsc]

I believe this issue should be a low/non-issue, since the attacker can't increase both reserve0 and reserve1 of the Multipool contract through this scenario. This is due to the slippage of the UniswapV3 pool during the attacker's swaps, when you try to increase token0, you need a greater amount of token1 value.

A malicious actor Bob can front-run this transaction if there are multiple feeTiers:

by first moving the price of feeTier1 to make tokenA very cheap (lots of tokenA in the pool) then moving the price of feeTier2 in opposite direction to make tokenB very cheap (lots of tokenB in the pool)

Therefore, the reserves will not be "overinflated". Also, since attacker can't increase both reserve0 and reserve1, the LP shares of the user will not become a lot smaller than before the large swap.

uint256 l0 = (amount0Desired * _totalSupply) / reserve0;
uint256 l1 = (amount1Desired * _totalSupply) / reserve1;
lpAmount = l0 < l1 ? l0 : l1;

[SergeKireev]

I believe this issue should be a low/non-issue, since the attacker can't increase both reserve0 and reserve1 of the Multipool contract through this scenario. This is due to the slippage of the UniswapV3 pool during the attacker's swaps, when you try to increase token0, you need a greater amount of token1 value.

A malicious actor Bob can front-run this transaction if there are multiple feeTiers:

by first moving the price of feeTier1 to make tokenA very cheap (lots of tokenA in the pool) then moving the price of feeTier2 in opposite direction to make tokenB very cheap (lots of tokenB in the pool)

Therefore, the reserves will not be "overinflated". Also, since attacker can't increase both reserve0 and reserve1, the LP shares of the user will not become a lot smaller than before the large swap.

uint256 l0 = (amount0Desired * _totalSupply) / reserve0;
uint256 l1 = (amount1Desired * _totalSupply) / reserve1;
lpAmount = l0 < l1 ? l0 : l1;

The attacker can increase both reserves precisely because multiple underlying pools are available (multiple fee tiers), so the attacker adds more of token0 to one fee tier and more of token1 to the other fee tier. This was detailed in the scenario of the original submission.

The slippage during the first swap of the attacker does not matter, as a sandwicher always incurs heavy slippage during a sandwich and rebalances during back run

[huuducsc]

I know that an attacker can manipulate multiple pools with multiple fee tiers. However, when the attacker adds more token0 to the first fee tier, the token1 amount of the position in this UniswapV3 pool will decrease. Similarly, the token0 amount of the position in the second fee tier will decrease. Moreover, the decreased amounts hold more value than the increased amounts due to the slippage of the UniswapV3 pool.

For example:

1. There are 2 positions in a 2 fee tier UniswapV3 pool. Initially, the first position (first fee tier) represents 1000 USDT-1000 USDC, and the second position (second fee tier) represents 2000 USDT-2000 USDC => Assuming that there is no fee, the initial reserves are 3000 USDT-3000 USDC.
2. The attacker increases USDT in the first pool -> the first position will have 1500 USDT-499 USDC. The attacker increases USDC in the second pool -> the second position will have 1499 USDT-2500 USDC. => The new reserves will be 2999 USDT-2999 USDC (decreased).

[huuducsc]

The amounts of token0 and token1 in a position are calculated based on the current price and liquidity amount https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/LiquidityAmounts.sol#L120-L136. This mechanism is used in concentrated pools to prevent profiting from manipulating price then withdrawing position. If an attacker can increase the reserves without incurring any losses, as in the scenario described above, could anyone else do something similar and profit from their positions?

[fann95]

For example:

1. There are 2 positions in a 2 fee tier UniswapV3 pool. Initially, the first position (first fee tier) represents 1000 USDT-1000 USDC, and the second position (second fee tier) represents 2000 USDT-2000 USDC => Assuming that there is no fee, the initial reserves are 3000 USDT-3000 USDC.
2. The attacker increases USDT in the first pool -> the first position will have 1500 USDT-499 USDC. The attacker increases USDC in the second pool -> the second position will have 1499 USDT-2500 USDC. => The new reserves will be 2999 USDT-2999 USDC (decreased).

https://github.com/RealWagmi/concentrator/blob/9036fe43b0de2694a0c5e18cfe821d40ce17a9b8/contracts/Multipool.sol#L1013C14-L1013C34 We have protection for a similar scenario of price manipulation in pools..do you think it's not enough? It looks like this issue is invalid.

[0xffff11]

Fixed by adding the lpAmountMin parameter when depositing

[0x3f97]

Is this issuse finally confirmed to be valid?