fix: slippage checks (#285)

* update amount checks

* take test after increase

* make natspec better, comments

---------

Co-authored-by: Alice Henshaw <[email protected]>

.forge-snapshots/PositionManager_burn_empty.snap

@@ -1 +1 @@
-47170
+47167

.forge-snapshots/PositionManager_burn_empty_native.snap

@@ -1 +1 @@
-46988
+46984

.forge-snapshots/PositionManager_burn_nonEmpty_native_withClose.snap

@@ -1 +1 @@
-123226
+123586

.forge-snapshots/PositionManager_burn_nonEmpty_native_withTakePair.snap

@@ -1 +1 @@
-122733
+123093

.forge-snapshots/PositionManager_burn_nonEmpty_withClose.snap

@@ -1 +1 @@
-130304
+130664

.forge-snapshots/PositionManager_burn_nonEmpty_withTakePair.snap

@@ -1 +1 @@
-129812
+130172

.forge-snapshots/PositionManager_collect_native.snap

@@ -1 +1 @@
-141698
+142147

.forge-snapshots/PositionManager_collect_sameRange.snap

@@ -1 +1 @@
-150546
+150995

.forge-snapshots/PositionManager_collect_withClose.snap

@@ -1 +1 @@
-150546
+150995

.forge-snapshots/PositionManager_collect_withTakePair.snap

@@ -1 +1 @@
-149918
+150367

.forge-snapshots/PositionManager_decreaseLiquidity_native.snap

@@ -1 +1 @@
-108833
+109192

.forge-snapshots/PositionManager_decreaseLiquidity_withClose.snap

@@ -1 +1 @@
-116089
+116538

.forge-snapshots/PositionManager_decreaseLiquidity_withTakePair.snap

@@ -1 +1 @@
-115461
+115910

.forge-snapshots/PositionManager_decrease_burnEmpty.snap

@@ -1 +1 @@
-134384
+134740

.forge-snapshots/PositionManager_decrease_burnEmpty_native.snap

@@ -1 +1 @@
-127123
+127479

.forge-snapshots/PositionManager_decrease_sameRange_allLiquidity.snap

@@ -1 +1 @@
-128805
+129254

.forge-snapshots/PositionManager_decrease_take_take.snap

@@ -1 +1 @@
-116622
+117071

.forge-snapshots/PositionManager_increaseLiquidity_erc20_withClose.snap

@@ -1 +1 @@
-154942
+155245

.forge-snapshots/PositionManager_increaseLiquidity_erc20_withSettlePair.snap

@@ -1 +1 @@
-153944
+154247

.forge-snapshots/PositionManager_increaseLiquidity_native.snap

@@ -1 +1 @@
-136742
+137045

.forge-snapshots/PositionManager_increase_autocompoundExactUnclaimedFees.snap

@@ -1 +1 @@
-132841
+133390

.forge-snapshots/PositionManager_increase_autocompoundExcessFeesCredit.snap

@@ -1 +1 @@
-173757
+174306

.forge-snapshots/PositionManager_increase_autocompound_clearExcess.snap

@@ -1 +1 @@
-143713
+144262

.forge-snapshots/PositionManager_mint_native.snap

@@ -1 +1 @@
-340639
+341062

.forge-snapshots/PositionManager_mint_nativeWithSweep_withClose.snap

@@ -1 +1 @@
-349131
+349554

.forge-snapshots/PositionManager_mint_nativeWithSweep_withSettlePair.snap

@@ -1 +1 @@
-348433
+348856

.forge-snapshots/PositionManager_mint_onSameTickLower.snap

@@ -1 +1 @@
-318621
+319044

.forge-snapshots/PositionManager_mint_onSameTickUpper.snap

@@ -1 +1 @@
-319263
+319686

.forge-snapshots/PositionManager_mint_sameRange.snap

@@ -1 +1 @@
-244845
+245268

.forge-snapshots/PositionManager_mint_settleWithBalance_sweep.snap

@@ -1 +1 @@
-374663
+375086

.forge-snapshots/PositionManager_mint_warmedPool_differentRange.snap

@@ -1 +1 @@
-324639
+325062

.forge-snapshots/PositionManager_mint_withClose.snap

@@ -1 +1 @@
-375939
+376362

.forge-snapshots/PositionManager_mint_withSettlePair.snap

@@ -1 +1 @@
-375079
+375502

.forge-snapshots/PositionManager_multicall_initialize_mint.snap

@@ -1 +1 @@
-420413
+420836

src/PositionManager.sol

@@ -217,8 +217,10 @@ contract PositionManager is
         bytes calldata hookData
     ) internal onlyIfApproved(msgSender(), tokenId) onlyValidConfig(tokenId, config) {
         // Note: The tokenId is used as the salt for this position, so every minted position has unique storage in the pool manager.
-        BalanceDelta liquidityDelta = _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
-        liquidityDelta.validateMaxInNegative(amount0Max, amount1Max);
+        (BalanceDelta liquidityDelta, BalanceDelta feesAccrued) =
+            _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
+        // Slippage checks should be done on the principal liquidityDelta which is the liquidityDelta - feesAccrued
+        (liquidityDelta - feesAccrued).validateMaxIn(amount0Max, amount1Max);
     }
 
     /// @dev Calling decrease with 0 liquidity will credit the caller with any underlying fees of the position
@@ -231,8 +233,10 @@ contract PositionManager is
         bytes calldata hookData
     ) internal onlyIfApproved(msgSender(), tokenId) onlyValidConfig(tokenId, config) {
         // Note: the tokenId is used as the salt.
-        BalanceDelta liquidityDelta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
-        liquidityDelta.validateMinOut(amount0Min, amount1Min);
+        (BalanceDelta liquidityDelta, BalanceDelta feesAccrued) =
+            _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
+        // Slippage checks should be done on the principal liquidityDelta which is the liquidityDelta - feesAccrued
+        (liquidityDelta - feesAccrued).validateMinOut(amount0Min, amount1Min);
     }
 
     function _mint(
@@ -252,8 +256,10 @@ contract PositionManager is
         _mint(owner, tokenId);
 
         // _beforeModify is not called here because the tokenId is newly minted
-        BalanceDelta liquidityDelta = _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
-        liquidityDelta.validateMaxIn(amount0Max, amount1Max);
+        (BalanceDelta liquidityDelta, BalanceDelta feesAccrued) =
+            _modifyLiquidity(config, liquidity.toInt256(), bytes32(tokenId), hookData);
+        // Slippage checks should be done on the principal liquidityDelta which is the liquidityDelta - feesAccrued
+        (liquidityDelta - feesAccrued).validateMaxIn(amount0Max, amount1Max);
         positionConfigs.setConfigId(tokenId, config);
 
         emit MintPosition(tokenId, config);
@@ -269,11 +275,12 @@ contract PositionManager is
     ) internal onlyIfApproved(msgSender(), tokenId) onlyValidConfig(tokenId, config) {
         uint256 liquidity = uint256(getPositionLiquidity(tokenId, config));
 
-        BalanceDelta liquidityDelta;
         // Can only call modify if there is non zero liquidity.
         if (liquidity > 0) {
-            liquidityDelta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
-            liquidityDelta.validateMinOut(amount0Min, amount1Min);
+            (BalanceDelta liquidityDelta, BalanceDelta feesAccrued) =
+                _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData);
+            // Slippage checks should be done on the principal liquidityDelta which is the liquidityDelta - feesAccrued
+            (liquidityDelta - feesAccrued).validateMinOut(amount0Min, amount1Min);
         }
 
         delete positionConfigs[tokenId];
@@ -332,8 +339,7 @@ contract PositionManager is
         int256 liquidityChange,
         bytes32 salt,
         bytes calldata hookData
-    ) internal returns (BalanceDelta liquidityDelta) {
-        BalanceDelta feesAccrued;
+    ) internal returns (BalanceDelta liquidityDelta, BalanceDelta feesAccrued) {
         (liquidityDelta, feesAccrued) = poolManager.modifyLiquidity(
             config.poolKey,
             IPoolManager.ModifyLiquidityParams({

src/libraries/SlippageCheck.sol

@@ -2,33 +2,42 @@
 pragma solidity ^0.8.0;
 
 import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
+import {SafeCastTemp} from "./SafeCast.sol";
 
 /// @title Slippage Check Library
 /// @notice a library for checking if a delta exceeds a maximum ceiling or fails to meet a minimum floor
 library SlippageCheckLibrary {
+    using SafeCastTemp for int128;
+
     error MaximumAmountExceeded();
     error MinimumAmountInsufficient();
 
     /// @notice Revert if one or both deltas does not meet a minimum output
-    /// @dev to be used when removing liquidity to guarantee a minimum output
+    /// @param delta The principal amount of tokens to be removed, does not include any fees accrued
+    /// @param amount0Min The minimum amount of token0 to receive
+    /// @param amount1Min The minimum amount of token1 to receive
+    /// @dev This should be called when removing liquidity (burn or decrease)
     function validateMinOut(BalanceDelta delta, uint128 amount0Min, uint128 amount1Min) internal pure {
-        if (uint128(delta.amount0()) < amount0Min || uint128(delta.amount1()) < amount1Min) {
+        // Called on burn or decrease, where we expect the returned delta to be positive.
+        // However, on pools where hooks can return deltas on modify liquidity, it is possible for a returned delta to be negative.
+        // Because we use SafeCast, this will revert in those cases when the delta is negative.
+        // This means this contract will NOT support pools where the hook returns a negative delta on burn/decrease.
+        if (delta.amount0().toUint128() < amount0Min || delta.amount1().toUint128() < amount1Min) {
             revert MinimumAmountInsufficient();
         }
     }
 
     /// @notice Revert if one or both deltas exceeds a maximum input
-    /// @dev to be used when minting liquidity to guarantee a maximum input
+    /// @param delta The principal amount of tokens to be added, does not include any fees accrued (which is possible on increase)
+    /// @param amount0Max The maximum amount of token0 to spend
+    /// @param amount1Max The maximum amount of token1 to spend
+    /// @dev This should be called when adding liquidity (mint or increase)
     function validateMaxIn(BalanceDelta delta, uint128 amount0Max, uint128 amount1Max) internal pure {
-        if (uint128(-delta.amount0()) > amount0Max || uint128(-delta.amount1()) > amount1Max) {
-            revert MaximumAmountExceeded();
-        }
-    }
-
-    /// @notice Revert if one or both deltas exceeds a maximum input
-    /// @dev When increasing liquidity, delta can be positive when excess fees need to be collected
-    /// in those cases, slippage checks are not required
-    function validateMaxInNegative(BalanceDelta delta, uint128 amount0Max, uint128 amount1Max) internal pure {
+        // Called on mint or increase, where we expect the returned delta to be negative.
+        // However, on pools where hooks can return deltas on modify liquidity, it is possible for a returned delta to be positive (even after discounting fees accrued).
+        // Thus, we only cast the delta if it is guaranteed to be negative.
+        // And we do NOT revert in the positive delta case. Since a positive delta means the hook is crediting tokens to the user for minting/increasing liquidity, we do not check slippage.
+        // This means this contract will NOT support _positive_ slippage checks (minAmountOut checks) on pools where the hook returns a positive delta on mint/increase.
         if (
             delta.amount0() < 0 && amount0Max < uint128(-delta.amount0())
                 || delta.amount1() < 0 && amount1Max < uint128(-delta.amount1())

test/position-managers/IncreaseLiquidity.t.sol

@@ -75,6 +75,75 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
         config = PositionConfig({poolKey: key, tickLower: -300, tickUpper: 300});
     }
 
+    /// @notice Increase liquidity by less than the amount of liquidity the position has earned, requiring a take
+    function test_increaseLiquidity_withCollection_takePair() public {
+        // Alice and Bob provide liquidity on the range
+        // Alice uses her exact fees to increase liquidity (compounding)
+
+        uint256 liquidityAlice = 3_000e18;
+        uint256 liquidityBob = 1_000e18;
+
+        // alice provides liquidity
+        vm.startPrank(alice);
+        uint256 tokenIdAlice = lpm.nextTokenId();
+        mint(config, liquidityAlice, alice, ZERO_BYTES);
+        vm.stopPrank();
+
+        // bob provides liquidity
+        vm.startPrank(bob);
+        mint(config, liquidityBob, bob, ZERO_BYTES);
+        vm.stopPrank();
+
+        // donate to create fees
+        uint256 amountDonate = 0.1e18;
+        donateRouter.donate(key, amountDonate, amountDonate, ZERO_BYTES);
+
+        // alice uses her half her fees to increase liquidity
+        // Slight error in this calculation vs. actual fees.. TODO: Fix this.
+        BalanceDelta feesOwedAlice = IPositionManager(lpm).getFeesOwed(manager, config, tokenIdAlice);
+        // Note: You can alternatively calculate Alice's fees owed from the swap amount, fee on the pool, and total liquidity in that range.
+        // swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityAlice, liquidityAlice + liquidityBob);
+
+        (uint160 sqrtPriceX96,,,) = StateLibrary.getSlot0(manager, config.poolKey.toId());
+        uint256 liquidityDelta = LiquidityAmounts.getLiquidityForAmounts(
+            sqrtPriceX96,
+            TickMath.getSqrtPriceAtTick(config.tickLower),
+            TickMath.getSqrtPriceAtTick(config.tickUpper),
+            uint256(int256(feesOwedAlice.amount0() / 2)),
+            uint256(int256(feesOwedAlice.amount1() / 2))
+        );
+
+        uint256 balance0BeforeAlice = currency0.balanceOf(alice);
+        uint256 balance1BeforeAlice = currency1.balanceOf(alice);
+
+        //  Set the slippage amounts to be exactly half the fees that alice is reinvesting.
+
+        Plan memory planner = Planner.init();
+        planner.add(
+            Actions.INCREASE_LIQUIDITY,
+            abi.encode(
+                tokenIdAlice,
+                config,
+                liquidityDelta,
+                feesOwedAlice.amount0() / 2,
+                feesOwedAlice.amount1() / 2,
+                ZERO_BYTES
+            )
+        );
+        bytes memory calls = planner.finalizeModifyLiquidityWithTakePair(config.poolKey, address(alice));
+        vm.startPrank(alice);
+        lpm.modifyLiquidities(calls, _deadline);
+        vm.stopPrank();
+
+        // alices current balance is the balanceBefore plus half of her fees owed
+        assertApproxEqAbs(
+            currency0.balanceOf(alice), balance0BeforeAlice + uint256(int256(feesOwedAlice.amount0() / 2)), tolerance
+        );
+        assertApproxEqAbs(
+            currency1.balanceOf(alice), balance1BeforeAlice + uint256(int256(feesOwedAlice.amount1() / 2)), tolerance
+        );
+    }
+
     /// @notice Increase liquidity with exact fees, taking dust
     function test_increaseLiquidity_withExactFees_take() public {
         // Alice and Bob provide liquidity on the range
@@ -119,7 +188,10 @@ contract IncreaseLiquidityTest is Test, PosmTestSetup, Fuzzers {
 
         Plan memory planner = Planner.init();
         planner.add(
-            Actions.INCREASE_LIQUIDITY, abi.encode(tokenIdAlice, config, liquidityDelta, 0 wei, 0 wei, ZERO_BYTES)
+            Actions.INCREASE_LIQUIDITY,
+            abi.encode(
+                tokenIdAlice, config, liquidityDelta, feesOwedAlice.amount0(), feesOwedAlice.amount1(), ZERO_BYTES
+            )
         );
         bytes memory calls = planner.finalizeModifyLiquidityWithClose(config.poolKey);
         vm.startPrank(alice);