Users are forced to lose funds with the fixed slippage values on the project.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L198-L199
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L258-L259
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L299-L300
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L477

Vulnerability details

Impact

Users lose funds to sandwich attack due to hardcoded high slippage.

Proof of Concept

The various slippage amount are used on the project which will force users to lose funds to sandwich attack. Slipages of 5% and even 100% slippage are forced on users causing users to lose all funds.

1. 5% slippage means when a user adds liquidity with 100ETH, the user will lose 5ETH to frontrunning.

(uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
      INonfungiblePositionManager.IncreaseLiquidityParams({
        tokenId: tokenId,
        amount0Desired: n0,
        amount1Desired: n1,
        amount0Min: n0 * 95 / 100, //@audit allow slippage as input
        amount1Min: n1 * 95 / 100,
        deadline: block.timestamp //@audit this can be gamed.
      })
    );

2. Zero amount0Min and amount1Min mean 100% slippage which could allow buts steal all the funds.

TOKEN1.token.safeIncreaseAllowance(address(POS_MGR), fee1);
      (uint128 newLiquidity, uint256 added0, uint256 added1) = POS_MGR.increaseLiquidity(
        INonfungiblePositionManager.IncreaseLiquidityParams({
          tokenId: tokenId,
          amount0Desired: fee0,
          amount1Desired: fee1,
          amount0Min: 0, //@audit total loss of funds
          amount1Min: 0, //@audit 100% slippage
          deadline: block.timestamp
        })
      );

3. This 1% slippage forced which can be high or low in some cases. Allow user pass slippage as input

uint[] memory amounts = ammRouter.swapExactTokensForTokens(
        amount, 
        getTargetAmountFromOracle(oracle, path[0], amount, path[1]) * 99 / 100, // allow 1% slippage //@audit allow user pass slippage.
        path, 
        address(this), 
        block.timestamp//@audit allow deadline to be passed
      );

Tools Used

Manual Review

Recommended Mitigation Steps

Allows users to pass the minimum amount(AmountMin) they are willing to take instead of hardcoding the slippage.

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 marked the issue as unsatisfactory:
Insufficient proof

[c4-judge]

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