OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, U…

…nsubscribe Minimum Gas Limit (#318) * check subscriber code length * revert unsubscribe if not subscribed * check if subscriber has code before notifying * custom reverts * SB-M78: minimum gas limit on unsubscribe * natspec on unsubscribe for SB M78 * Update src/interfaces/INotifier.sol Co-authored-by: Alice <[email protected]> * use constructor for subscriber gas limit * natspec for unsubscribeGasLimit * document EIP-150 instead * Update src/base/Notifier.sol Co-authored-by: Alice <[email protected]> * forge fmt lol * gas benchmarks for subscribe/unsubscribe * gas optimize unsubscribe check --------- Co-authored-by: Alice <[email protected]>
Uniswap · Sep 4, 2024 · 255b20e · 255b20e
1 parent f9cf32b
commit 255b20e
Show file tree

Hide file tree

Showing 14 changed files with 204 additions and 18 deletions.
diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap
@@ -1 +1 @@
-420775
+420753
diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap
@@ -1 +1 @@
-79492
+79470
diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap
@@ -1 +1 @@
-62380
+62358
diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap
@@ -1 +1 @@
-45268
+45246
diff --git a/.forge-snapshots/PositionManager_subscribe.snap b/.forge-snapshots/PositionManager_subscribe.snap
@@ -0,0 +1 @@
+88474
diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap
@@ -0,0 +1 @@
+62639
diff --git a/script/DeployPosm.s.sol b/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();

diff --git a/src/PositionManager.sol b/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

diff --git a/src/base/Notifier.sol b/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;
+    }
+
     /// @notice Only allow callers that are approved as spenders or operators of the tokenId
     /// @dev to be implemented by the parent contract (PositionManager)
     /// @param caller the address of the caller
@@ -68,6 +69,7 @@ abstract contract Notifier is INotifier {
         onlyIfApproved(msg.sender, tokenId)
         onlyValidConfig(tokenId, config)
     {
+        if (!_positionConfigs(tokenId).hasSubscriber()) NotSubscribed.selector.revertWith();
         _unsubscribe(tokenId, config);
     }
 
@@ -77,10 +79,13 @@ abstract contract Notifier is INotifier {
 
         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));
     }
@@ -115,6 +120,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)
         }

diff --git a/src/interfaces/INotifier.sol b/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);
 }
diff --git a/src/interfaces/ISubscriber.sol b/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;

diff --git a/test/position-managers/PositionManager.gas.t.sol b/test/position-managers/PositionManager.gas.t.sol
@@ -24,6 +24,7 @@ import {IMulticall_v4} from "../../src/interfaces/IMulticall_v4.sol";
 import {Planner, Plan} from "../shared/Planner.sol";
 import {PosmTestSetup} from "../shared/PosmTestSetup.sol";
 import {ActionConstants} from "../../src/libraries/ActionConstants.sol";
+import {MockSubscriber} from "../mocks/MockSubscriber.sol";
 
 contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
     using FixedPointMathLib for uint256;
@@ -43,6 +44,8 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
     PositionConfig config;
     PositionConfig configNative;
 
+    MockSubscriber sub;
+
     function setUp() public {
         (alice, alicePK) = makeAddrAndKey("ALICE");
         (bob, bobPK) = makeAddrAndKey("BOB");
@@ -68,6 +71,8 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
         // define a reusable range
         config = PositionConfig({poolKey: key, tickLower: -300, tickUpper: 300});
         configNative = PositionConfig({poolKey: nativeKey, tickLower: -300, tickUpper: 300});
+
+        sub = new MockSubscriber(lpm);
     }
 
     function test_gas_mint_withClose() public {
@@ -850,4 +855,15 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot {
         lpm.modifyLiquidities(calls, _deadline);
         snapLastCall("PositionManager_decrease_take_take");
     }
+
+    function test_gas_subscribe_unsubscribe() public {
+        uint256 tokenId = lpm.nextTokenId();
+        mint(config, 1e18, ActionConstants.MSG_SENDER, ZERO_BYTES);
+
+        lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES);
+        snapLastCall("PositionManager_subscribe");
+
+        lpm.unsubscribe(tokenId, config);
+        snapLastCall("PositionManager_unsubscribe");
+    }
 }
diff --git a/test/position-managers/PositionManager.notifier.t.sol b/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,24 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
         lpm.approve(address(this), tokenId);
         vm.stopPrank();
 
-        vm.expectRevert();
+        vm.expectRevert(INotifier.NotSubscribed.selector);
+        lpm.unsubscribe(tokenId, config);
+    }
+
+    function test_unsubscribe_twice_reverts() 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);
+
+        lpm.unsubscribe(tokenId, config);
+
+        vm.expectRevert(INotifier.NotSubscribed.selector);
         lpm.unsubscribe(tokenId, config);
     }
 
@@ -500,4 +615,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);
+        }
+    }
 }