No slippage Protecting while adding liquidity to the pool

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L198-L199

Vulnerability details

Impact

The presence of the front-running vulnerability in the claimFee() function can have significant repercussions for users. Exploiting this vulnerability enables malicious actors to prioritize their transactions, potentially leading to unfavorable prices for other users. Consequently, users who initially submitted their transactions may end up receiving worse prices than they anticipated. This can result in financial losses.

Proof of Concept

The claimFee() function in the TokenisableRange.sol code is responsible for claiming fee. However, it suffers from a front-running vulnerability due to the inadequate specification of minimum token amounts (amount0Min and amount1Min).

When the amount0Min and amount1Min parameters are both set to 0, the code allows any amount of token0 and token1 to be accepted during the fee claiming process. This lack of minimum token amount validation creates an opportunity for attackers to manipulate the transaction and impact the price received by the victim.

Malicious users can monitor pending transactions and quickly execute their own transaction with higher fees, effectively "sandwiching" the original transaction. By manipulating the price, the attacker can cause the original transaction to receive unfavorable rates, resulting in financial harm to the user.

In the below instances, a call to Uniswap V3 is made and amount0Min and amount1Min are each set to 0, which allows for 100% slippage tolerance. This means that the action could lead to the caller losing up to 100% of their tokens due to slippage.

INonfungiblePositionManager.IncreaseLiquidityParams({
          tokenId: tokenId,
          amount0Desired: fee0,
          amount1Desired: fee1,
          amount0Min: 0, //@audit
          amount1Min: 0,
          deadline: block.timestamp 
        })

Tools Used

Manual review

Recommended Mitigation Steps

Implement it like you have done in withdraw() function (take if as input from user)

function withdraw(uint256 lp, uint256 amount0Min, uint256 amount1Min) external nonReentrant returns (uint256 removed0, uint256 removed1) {
    claimFee();
    uint removedLiquidity = uint(liquidity) * lp / totalSupply();
    (removed0, removed1) = POS_MGR.decreaseLiquidity(
      INonfungiblePositionManager.DecreaseLiquidityParams({
        tokenId: tokenId,
        liquidity: uint128(removedLiquidity),
        amount0Min: amount0Min,
        amount1Min: amount1Min,
        deadline: block.timestamp //@audit
      })

Assessed type

Uniswap

[c4-pre-sort]

141345 marked the issue as duplicate of #78

[c4-pre-sort]

141345 marked the issue as duplicate of #260

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-c

[Nabeel-javaid]

Hey, can you please have another look at this function, its a valid medium and historically its judged as medium
here are a references

[lokithe5th]

The guard against slippage for the function referenced in your issue is placed here

[gzeon-c4]

Also see #260 (comment)