lack of slippage protection

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L135
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L331
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L119
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L131

Vulnerability details

Impact

There are some functions contains no slippage , which makes it vulnerable to sandwich attacks, MEV exploits and may lead to significant loss of yield.

Proof of Concept

According to TokenisableRange.withdraw, there are two parameters uint256 amount0Min and uint256 amount1Min need to be set to prevent MEV.

  /// @notice Withdraw assets from a range
  /// @param lp Amount of tokens withdrawn
  /// @param amount0Min Minimum amount of quote token withdrawn
  /// @param amount1Min Minimum amount of base token withdrawn
  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,    <------------------------------ Here
        amount1Min: amount1Min,    <------------------------------ Here
        deadline: block.timestamp
      })
    );
    liquidity = uint128(uint256(liquidity) - removedLiquidity); 
    
    POS_MGR.collect( 
      INonfungiblePositionManager.CollectParams({
        tokenId: tokenId,
        recipient: msg.sender,
        amount0Max: uint128(removed0),
        amount1Max: uint128(removed1)
      })
    );
    // Handle uncompounded fees
    if (fee0 > 0) {
      TOKEN0.token.safeTransfer( msg.sender, fee0 * lp / totalSupply());
      removed0 += fee0 * lp / totalSupply();
      fee0 -= fee0 * lp / totalSupply();
    } 
    if (fee1 > 0) {
      TOKEN1.token.safeTransfer(  msg.sender, fee1 * lp / totalSupply());
      removed1 += fee1 * lp / totalSupply();
      fee1 -= fee1 * lp / totalSupply();
    }
    _burn(msg.sender, lp);
    emit Withdraw(msg.sender, lp);
  }

But there are some code which calls TokenisableRange.withdraw but set uint256 amount0Min and uint256 amount1Min to be 0.
Such as:
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/OptionsPositionManager.sol#L135
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L331
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L119
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/RangeManager.sol#L131

Tools Used

vim

Recommended Mitigation Steps

Assessed type

MEV

[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)