[Part 2 of 2] Internal feedback from universal router]: protect against invalid Positionmanage action

.forge-snapshots/BinNativePancakeSwapV4Test#test_v4BinSwap_ExactInSingle_NativeIn.snap

@@ -1 +1 @@
-146304
+146587

.forge-snapshots/BinNativePancakeSwapV4Test#test_v4BinSwap_ExactInSingle_NativeOut_RouterRecipient.snap

@@ -1 +1 @@
-123362
+123645

.forge-snapshots/BinPancakeSwapV4Test#test_v4BinSwap_ExactInSingle.snap

@@ -1 +1 @@
-147266
+147552

.forge-snapshots/BinPancakeSwapV4Test#test_v4BinSwap_ExactIn_MultiHop.snap

@@ -1 +1 @@
-178676
+179182

.forge-snapshots/BinPancakeSwapV4Test#test_v4BinSwap_ExactIn_SingleHop.snap

@@ -1 +1 @@
-149062
+149348

.forge-snapshots/BinPancakeSwapV4Test#test_v4BinSwap_ExactOut_MultiHop.snap

@@ -1 +1 @@
-182517
+183027

.forge-snapshots/BinPancakeSwapV4Test#test_v4BinSwap_ExactOut_SingleHop.snap

@@ -1 +1 @@
-153435
+153723

.forge-snapshots/BinPancakeSwapV4Test#test_v4ClSwap_ExactOutSingle.snap

@@ -1 +1 @@
-151645
+151929

.forge-snapshots/CLNativePancakeSwapV4Test#test_v4ClSwap_ExactInSingle_NativeIn.snap

@@ -1 +1 @@
-172308
+172365

.forge-snapshots/CLNativePancakeSwapV4Test#test_v4ClSwap_ExactInSingle_NativeOut.snap

@@ -1 +1 @@
-174558
+174621

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactInSingle.snap

@@ -1 +1 @@
-181657
+181723

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactIn_MultiHop.snap

@@ -1 +1 @@
-246971
+247037

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactIn_SingleHop.snap

@@ -1 +1 @@
-183197
+183263

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactOutSingle.snap

@@ -1 +1 @@
-185996
+186062

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactOut_MultiHop.snap

@@ -1 +1 @@
-250730
+250804

.forge-snapshots/CLPancakeSwapV4Test#test_v4ClSwap_ExactOut_SingleHop.snap

@@ -1 +1 @@
-187529
+187599

.forge-snapshots/PancakeSwapV2Test#test_v2Swap_exactInput0For1.snap

@@ -1 +1 @@
-100125
+100141

.forge-snapshots/PancakeSwapV2Test#test_v2Swap_exactOutput0For1.snap

@@ -1 +1 @@
-100754
+100768

.forge-snapshots/PancakeSwapV3Test#test_v3Swap_exactOutput0For1.snap

@@ -1 +1 @@
-152659
+150557

.forge-snapshots/StableSwapTest#test_stableSwap_ExactInput0For1.snap

@@ -1 +1 @@
-193198
+193201

.forge-snapshots/StableSwapTest#test_stableSwap_ExactInput1For0.snap

@@ -1 +1 @@
-192654
+192657

.forge-snapshots/UniversalRouterBytecodeSize.snap

@@ -1 +1 @@
-24160
+24465

.forge-snapshots/UniversalRouterTest#test_sweep_token.snap

@@ -1 +1 @@
-55441
+55447

.forge-snapshots/V3ToV4MigrationNativeTest#test_v4CLPositionmanager_Mint_Native.snap

@@ -1 +1 @@
-559373
+564846

.forge-snapshots/V3ToV4MigrationTest#test_v3PositionManager_burn.snap

@@ -1 +1 @@
-291590
+291594

.forge-snapshots/V3ToV4MigrationTest#test_v4BinPositionmanager_BinAddLiquidity.snap

@@ -1 +1 @@
-594191
+594429

.forge-snapshots/V3ToV4MigrationTest#test_v4BinPositionmanager_BinAddLiquidity_Native.snap

@@ -1 +1 @@
-570028
+570263

.forge-snapshots/V3ToV4MigrationTest#test_v4CLPositionmanager_Mint.snap

@@ -1 +1 @@
-583544
+589080

foundry.toml

@@ -3,7 +3,7 @@ src = "src"
 out = 'foundry-out'
 libs = ["lib"]
 via_ir = true
-optimizer_runs = 30_000
+optimizer_runs = 25_000
 ffi = true
 fs_permissions = [
     { access = "read-write", path = ".forge-snapshots/" },

src/base/Dispatcher.sol

@@ -35,6 +35,7 @@ abstract contract Dispatcher is
     error InvalidCommandType(uint256 commandType);
     error BalanceTooLow();
     error InvalidAction(bytes4 action);
+    error InvalidPositionManagerAction(uint256 action);
     error NotAuthorizedForToken(uint256 tokenId);
 
     /// @notice Executes encoded commands along with provided inputs.
@@ -305,8 +306,13 @@ abstract contract Dispatcher is
                     (success, output) = address(V3_POSITION_MANAGER).call(inputs);
                     return (success, output);
                 } else if (command == Commands.V4_CL_POSITION_CALL) {
-                    // should only call modifyLiquidities() with Actions.CL_MINT_POSITION
-                    // do not permit or approve this contract over a v4 position or someone could use this command to decrease, burn, or transfer your position
+                    // Only modifyLiquidities() with Actions.CL_MINT_POSITION and SETTLEMENT related option are allowed
+                    // CL_INCREASE_LIQUIDITY, CL_DECREASE_LIQUIDITY and CL_BURN_POSITION are blocked
+                    (bool isInvalid, uint256 action) = containInValidV4AClActions(inputs);
+                    if (isInvalid) {
+                        revert InvalidPositionManagerAction(action);
+                    }
+
                     (success, output) = address(V4_CL_POSITION_MANAGER).call{value: address(this).balance}(inputs);
                     return (success, output);
                 } else if (command == Commands.V4_BIN_POSITION_CALL) {

src/libraries/Constants.sol

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity ^0.8.0;
 
-import {IWETH9} from "../interfaces/IWETH9.sol";
-
 /// @title Constant state
 /// @notice Constant state used by the Universal Router
 library Constants {

src/libraries/MaxInputAmount.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity ^0.8.24;
+
+/// @notice A library used to store the maximum desired amount of input tokens for exact output swaps; used for checking slippage
+library MaxInputAmount {
+    // The slot holding the the maximum desired amount of input tokens, transiently. bytes32(uint256(keccak256("MaxAmountIn")) - 1)
+    bytes32 constant MAX_AMOUNT_IN_SLOT = 0xaf28d9864a81dfdf71cab65f4e5d79a0cf9b083905fb8971425e6cb581b3f692;
+
+    function set(uint256 maxAmountIn) internal {
+        assembly ("memory-safe") {
+            tstore(MAX_AMOUNT_IN_SLOT, maxAmountIn)
+        }
+    }
+
+    function get() internal view returns (uint256 maxAmountIn) {
+        assembly ("memory-safe") {
+            maxAmountIn := tload(MAX_AMOUNT_IN_SLOT)
+        }
+    }
+}

src/modules/V3ToV4Migrator.sol

@@ -4,10 +4,16 @@ pragma solidity ^0.8.0;
 import {RouterImmutables} from "../base/RouterImmutables.sol";
 import {IV3NonfungiblePositionManager} from
     "pancake-v4-periphery/src/interfaces/external/IV3NonfungiblePositionManager.sol";
+import {CalldataDecoder} from "pancake-v4-periphery/src/libraries/CalldataDecoder.sol";
+import {Actions} from "pancake-v4-periphery/src/libraries/Actions.sol";
+import {Multicall} from "@openzeppelin/contracts/utils/Multicall.sol";
+import {IPositionManager} from "pancake-v4-periphery/src/interfaces/IPositionManager.sol";
 
 /// @title V3 to V4 Migrator
 /// @notice A contract that migrates liquidity from PancakeSwap V3 to V4
 abstract contract V3ToV4Migrator is RouterImmutables {
+    using CalldataDecoder for bytes;
+
     /// @dev validate if an action is decreaseLiquidity, collect, or burn
     function isValidAction(bytes4 selector) internal pure returns (bool) {
         return selector == IV3NonfungiblePositionManager.decreaseLiquidity.selector
@@ -21,4 +27,33 @@ abstract contract V3ToV4Migrator is RouterImmutables {
         return caller == owner || V3_POSITION_MANAGER.getApproved(tokenId) == caller
             || V3_POSITION_MANAGER.isApprovedForAll(owner, caller);
     }
+
+    function containInValidV4AClActions(bytes calldata inputs) internal pure returns (bool isInvalid, uint256 action) {
+        // Decode the data of modifyLiquidities(bytes calldata payload, uint256 deadline)
+        bytes4 selector = bytes4(inputs[:4]); // todo:
+
+        if (selector == IPositionManager.modifyLiquidities.selector) {
+            bytes calldata data = inputs[4:];
+
+            // decode payload to get bytes calldata actions, bytes[] calldata params
+            (bytes memory payload,) = abi.decode(data, (bytes, uint256));
+            (bytes memory actions,) = abi.decode(payload, (bytes, bytes[]));
+
+            for (uint256 actionIndex = 0; actionIndex < actions.length; actionIndex++) {
+                action = uint8(actions[actionIndex]);
+                if (
+                    action == Actions.CL_INCREASE_LIQUIDITY || action == Actions.CL_DECREASE_LIQUIDITY
+                        || action == Actions.CL_BURN_POSITION
+                ) {
+                    return (true, action);
+                }
+            }
+        } else if (selector == IPositionManager.modifyLiquiditiesWithoutLock.selector) {
+            // todo:
+        } else if (selector == Multicall.multicall.selector) {
+            // todo:
+        }
+
+        // todo: revert InvalidSelector
+    }
 }

src/modules/pancakeswap/StableSwapRouter.sol

@@ -9,11 +9,11 @@ import {Constants} from "../../libraries/Constants.sol";
 import {UniversalRouterHelper} from "../../libraries/UniversalRouterHelper.sol";
 import {ERC20} from "solmate/src/tokens/ERC20.sol";
 import {SafeTransferLib} from "solmate/src/utils/SafeTransferLib.sol";
-import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {Ownable, Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol";
 import {IStableSwap} from "../../interfaces/IStableSwap.sol";
 
 /// @title Router for PancakeSwap Stable Trades
-abstract contract StableSwapRouter is RouterImmutables, Permit2Payments, Ownable {
+abstract contract StableSwapRouter is RouterImmutables, Permit2Payments, Ownable2Step {
     using SafeTransferLib for ERC20;
     using UniversalRouterHelper for address;
 

src/modules/pancakeswap/v3/V3SwapRouter.sol

@@ -10,6 +10,7 @@ import {Constants} from "../../../libraries/Constants.sol";
 import {UniversalRouterHelper} from "../../../libraries/UniversalRouterHelper.sol";
 import {RouterImmutables} from "../../../base/RouterImmutables.sol";
 import {Permit2Payments} from "../../Permit2Payments.sol";
+import {MaxInputAmount} from "../../../libraries/MaxInputAmount.sol";
 import {ERC20} from "solmate/src/tokens/ERC20.sol";
 import {CalldataDecoder} from "pancake-v4-periphery/src/libraries/CalldataDecoder.sol";
 
@@ -26,13 +27,6 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IPancakeV3S
     error V3InvalidAmountOut();
     error V3InvalidCaller();
 
-    /// @dev Used as the placeholder value for maxAmountIn, because the computed amount in for an exact output swap
-    /// can never actually be this value
-    uint256 private constant DEFAULT_MAX_AMOUNT_IN = type(uint256).max;
-
-    /// @dev Transient storage variable used for checking slippage
-    uint256 private maxAmountInCached = DEFAULT_MAX_AMOUNT_IN;
-
     /// @dev The minimum value that can be returned from #getSqrtRatioAtTick. Equivalent to getSqrtRatioAtTick(MIN_TICK)
     uint160 internal constant MIN_SQRT_RATIO = 4295128739;
 
@@ -66,7 +60,7 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IPancakeV3S
                 path = path.skipToken();
                 _swap(-amountToPay.toInt256(), msg.sender, path, payer, false);
             } else {
-                if (amountToPay > maxAmountInCached) revert V3TooMuchRequested();
+                if (amountToPay > MaxInputAmount.get()) revert V3TooMuchRequested();
                 // note that because exact output swaps are executed in reverse order, tokenOut is actually tokenIn
                 payOrPermit2Transfer(tokenOut, payer, msg.sender, amountToPay);
             }
@@ -133,15 +127,15 @@ abstract contract V3SwapRouter is RouterImmutables, Permit2Payments, IPancakeV3S
         bytes calldata path,
         address payer
     ) internal {
-        maxAmountInCached = amountInMaximum;
+        MaxInputAmount.set(amountInMaximum);
         (int256 amount0Delta, int256 amount1Delta, bool zeroForOne) =
             _swap(-amountOut.toInt256(), recipient, path, payer, false);
 
         uint256 amountOutReceived = zeroForOne ? uint256(-amount1Delta) : uint256(-amount0Delta);
 
         if (amountOutReceived != amountOut) revert V3InvalidAmountOut();
 
-        maxAmountInCached = DEFAULT_MAX_AMOUNT_IN;
+        MaxInputAmount.set(0);
     }
 
     /// @dev Performs a single swap for both exactIn and exactOut

test/UniversalRouter.t.sol

@@ -57,6 +57,7 @@ contract UniversalRouterTest is Test, GasSnapshot, Permit2SignatureHelpers, Depl
             v4BinPositionManager: address(0)
         });
         router = new UniversalRouter(params);
+        assertEq(router.owner(), address(this));
 
         router = new UniversalRouter(params);
         erc20 = new MockERC20();
@@ -81,6 +82,20 @@ contract UniversalRouterTest is Test, GasSnapshot, Permit2SignatureHelpers, Depl
         emit log_uint(bytecodeSize);
     }
 
+    function test_Owner_TransferOwnership() public {
+        address alice = makeAddr("alice");
+        assertEq(router.owner(), address(this));
+
+        router.transferOwnership(alice);
+        assertEq(router.owner(), address(this));
+        assertEq(router.pendingOwner(), alice);
+
+        vm.prank(alice);
+        router.acceptOwnership();
+        assertEq(router.owner(), alice);
+        assertEq(router.pendingOwner(), address(0));
+    }
+
     function test_sweep_token() public {
         bytes memory commands = abi.encodePacked(bytes1(uint8(Commands.SWEEP)));
         bytes[] memory inputs = new bytes[](1);

test/V3ToV4Migration.t.sol

@@ -316,6 +316,30 @@ contract V3ToV4MigrationTest is BasePancakeSwapV4, OldVersionHelper, BinLiquidit
         assertEq(clPositionManager.ownerOf(1), alice);
     }
 
+    function test_v4CLPositionmanager_InvalidActions() public {
+        uint256[] memory blackListedActions = new uint256[](3);
+        blackListedActions[0] = Actions.CL_INCREASE_LIQUIDITY;
+        blackListedActions[1] = Actions.CL_DECREASE_LIQUIDITY;
+        blackListedActions[2] = Actions.CL_BURN_POSITION;
+
+        for (uint256 i = 0; i < blackListedActions.length; i++) {
+            Plan memory planner = Planner.init();
+            planner.add(Actions.CL_INCREASE_LIQUIDITY, abi.encode(10));
+
+            bytes memory commands = abi.encodePacked(bytes1(uint8(Commands.V4_CL_POSITION_CALL)));
+            bytes[] memory inputs = new bytes[](1);
+            inputs[0] = abi.encodePacked(
+                IPositionManager.modifyLiquidities.selector, abi.encode(planner.encode(), block.timestamp)
+            );
+
+            vm.expectRevert(
+                abi.encodeWithSelector(Dispatcher.InvalidPositionManagerAction.selector, Actions.CL_INCREASE_LIQUIDITY)
+            );
+            vm.prank(alice);
+            router.execute(commands, inputs);
+        }
+    }
+
     /// @dev Assume token0/token1 is aready in universal router from earlier steps on v3
     ///      then add liquidity to v4 cl and sweep remaining token
     function test_v4BinPositionmanager_BinAddLiquidity() public {