check pool manager lock on transfer, subscribe, and unsubscribe

.forge-snapshots/PositionManager_subscribe.snap

@@ -1 +1 @@
-84348
+88168

.forge-snapshots/PositionManager_unsubscribe.snap

@@ -1 +1 @@
-59260
+63080

src/PositionManager.sol

@@ -158,6 +158,12 @@ contract PositionManager is
         _;
     }
 
+    /// @notice Enforces that the PoolManager is locked.
+    modifier onlyIfPoolManagerLocked() override {
+        if (poolManager.isUnlocked()) revert PoolManagerMustBeLocked();
+        _;
+    }
+
     function tokenURI(uint256 tokenId) public view override returns (string memory) {
         return IPositionDescriptor(tokenDescriptor).tokenURI(this, tokenId);
     }
@@ -444,7 +450,8 @@ contract PositionManager is
     }
 
     /// @dev overrides solmate transferFrom in case a notification to subscribers is needed
-    function transferFrom(address from, address to, uint256 id) public virtual override {
+    /// @dev will revert if pool manager is locked
+    function transferFrom(address from, address to, uint256 id) public virtual override onlyIfPoolManagerLocked {
         super.transferFrom(from, to, id);
         if (positionInfo[id].hasSubscriber()) _notifyTransfer(id, from, to);
     }

src/base/Notifier.sol

@@ -29,6 +29,9 @@ abstract contract Notifier is INotifier {
     /// @param tokenId the tokenId of the position
     modifier onlyIfApproved(address caller, uint256 tokenId) virtual;
 
+    /// @notice Enforces that the PoolManager is locked.
+    modifier onlyIfPoolManagerLocked() virtual;
+
     function _setUnsubscribed(uint256 tokenId) internal virtual;
 
     function _setSubscribed(uint256 tokenId) internal virtual;
@@ -37,6 +40,7 @@ abstract contract Notifier is INotifier {
     function subscribe(uint256 tokenId, address newSubscriber, bytes calldata data)
         external
         payable
+        onlyIfPoolManagerLocked
         onlyIfApproved(msg.sender, tokenId)
     {
         ISubscriber _subscriber = subscriber[tokenId];
@@ -56,7 +60,12 @@ abstract contract Notifier is INotifier {
     }
 
     /// @inheritdoc INotifier
-    function unsubscribe(uint256 tokenId) external payable onlyIfApproved(msg.sender, tokenId) {
+    function unsubscribe(uint256 tokenId)
+        external
+        payable
+        onlyIfPoolManagerLocked
+        onlyIfApproved(msg.sender, tokenId)
+    {
         _unsubscribe(tokenId);
     }
 

src/interfaces/INotifier.sol

@@ -36,13 +36,15 @@ interface INotifier {
     /// @param data caller-provided data that's forwarded to the subscriber contract
     /// @dev Calling subscribe when a position is already subscribed will revert
     /// @dev payable so it can be multicalled with NATIVE related actions
+    /// @dev will revert if pool manager is locked
     function subscribe(uint256 tokenId, address newSubscriber, bytes calldata data) external payable;
 
     /// @notice Removes the subscriber from receiving notifications for a respective position
     /// @param tokenId the ERC721 tokenId
     /// @dev Callers must specify a high gas limit (remaining gas should be higher than unsubscriberGasLimit) such that the subscriber can be notified
     /// @dev payable so it can be multicalled with NATIVE related actions
     /// @dev Must always allow a user to unsubscribe. In the case of a malicious subscriber, a user can always unsubscribe safely, ensuring liquidity is always modifiable.
+    /// @dev will revert if pool manager is locked
     function unsubscribe(uint256 tokenId) external payable;
 
     /// @notice Returns and determines the maximum allowable gas-used for notifying unsubscribe

src/interfaces/IPositionManager.sol

@@ -14,6 +14,9 @@ interface IPositionManager is INotifier, IImmutableState {
     error NotApproved(address caller);
     /// @notice Thrown when the block.timestamp exceeds the user-provided deadline
     error DeadlinePassed(uint256 deadline);
+    /// @notice Thrown when calling transfer, subscribe, or unsubscribe when the PoolManager is unlocked.
+    /// @dev This is to prevent hooks from being able to trigger notifications at the same time the position is being modified.
+    error PoolManagerMustBeLocked();
 
     /// @notice Unlocks Uniswap v4 PoolManager and batches actions for modifying liquidity
     /// @dev This is the standard entrypoint for the PositionManager

test/mocks/MockReenterHook.sol

@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.20;
+
+import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol";
+import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol";
+import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol";
+import {BaseTestHooks} from "@uniswap/v4-core/src/test/BaseTestHooks.sol";
+import {PositionManager} from "../../src/PositionManager.sol";
+
+contract MockReenterHook is BaseTestHooks {
+    PositionManager posm;
+
+    function beforeAddLiquidity(
+        address,
+        PoolKey calldata,
+        IPoolManager.ModifyLiquidityParams calldata,
+        bytes calldata functionSelector
+    ) external override returns (bytes4) {
+        if (functionSelector.length == 0) {
+            return this.beforeAddLiquidity.selector;
+        }
+        (bytes4 selector, address owner, uint256 tokenId) = abi.decode(functionSelector, (bytes4, address, uint256));
+
+        if (selector == posm.transferFrom.selector) {
+            posm.transferFrom(owner, address(this), tokenId);
+        } else if (selector == posm.subscribe.selector) {
+            posm.subscribe(tokenId, address(this), "");
+        } else if (selector == posm.unsubscribe.selector) {
+            posm.unsubscribe(tokenId);
+        }
+        return this.beforeAddLiquidity.selector;
+    }
+
+    function setPosm(PositionManager _posm) external {
+        posm = _posm;
+    }
+}

test/position-managers/PositionManager.notifier.t.sol

@@ -9,6 +9,7 @@ import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol";
 import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol";
 import {PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol";
 import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
+import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol";
 import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol";
 import {PosmTestSetup} from "../shared/PosmTestSetup.sol";
 import {MockSubscriber} from "../mocks/MockSubscriber.sol";
@@ -20,6 +21,7 @@ import {Actions} from "../../src/libraries/Actions.sol";
 import {INotifier} from "../../src/interfaces/INotifier.sol";
 import {MockReturnDataSubscriber, MockRevertSubscriber} from "../mocks/MockBadSubscribers.sol";
 import {PositionInfoLibrary, PositionInfo} from "../../src/libraries/PositionInfoLibrary.sol";
+import {MockReenterHook} from "../mocks/MockReenterHook.sol";
 
 contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
     using PoolIdLibrary for PoolKey;
@@ -31,10 +33,13 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
     MockReturnDataSubscriber badSubscriber;
     PositionConfig config;
     MockRevertSubscriber revertSubscriber;
+    MockReenterHook reenterHook;
 
     address alice = makeAddr("ALICE");
     address bob = makeAddr("BOB");
 
+    PositionConfig reenterConfig;
+
     function setUp() public {
         deployFreshManagerAndRouters();
         deployMintAndApprove2Currencies();
@@ -49,6 +54,17 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         revertSubscriber = new MockRevertSubscriber(lpm);
         config = PositionConfig({poolKey: key, tickLower: -300, tickUpper: 300});
 
+        // set the reenter hook
+        MockReenterHook impl = new MockReenterHook();
+        address hookAddr = payable(address(uint160(Hooks.BEFORE_ADD_LIQUIDITY_FLAG)));
+        vm.etch(hookAddr, address(impl).code);
+        reenterHook = MockReenterHook(hookAddr);
+        reenterHook.setPosm(lpm);
+
+        PoolKey memory reenterKey = PoolKey(currency0, currency1, 3000, 60, IHooks(reenterHook));
+        manager.initialize(reenterKey, SQRT_PRICE_1_1);
+        reenterConfig = PositionConfig({poolKey: reenterKey, tickLower: -60, tickUpper: 60});
+
         // TODO: Test NATIVE poolKey
     }
 
@@ -647,4 +663,69 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
             assertEq(sub.notifyUnsubscribeCount(), beforeUnsubCount + 1);
         }
     }
+
+    function test_unsubscribe_reverts_PoolManagerMustBeLocked() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(reenterConfig, 10e18, address(this), ZERO_BYTES);
+
+        bytes memory hookData = abi.encode(lpm.unsubscribe.selector, address(this), tokenId);
+        bytes memory actions = getMintEncoded(reenterConfig, 10e18, address(this), hookData);
+
+        // approve hook as it should not revert because it does not have permissions
+        lpm.approve(address(reenterHook), tokenId);
+        // subscribe as it should not revert because there is no subscriber
+        lpm.subscribe(tokenId, address(sub), ZERO_BYTES);
+
+        // should revert since the pool manager is unlocked
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.Wrap__FailedHookCall.selector,
+                address(reenterHook),
+                abi.encodeWithSelector(IPositionManager.PoolManagerMustBeLocked.selector)
+            )
+        );
+        lpm.modifyLiquidities(actions, _deadline);
+    }
+
+    function test_subscribe_reverts_PoolManagerMustBeLocked() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(reenterConfig, 10e18, address(this), ZERO_BYTES);
+
+        bytes memory hookData = abi.encode(lpm.subscribe.selector, address(this), tokenId);
+        bytes memory actions = getMintEncoded(reenterConfig, 10e18, address(this), hookData);
+
+        // approve hook as it should not revert because it does not have permissions
+        lpm.approve(address(reenterHook), tokenId);
+
+        // should revert since the pool manager is unlocked
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.Wrap__FailedHookCall.selector,
+                address(reenterHook),
+                abi.encodeWithSelector(IPositionManager.PoolManagerMustBeLocked.selector)
+            )
+        );
+        lpm.modifyLiquidities(actions, _deadline);
+    }
+
+    function test_transferFrom_reverts_PoolManagerMustBeLocked() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(reenterConfig, 10e18, address(this), ZERO_BYTES);
+
+        bytes memory hookData = abi.encode(lpm.transferFrom.selector, address(this), tokenId);
+        bytes memory actions = getMintEncoded(reenterConfig, 10e18, address(this), hookData);
+
+        // approve hook as it should not revert because it does not have permissions
+        lpm.approve(address(reenterHook), tokenId);
+
+        // should revert since the pool manager is unlocked
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Hooks.Wrap__FailedHookCall.selector,
+                address(reenterHook),
+                abi.encodeWithSelector(IPositionManager.PoolManagerMustBeLocked.selector)
+            )
+        );
+        lpm.modifyLiquidities(actions, _deadline);
+    }
 }