feat: audit feedback

.forge-snapshots/CLPositionManager_collect_native.snap

@@ -1 +1 @@
-205654
+205632

.forge-snapshots/CLPositionManager_collect_sameRange.snap

@@ -1 +1 @@
-214233
+214211

.forge-snapshots/CLPositionManager_collect_withClose.snap

@@ -1 +1 @@
-214233
+214211

.forge-snapshots/CLPositionManager_collect_withTakePair.snap

@@ -1 +1 @@
-213557
+213535

.forge-snapshots/CLPositionManager_decreaseLiquidity_native.snap

@@ -1 +1 @@
-170947
+170925

.forge-snapshots/CLPositionManager_decreaseLiquidity_withClose.snap

@@ -1 +1 @@
-179526
+179504

.forge-snapshots/CLPositionManager_decreaseLiquidity_withTakePair.snap

@@ -1 +1 @@
-178850
+178828

.forge-snapshots/CLPositionManager_decrease_burnEmpty.snap

@@ -1 +1 @@
-186820
+186798

.forge-snapshots/CLPositionManager_decrease_burnEmpty_native.snap

@@ -1 +1 @@
-178241
+178219

.forge-snapshots/CLPositionManager_decrease_sameRange_allLiquidity.snap

@@ -1 +1 @@
-192151
+192129

.forge-snapshots/CLPositionManager_decrease_take_take.snap

@@ -1 +1 @@
-180193
+180171

.forge-snapshots/CLPositionManager_increaseLiquidity_erc20_withClose.snap

@@ -1 +1 @@
-220862
+220840

.forge-snapshots/CLPositionManager_increaseLiquidity_erc20_withSettlePair.snap

@@ -1 +1 @@
-219779
+219757

.forge-snapshots/CLPositionManager_increaseLiquidity_native.snap

@@ -1 +1 @@
-204210
+204188

.forge-snapshots/CLPositionManager_increase_autocompoundExactUnclaimedFees.snap

@@ -1 +1 @@
-165716
+165694

.forge-snapshots/CLPositionManager_increase_autocompoundExcessFeesCredit.snap

@@ -1 +1 @@
-239223
+239201

.forge-snapshots/CLPositionManager_increase_autocompound_clearExcess.snap

@@ -1 +1 @@
-210093
+210071

src/libraries/Actions.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 
 /// @notice Library to define different pool actions.
 /// @dev These are suggested common commands, however additional commands should be defined as required
+/// Some of these actions are not supported in the Router contracts or Position Manager contracts, but are left as they may be helpful commands for other peripheral contracts.
 library Actions {
     // cl-pool actions
     // liquidity actions
@@ -19,7 +20,9 @@ library Actions {
     uint256 constant CL_SWAP_EXACT_IN = 0x07;
     uint256 constant CL_SWAP_EXACT_OUT_SINGLE = 0x08;
     uint256 constant CL_SWAP_EXACT_OUT = 0x09;
+
     // donate
+    /// @dev this is not supported in the position manager or router
     uint256 constant CL_DONATE = 0x0a;
 
     // closing deltas on the pool manager
@@ -40,6 +43,7 @@ library Actions {
     uint256 constant UNWRAP = 0x16;
 
     // minting/burning 6909s to close deltas
+    /// @dev this is not supported in the position manager or router
     uint256 constant MINT_6909 = 0x17;
     uint256 constant BURN_6909 = 0x18;
 

src/pool-cl/CLPositionManager.sol

@@ -375,8 +375,8 @@ contract CLPositionManager is
 
         // Can only call modify if there is non zero liquidity.
         BalanceDelta feesAccrued;
-        BalanceDelta liquidityDelta;
         if (liquidity > 0) {
+            BalanceDelta liquidityDelta;
             (liquidityDelta, feesAccrued) = clPoolManager.modifyLiquidity(
                 poolKey,
                 ICLPoolManager.ModifyLiquidityParams({

src/pool-cl/base/CLNotifier.sol

@@ -90,33 +90,28 @@ abstract contract CLNotifier is ICLNotifier {
         uint256 liquidity,
         BalanceDelta feesAccrued
     ) internal {
-        ICLSubscriber _subscriber = subscriber[tokenId];
+        address _subscriber = address(subscriber[tokenId]);
 
         // remove the subscriber
         delete subscriber[tokenId];
 
-        bool success = _call(
-            address(_subscriber),
-            abi.encodeCall(ICLSubscriber.notifyBurn, (tokenId, owner, info, liquidity, feesAccrued))
-        );
+        bool success =
+            _call(_subscriber, abi.encodeCall(ICLSubscriber.notifyBurn, (tokenId, owner, info, liquidity, feesAccrued)));
 
         if (!success) {
-            address(_subscriber).bubbleUpAndRevertWith(
-                ICLSubscriber.notifyBurn.selector, BurnNotificationReverted.selector
-            );
+            _subscriber.bubbleUpAndRevertWith(ICLSubscriber.notifyBurn.selector, BurnNotificationReverted.selector);
         }
     }
 
     function _notifyModifyLiquidity(uint256 tokenId, int256 liquidityChange, BalanceDelta feesAccrued) internal {
-        ICLSubscriber _subscriber = subscriber[tokenId];
+        address _subscriber = address(subscriber[tokenId]);
 
         bool success = _call(
-            address(_subscriber),
-            abi.encodeCall(ICLSubscriber.notifyModifyLiquidity, (tokenId, liquidityChange, feesAccrued))
+            _subscriber, abi.encodeCall(ICLSubscriber.notifyModifyLiquidity, (tokenId, liquidityChange, feesAccrued))
         );
 
         if (!success) {
-            address(_subscriber).bubbleUpAndRevertWith(
+            _subscriber.bubbleUpAndRevertWith(
                 ICLSubscriber.notifyModifyLiquidity.selector, ModifyLiquidityNotificationReverted.selector
             );
         }

src/pool-cl/interfaces/ICLNotifier.sol

@@ -18,8 +18,6 @@ interface ICLNotifier {
     error ModifyLiquidityNotificationReverted(address subscriber, bytes reason);
     /// @notice Wraps the revert message of the subscriber contract on a reverting burn notification
     error BurnNotificationReverted(address subscriber, bytes reason);
-    /// @notice Wraps the revert message of the subscriber contract on a reverting transfer notification
-    error TransferNotificationReverted(address subscriber, bytes reason);
     /// @notice Thrown when a tokenId already has a subscriber
     error AlreadySubscribed(uint256 tokenId, address subscriber);
 

src/pool-cl/interfaces/ICLSubscriber.sol

@@ -6,14 +6,17 @@ import {CLPositionInfo} from "../libraries/CLPositionInfoLibrary.sol";
 
 /// @notice Interface that a Subscriber contract should implement to receive updates from the v4 cl pool position manager
 interface ICLSubscriber {
+    /// @notice Called when a position subscribes to this subscriber contract
     /// @param tokenId the token ID of the position
     /// @param data additional data passed in by the caller
     function notifySubscribe(uint256 tokenId, bytes memory data) external;
+
     /// @notice Called when a position unsubscribes from the subscriber
     /// @dev This call's gas is capped at `unsubscribeGasLimit` (set at deployment)
     /// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()
     /// @param tokenId the token ID of the position
     function notifyUnsubscribe(uint256 tokenId) external;
+
     /// @notice Called when a position is burned
     /// @param tokenId the token ID of the position
     /// @param owner the current owner of the tokenId
@@ -27,8 +30,13 @@ interface ICLSubscriber {
         uint256 liquidity,
         BalanceDelta feesAccrued
     ) external;
+
+    /// @notice Called when a position modifies its liquidity or collects fees
     /// @param tokenId the token ID of the position
     /// @param liquidityChange the change in liquidity on the underlying position
     /// @param feesAccrued the fees to be collected from the position as a result of the modifyLiquidity call
+    /// @dev feesAccrued can be artificially inflated by a malicious user
+    /// Pools with a single liquidity position can inflate feeGrowthGlobal (and consequently feesAccrued) by donating to themselves;
+    /// automatically donating and collecting fees within the same unlockCallback may further inflate feeGrowthGlobal/feesAccrued
     function notifyModifyLiquidity(uint256 tokenId, int256 liquidityChange, BalanceDelta feesAccrued) external;
 }

src/pool-cl/libraries/CLCalldataDecoder.sol

@@ -91,16 +91,14 @@ library CLCalldataDecoder {
         pure
         returns (uint256 tokenId, uint256 liquidity, uint128 amount0, uint128 amount1, bytes calldata hookData)
     {
+        // length validation is already handled in `params.toBytes`
         assembly ("memory-safe") {
-            if lt(params.length, 0x80) {
-                mstore(0, SLICE_ERROR_SELECTOR)
-                revert(0x1c, 4)
-            }
             tokenId := calldataload(params.offset)
             liquidity := calldataload(add(params.offset, 0x20))
             amount0 := calldataload(add(params.offset, 0x40))
             amount1 := calldataload(add(params.offset, 0x60))
         }
+
         hookData = params.toBytes(4);
     }
 
@@ -110,6 +108,7 @@ library CLCalldataDecoder {
         pure
         returns (uint256 tokenId, uint128 amount0Max, uint128 amount1Max, bytes calldata hookData)
     {
+        // length validation is already handled in `params.toBytes`
         assembly ("memory-safe") {
             tokenId := calldataload(params.offset)
             amount0Max := calldataload(add(params.offset, 0x20))
@@ -134,6 +133,7 @@ library CLCalldataDecoder {
             bytes calldata hookData
         )
     {
+        // length validation is already handled in `params.toBytes`
         assembly ("memory-safe") {
             poolKey := params.offset
             tickLower := calldataload(add(params.offset, 0xc0))
@@ -160,6 +160,7 @@ library CLCalldataDecoder {
             bytes calldata hookData
         )
     {
+        // length validation is already handled in `params.toBytes`
         assembly ("memory-safe") {
             poolKey := params.offset
             tickLower := calldataload(add(params.offset, 0xc0))
@@ -178,6 +179,7 @@ library CLCalldataDecoder {
         pure
         returns (uint256 tokenId, uint128 amount0Min, uint128 amount1Min, bytes calldata hookData)
     {
+        // length validation is already handled in `params.toBytes`
         assembly ("memory-safe") {
             tokenId := calldataload(params.offset)
             amount0Min := calldataload(add(params.offset, 0x20))

test/pool-cl/shared/FeeMath.sol

@@ -33,11 +33,11 @@ library FeeMath {
         ) = posm.positions(tokenId);
         PoolId poolId = poolKey.toId();
 
-        (uint256 feeGrowthInside0X218, uint256 feeGrowthInside1X128) =
+        (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) =
             _getFeeGrowthInside(manager, poolId, tickLower, tickUpper);
 
         feesOwed = getFeesOwed(
-            feeGrowthInside0X218, feeGrowthInside1X128, feeGrowthInside0LastX128, feeGrowthInside1LastX128, liquidity
+            feeGrowthInside0X128, feeGrowthInside1X128, feeGrowthInside0LastX128, feeGrowthInside1LastX128, liquidity
         );
     }