OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit

.forge-snapshots/PositionManager_multicall_initialize_mint.snap

@@ -1 +1 @@
-420775
+420753

.forge-snapshots/PositionManager_permit.snap

@@ -1 +1 @@
-79492
+79470

.forge-snapshots/PositionManager_permit_secondPosition.snap

@@ -1 +1 @@
-62380
+62358

.forge-snapshots/PositionManager_permit_twice.snap

@@ -1 +1 @@
-45268
+45246

script/DeployPosm.s.sol

@@ -12,10 +12,15 @@ import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"
 contract DeployPosmTest is Script {
     function setUp() public {}
 
-    function run(address poolManager, address permit2) public returns (PositionManager posm) {
+    function run(address poolManager, address permit2, uint256 unsubscribeGasLimit)
+        public
+        returns (PositionManager posm)
+    {
         vm.startBroadcast();
 
-        posm = new PositionManager{salt: hex"03"}(IPoolManager(poolManager), IAllowanceTransfer(permit2));
+        posm = new PositionManager{salt: hex"03"}(
+            IPoolManager(poolManager), IAllowanceTransfer(permit2), unsubscribeGasLimit
+        );
         console2.log("PositionManager", address(posm));
 
         vm.stopBroadcast();

src/PositionManager.sol

@@ -125,10 +125,11 @@ contract PositionManager is
         return positionConfigs[tokenId];
     }
 
-    constructor(IPoolManager _poolManager, IAllowanceTransfer _permit2)
+    constructor(IPoolManager _poolManager, IAllowanceTransfer _permit2, uint256 _unsubscribeGasLimit)
         BaseActionsRouter(_poolManager)
         Permit2Forwarder(_permit2)
         ERC721Permit_v4("Uniswap V4 Positions NFT", "UNI-V4-POSM")
+        Notifier(_unsubscribeGasLimit)
     {}
 
     /// @notice Reverts if the deadline has passed

src/base/Notifier.sol

@@ -4,26 +4,27 @@ pragma solidity ^0.8.0;
 import {ISubscriber} from "../interfaces/ISubscriber.sol";
 import {PositionConfig} from "../libraries/PositionConfig.sol";
 import {PositionConfigId, PositionConfigIdLibrary} from "../libraries/PositionConfigId.sol";
-import {BipsLibrary} from "../libraries/BipsLibrary.sol";
 import {INotifier} from "../interfaces/INotifier.sol";
 import {CustomRevert} from "@uniswap/v4-core/src/libraries/CustomRevert.sol";
 import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
 
 /// @notice Notifier is used to opt in to sending updates to external contracts about position modifications or transfers
 abstract contract Notifier is INotifier {
-    using BipsLibrary for uint256;
     using CustomRevert for bytes4;
     using PositionConfigIdLibrary for PositionConfigId;
 
     ISubscriber private constant NO_SUBSCRIBER = ISubscriber(address(0));
 
-    // a percentage of the block.gaslimit denoted in BPS, used as the gas limit for subscriber calls
-    // 100 bps is 1%, at 30M gas, the limit is 300K
-    uint256 private constant BLOCK_LIMIT_BPS = 100;
+    /// @inheritdoc INotifier
+    uint256 public immutable unsubscribeGasLimit;
 
     /// @inheritdoc INotifier
     mapping(uint256 tokenId => ISubscriber subscriber) public subscriber;
 
+    constructor(uint256 _unsubscribeGasLimit) {
+        unsubscribeGasLimit = _unsubscribeGasLimit;
+    }
+
     modifier onlyIfApproved(address caller, uint256 tokenId) virtual;
     modifier onlyValidConfig(uint256 tokenId, PositionConfig calldata config) virtual;
 
@@ -65,13 +66,17 @@ abstract contract Notifier is INotifier {
     function _unsubscribe(uint256 tokenId, PositionConfig calldata config) internal {
         _positionConfigs(tokenId).setUnsubscribe();
         ISubscriber _subscriber = subscriber[tokenId];
+        if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith();
 
         delete subscriber[tokenId];
 
-        // A gas limit and a try-catch block are used to protect users from a malicious subscriber.
-        // Users should always be able to unsubscribe, not matter how the subscriber behaves.
-        uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS);
-        try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config) {} catch {}
+        if (address(_subscriber).code.length > 0) {
+            // require that the remaining gas is sufficient to notify the subscriber
+            // otherwise, users can select a gas limit where .notifyUnsubscribe hits OutOfGas yet the transaction/unsubscription
+            // can still succeed
+            if (gasleft() < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith();
+            try _subscriber.notifyUnsubscribe{gas: unsubscribeGasLimit}(tokenId, config) {} catch {}
+        }
 
         emit Unsubscription(tokenId, address(_subscriber));
     }
@@ -106,6 +111,7 @@ abstract contract Notifier is INotifier {
     }
 
     function _call(address target, bytes memory encodedCall) internal returns (bool success) {
+        if (target.code.length == 0) NoCodeSubscriber.selector.revertWith();
         assembly ("memory-safe") {
             success := call(gas(), target, 0, add(encodedCall, 0x20), mload(encodedCall), 0, 0)
         }

src/interfaces/INotifier.sol

@@ -6,6 +6,12 @@ import {ISubscriber} from "./ISubscriber.sol";
 
 /// @notice This interface is used to opt in to sending updates to external contracts about position modifications or transfers
 interface INotifier {
+    /// @notice Thrown when unsubscribing without a subscriber
+    error NotSubscribed();
+    /// @notice Thrown when a subscriber does not have code
+    error NoCodeSubscriber();
+    /// @notice Thrown when a user specifies a gas limit too low to avoid valid unsubscribe notifications
+    error GasLimitTooLow();
     /// @notice Wraps the revert message of the subscriber contract on a reverting subscription
     error Wrap__SubscriptionReverted(address subscriber, bytes reason);
     /// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification
@@ -39,6 +45,7 @@ interface INotifier {
     /// @notice Removes the subscriber from receiving notifications for a respective position
     /// @param tokenId the ERC721 tokenId
     /// @param config the corresponding PositionConfig for the tokenId
+    /// @dev Callers must specify a high gas limit (remaining gas should be higher than subscriberGasLimit) 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.
     function unsubscribe(uint256 tokenId, PositionConfig calldata config) external payable;
@@ -47,4 +54,8 @@ interface INotifier {
     /// @param tokenId the ERC721 tokenId
     /// @return bool whether or not the position has a subscriber
     function hasSubscriber(uint256 tokenId) external view returns (bool);
+
+    /// @notice Returns and determines the maximum allowable gas-used for notifying unsubscribe
+    /// @return uint256 the maximum gas limit when notifying a subscriber's `notifyUnsubscribe` function
+    function unsubscribeGasLimit() external view returns (uint256);
 }

src/interfaces/ISubscriber.sol

@@ -10,6 +10,9 @@ interface ISubscriber {
     /// @param config details about the position
     /// @param data additional data passed in by the caller
     function notifySubscribe(uint256 tokenId, PositionConfig memory config, 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
     /// @param config details about the position
     function notifyUnsubscribe(uint256 tokenId, PositionConfig memory config) external;

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

@@ -97,6 +97,22 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(sub.notifySubscribeCount(), 1);
     }
 
+    /// @notice Revert when subscribing to an address without code
+    function test_subscribe_revert_empty(address _subscriber) public {
+        vm.assume(_subscriber.code.length == 0);
+
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        lpm.subscribe(tokenId, config, _subscriber, ZERO_BYTES);
+    }
+
     function test_subscribe_revertsWithAlreadySubscribed() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
@@ -145,6 +161,28 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(sub.notifyModifyLiquidityCount(), 10);
     }
 
+    function test_notifyModifyLiquidity_selfDestruct_revert() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+
+        assertEq(lpm.hasSubscriber(tokenId), true);
+        assertEq(address(lpm.subscriber(tokenId)), address(sub));
+
+        // simulate selfdestruct by etching the bytecode to 0
+        vm.etch(address(sub), ZERO_BYTES);
+
+        uint256 liquidityToAdd = 10e18;
+        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        increaseLiquidity(tokenId, config, liquidityToAdd, ZERO_BYTES);
+    }
+
     function test_notifyModifyLiquidity_args() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
@@ -192,6 +230,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(sub.notifyTransferCount(), 1);
     }
 
+    function test_notifyTransfer_withTransferFrom_selfDestruct_revert() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+        assertEq(lpm.hasSubscriber(tokenId), true);
+        assertEq(address(lpm.subscriber(tokenId)), address(sub));
+
+        // simulate selfdestruct by etching the bytecode to 0
+        vm.etch(address(sub), ZERO_BYTES);
+
+        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        lpm.transferFrom(alice, bob, tokenId);
+    }
+
     function test_notifyTransfer_withSafeTransferFrom_succeeds() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
@@ -211,6 +269,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(sub.notifyTransferCount(), 1);
     }
 
+    function test_notifyTransfer_withSafeTransferFrom_selfDestruct_revert() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+        assertEq(lpm.hasSubscriber(tokenId), true);
+        assertEq(address(lpm.subscriber(tokenId)), address(sub));
+
+        // simulate selfdestruct by etching the bytecode to 0
+        vm.etch(address(sub), ZERO_BYTES);
+
+        vm.expectRevert(INotifier.NoCodeSubscriber.selector);
+        lpm.safeTransferFrom(alice, bob, tokenId);
+    }
+
     function test_notifyTransfer_withSafeTransferFromData_succeeds() public {
         uint256 tokenId = lpm.nextTokenId();
         mint(config, 100e18, alice, ZERO_BYTES);
@@ -268,6 +346,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(address(lpm.subscriber(tokenId)), address(0));
     }
 
+    function test_unsubscribe_selfDestructed() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+
+        // simulate selfdestruct by etching the bytecode to 0
+        vm.etch(address(sub), ZERO_BYTES);
+
+        lpm.unsubscribe(tokenId, config);
+
+        assertEq(lpm.hasSubscriber(tokenId), false);
+        assertEq(address(lpm.subscriber(tokenId)), address(0));
+    }
+
     function test_multicall_mint_subscribe() public {
         uint256 tokenId = lpm.nextTokenId();
 
@@ -339,7 +437,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         lpm.approve(address(this), tokenId);
         vm.stopPrank();
 
-        vm.expectRevert();
+        vm.expectRevert(INotifier.NotSubscribed.selector);
         lpm.unsubscribe(tokenId, config);
     }
 
@@ -500,4 +598,31 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         assertEq(lpm.hasSubscriber(tokenId), false);
         assertEq(sub.notifyUnsubscribeCount(), 1);
     }
+
+    /// @notice Test that users cannot forcibly avoid unsubscribe logic via gas limits
+    function test_fuzz_unsubscribe_with_gas_limit(uint64 gasLimit) public {
+        // enforce a minimum amount of gas to avoid OutOfGas reverts
+        gasLimit = uint64(bound(gasLimit, 125_000, block.gaslimit));
+
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 100e18, alice, ZERO_BYTES);
+
+        // approve this contract to operate on alices liq
+        vm.startPrank(alice);
+        lpm.approve(address(this), tokenId);
+        vm.stopPrank();
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+        uint256 beforeUnsubCount = sub.notifyUnsubscribeCount();
+
+        if (gasLimit < lpm.unsubscribeGasLimit()) {
+            // gas too low to call a valid unsubscribe
+            vm.expectRevert(INotifier.GasLimitTooLow.selector);
+            lpm.unsubscribe{gas: gasLimit}(tokenId, config);
+        } else {
+            // increasing gas limit succeeds and unsubscribe was called
+            lpm.unsubscribe{gas: gasLimit}(tokenId, config);
+            assertEq(sub.notifyUnsubscribeCount(), beforeUnsubCount + 1);
+        }
+    }
 }

test/shared/PosmTestSetup.sol

@@ -57,7 +57,7 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations {
     function deployPosm(IPoolManager poolManager) internal {
         // We use deployPermit2() to prevent having to use via-ir in this repository.
         permit2 = IAllowanceTransfer(deployPermit2());
-        lpm = new PositionManager(poolManager, permit2);
+        lpm = new PositionManager(poolManager, permit2, 100_000);
     }
 
     function seedBalance(address to) internal {