From eca398f78cbb82004908d44872523f3ac3ac7ec5 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Thu, 29 Aug 2024 20:15:32 -0400 Subject: [PATCH 01/14] check subscriber code length --- src/base/Notifier.sol | 6 ++++-- .../PositionManager.notifier.t.sol | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 3fe6b2406..5e56c384d 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -71,8 +71,10 @@ abstract contract Notifier is INotifier { delete subscriber[tokenId]; - uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS); - try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {} + if (address(_subscriber).code.length > 0) { + uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS); + try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {} + } emit Unsubscribed(tokenId, address(_subscriber)); } diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 28f5813f4..9a5dca4d4 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -249,6 +249,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, ZERO_BYTES); + + assertEq(lpm.hasSubscriber(tokenId), false); + assertEq(address(lpm.subscriber(tokenId)), address(0)); + } + function test_multicall_mint_subscribe() public { uint256 tokenId = lpm.nextTokenId(); From c93fc95493b32d7ba50194d1ae9817ffc48f35f6 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Thu, 29 Aug 2024 20:15:50 -0400 Subject: [PATCH 02/14] revert unsubscribe if not subscribed --- src/base/Notifier.sol | 1 + src/interfaces/INotifier.sol | 2 ++ test/position-managers/PositionManager.notifier.t.sol | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 5e56c384d..318e5dadf 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -68,6 +68,7 @@ abstract contract Notifier is INotifier { { _positionConfigs(tokenId).setUnsubscribe(); ISubscriber _subscriber = subscriber[tokenId]; + if (_subscriber == NO_SUBSCRIBER) revert NotSubscribed(); delete subscriber[tokenId]; diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index 64f734309..49ecddea9 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -6,6 +6,8 @@ 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 Wraps the revert message of the subscriber contract on a reverting subscription error Wrap__SubsciptionReverted(address subscriber, bytes reason); /// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 9a5dca4d4..82be5f3d9 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -340,7 +340,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, ZERO_BYTES); } From dd948d29df5e8249226a8c17f2bd0d79cfe74ba5 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Thu, 29 Aug 2024 21:50:59 -0400 Subject: [PATCH 03/14] check if subscriber has code before notifying --- src/base/Notifier.sol | 1 + src/interfaces/INotifier.sol | 2 + .../PositionManager.notifier.t.sol | 78 +++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 318e5dadf..a114528c3 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -110,6 +110,7 @@ abstract contract Notifier is INotifier { } function _call(address target, bytes memory encodedCall) internal returns (bool success) { + if (target.code.length == 0) revert NoCodeSubscriber(); 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 index 49ecddea9..f70d5b8b3 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -8,6 +8,8 @@ import {ISubscriber} from "./ISubscriber.sol"; interface INotifier { /// @notice Thrown when unsubscribing without a subscriber error NotSubscribed(); + /// @notice Thrown when a subscriber does not have code + error NoCodeSubscriber(); /// @notice Wraps the revert message of the subscriber contract on a reverting subscription error Wrap__SubsciptionReverted(address subscriber, bytes reason); /// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 82be5f3d9..3f566ccc3 100644 --- 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_notifyModifyLiquidity_succeeds() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -126,6 +142,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); @@ -173,6 +211,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); @@ -192,6 +250,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); From 845a1d4b044a5924ff200aae735760693d5b56fe Mon Sep 17 00:00:00 2001 From: saucepoint Date: Thu, 29 Aug 2024 22:32:54 -0400 Subject: [PATCH 04/14] custom reverts --- src/base/Notifier.sol | 6 +++--- test/position-managers/PositionManager.notifier.t.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index a114528c3..7099e5263 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -46,7 +46,7 @@ abstract contract Notifier is INotifier { _positionConfigs(tokenId).setSubscribe(); ISubscriber _subscriber = subscriber[tokenId]; - if (_subscriber != NO_SUBSCRIBER) revert AlreadySubscribed(address(_subscriber)); + if (_subscriber != NO_SUBSCRIBER) AlreadySubscribed.selector.revertWith(address(_subscriber)); subscriber[tokenId] = ISubscriber(newSubscriber); bool success = @@ -68,7 +68,7 @@ abstract contract Notifier is INotifier { { _positionConfigs(tokenId).setUnsubscribe(); ISubscriber _subscriber = subscriber[tokenId]; - if (_subscriber == NO_SUBSCRIBER) revert NotSubscribed(); + if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith(); delete subscriber[tokenId]; @@ -110,7 +110,7 @@ abstract contract Notifier is INotifier { } function _call(address target, bytes memory encodedCall) internal returns (bool success) { - if (target.code.length == 0) revert NoCodeSubscriber(); + 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/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 3f566ccc3..966241210 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -100,7 +100,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { /// @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); From 732bee4eb99f67e41e7799931374cf548e571d0b Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 30 Aug 2024 16:30:47 -0400 Subject: [PATCH 05/14] SB-M78: minimum gas limit on unsubscribe --- src/base/Notifier.sol | 6 ++++ src/interfaces/INotifier.sol | 2 ++ .../PositionManager.notifier.t.sol | 31 +++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 2b1f6bfe0..b162aab1a 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -73,6 +73,12 @@ abstract contract Notifier is INotifier { if (address(_subscriber).code.length > 0) { uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS); + + // 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 + // to account for EIP-150, condition could be 64 * gasleft() / 63 <= subscriberGasLimit + if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith(); try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {} } diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index f70d5b8b3..00257c065 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -10,6 +10,8 @@ interface INotifier { 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__SubsciptionReverted(address subscriber, bytes reason); /// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 966241210..d617b24cc 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -19,11 +19,13 @@ import {Actions} from "../../src/libraries/Actions.sol"; import {INotifier} from "../../src/interfaces/INotifier.sol"; import {MockReturnDataSubscriber, MockRevertSubscriber} from "../mocks/MockBadSubscribers.sol"; import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BipsLibrary} from "../../src/libraries/BipsLibrary.sol"; contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { using PoolIdLibrary for PoolKey; using StateLibrary for IPoolManager; using Planner for Plan; + using BipsLibrary for uint256; MockSubscriber sub; MockReturnDataSubscriber badSubscriber; @@ -575,4 +577,33 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { ); lpm.safeTransferFrom(alice, bob, tokenId, ""); } + + /// @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, 150_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(); + + uint256 subscriberGasLimit = block.gaslimit.calculatePortion(100); + + if (gasLimit < subscriberGasLimit) { + // gas too low to call a valid unsubscribe + vm.expectRevert(INotifier.GasLimitTooLow.selector); + lpm.unsubscribe{gas: gasLimit}(tokenId, config, ZERO_BYTES); + } else { + // increasing gas limit succeeds and unsubscribe was called + lpm.unsubscribe{gas: gasLimit}(tokenId, config, ZERO_BYTES); + assertEq(sub.notifyUnsubscribeCount(), beforeUnsubCount + 1); + } + } } From 234596f7dc9ba3feb0abe90cf3fbdfcd660ee89d Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 30 Aug 2024 17:10:31 -0400 Subject: [PATCH 06/14] natspec on unsubscribe for SB M78 --- src/interfaces/INotifier.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index 00257c065..657dd41e1 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -39,6 +39,7 @@ interface INotifier { /// @param tokenId the ERC721 tokenId /// @param config the corresponding PositionConfig for the tokenId /// @param data caller-provided data that's forwarded to the subscriber contract + /// @dev Callers must specify a high gas limit (remaining gas should be higher than 300,000 gas) 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, bytes calldata data) external payable; From a05c0c94c72ed716189c2bf176c3bc6e5a21b59e Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Tue, 3 Sep 2024 10:45:26 -0400 Subject: [PATCH 07/14] Update src/interfaces/INotifier.sol Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com> --- src/interfaces/INotifier.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index 657dd41e1..45d8f2151 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -39,7 +39,7 @@ interface INotifier { /// @param tokenId the ERC721 tokenId /// @param config the corresponding PositionConfig for the tokenId /// @param data caller-provided data that's forwarded to the subscriber contract - /// @dev Callers must specify a high gas limit (remaining gas should be higher than 300,000 gas) such that the subscriber can be notified + /// @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, bytes calldata data) external payable; From e7fe727188b9210bbbfd1cb94c0e9feb5c935eb3 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Tue, 3 Sep 2024 14:29:13 -0400 Subject: [PATCH 08/14] use constructor for subscriber gas limit --- ...tionManager_multicall_initialize_mint.snap | 2 +- .forge-snapshots/PositionManager_permit.snap | 2 +- ...PositionManager_permit_secondPosition.snap | 2 +- .../PositionManager_permit_twice.snap | 2 +- script/DeployPosm.s.sol | 4 ++-- src/PositionManager.sol | 3 ++- src/base/Notifier.sol | 19 ++++++++----------- .../PositionManager.notifier.t.sol | 8 ++------ test/shared/PosmTestSetup.sol | 2 +- 9 files changed, 19 insertions(+), 25 deletions(-) diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap index e06544be4..ef991ef7d 100644 --- a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap +++ b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap @@ -1 +1 @@ -421102 \ No newline at end of file +421080 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap index d3f21d676..a7d5dc1ae 100644 --- a/.forge-snapshots/PositionManager_permit.snap +++ b/.forge-snapshots/PositionManager_permit.snap @@ -1 +1 @@ -79595 \ No newline at end of file +79573 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap index 4bd3ebf9a..5761f4449 100644 --- a/.forge-snapshots/PositionManager_permit_secondPosition.snap +++ b/.forge-snapshots/PositionManager_permit_secondPosition.snap @@ -1 +1 @@ -62483 \ No newline at end of file +62461 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap index 39f8dac50..b551d8fd8 100644 --- a/.forge-snapshots/PositionManager_permit_twice.snap +++ b/.forge-snapshots/PositionManager_permit_twice.snap @@ -1 +1 @@ -45371 \ No newline at end of file +45349 \ No newline at end of file diff --git a/script/DeployPosm.s.sol b/script/DeployPosm.s.sol index 92084bdca..7775f81bf 100644 --- a/script/DeployPosm.s.sol +++ b/script/DeployPosm.s.sol @@ -12,10 +12,10 @@ 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 index 5f76ed518..4c563f998 100644 --- 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 index ecdf53dfb..4c47398a4 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -4,14 +4,12 @@ pragma solidity ^0.8.24; 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; @@ -22,14 +20,15 @@ abstract contract Notifier is INotifier { 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; + 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; @@ -76,14 +75,12 @@ abstract contract Notifier is INotifier { delete subscriber[tokenId]; if (address(_subscriber).code.length > 0) { - uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS); - // 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 - // to account for EIP-150, condition could be 64 * gasleft() / 63 <= subscriberGasLimit - if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith(); - try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config) {} catch {} + // to account for EIP-150, condition could be 64 * gasleft() / 63 <= unsubscribeGasLimit + if ((64 * gasleft() / 63) < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith(); + try _subscriber.notifyUnsubscribe{gas: unsubscribeGasLimit}(tokenId, config) {} catch {} } emit Unsubscribed(tokenId, address(_subscriber)); diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index d5f318cd6..3869a7663 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -19,13 +19,11 @@ import {Actions} from "../../src/libraries/Actions.sol"; import {INotifier} from "../../src/interfaces/INotifier.sol"; import {MockReturnDataSubscriber, MockRevertSubscriber} from "../mocks/MockBadSubscribers.sol"; import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; -import {BipsLibrary} from "../../src/libraries/BipsLibrary.sol"; contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { using PoolIdLibrary for PoolKey; using StateLibrary for IPoolManager; using Planner for Plan; - using BipsLibrary for uint256; MockSubscriber sub; MockReturnDataSubscriber badSubscriber; @@ -585,7 +583,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { /// @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, 150_000, block.gaslimit)); + gasLimit = uint64(bound(gasLimit, 120_000, block.gaslimit)); uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -598,9 +596,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); uint256 beforeUnsubCount = sub.notifyUnsubscribeCount(); - uint256 subscriberGasLimit = block.gaslimit.calculatePortion(100); - - if (gasLimit < subscriberGasLimit) { + if (gasLimit < lpm.unsubscribeGasLimit()) { // gas too low to call a valid unsubscribe vm.expectRevert(INotifier.GasLimitTooLow.selector); lpm.unsubscribe{gas: gasLimit}(tokenId, config); diff --git a/test/shared/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index 345726c4d..1154e059c 100644 --- a/test/shared/PosmTestSetup.sol +++ b/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 { From de32571f6b8ea98d677c52c0548ad43ca88fa72d Mon Sep 17 00:00:00 2001 From: saucepoint Date: Tue, 3 Sep 2024 14:33:49 -0400 Subject: [PATCH 09/14] natspec for unsubscribeGasLimit --- src/base/Notifier.sol | 1 + src/interfaces/INotifier.sol | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 4c47398a4..e22c02f06 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -20,6 +20,7 @@ abstract contract Notifier is INotifier { ISubscriber private constant NO_SUBSCRIBER = ISubscriber(address(0)); + /// @inheritdoc INotifier uint256 public immutable unsubscribeGasLimit; /// @inheritdoc INotifier diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index 2bb005e5d..6170f479a 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -47,4 +47,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); } From e96d8d3bb55ee595abffb2b0c45ec2f55f449ec2 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Tue, 3 Sep 2024 17:31:15 -0400 Subject: [PATCH 10/14] document EIP-150 instead --- src/base/Notifier.sol | 5 ++--- src/interfaces/ISubscriber.sol | 3 +++ test/position-managers/PositionManager.notifier.t.sol | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index e22c02f06..fdd8c815b 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -26,7 +26,7 @@ abstract contract Notifier is INotifier { /// @inheritdoc INotifier mapping(uint256 tokenId => ISubscriber subscriber) public subscriber; - constructor (uint256 _unsubscribeGasLimit) { + constructor(uint256 _unsubscribeGasLimit) { unsubscribeGasLimit = _unsubscribeGasLimit; } @@ -79,8 +79,7 @@ abstract contract Notifier is INotifier { // 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 - // to account for EIP-150, condition could be 64 * gasleft() / 63 <= unsubscribeGasLimit - if ((64 * gasleft() / 63) < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith(); + if (gasleft() < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith(); try _subscriber.notifyUnsubscribe{gas: unsubscribeGasLimit}(tokenId, config) {} catch {} } diff --git a/src/interfaces/ISubscriber.sol b/src/interfaces/ISubscriber.sol index 78bdf41cb..0c3c6890c 100644 --- 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.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 3869a7663..066d5866b 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -583,7 +583,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { /// @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, 120_000, block.gaslimit)); + gasLimit = uint64(bound(gasLimit, 125_000, block.gaslimit)); uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); From 0faaa5976fa69354a0b3f8ac40d3c8cac99bf4cc Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:08:52 -0400 Subject: [PATCH 11/14] Update src/base/Notifier.sol Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com> --- src/base/Notifier.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index ad5c682e5..d29d3adc1 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -72,8 +72,8 @@ abstract contract Notifier is INotifier { 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 + // 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 {} } From 21fa51f9c1557a612c79e04ab8de8ab507726fff Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 4 Sep 2024 11:17:46 -0400 Subject: [PATCH 12/14] forge fmt lol --- src/base/Notifier.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index d29d3adc1..4fdf9fe6e 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -72,7 +72,7 @@ abstract contract Notifier is INotifier { 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 + // 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 {} From f151630ad26e4231fcb1c4042bd2978e0e7033d5 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 4 Sep 2024 11:33:09 -0400 Subject: [PATCH 13/14] gas benchmarks for subscribe/unsubscribe --- .forge-snapshots/PositionManager_subscribe.snap | 1 + .../PositionManager_unsubscribe.snap | 1 + test/position-managers/PositionManager.gas.t.sol | 16 ++++++++++++++++ 3 files changed, 18 insertions(+) create mode 100644 .forge-snapshots/PositionManager_subscribe.snap create mode 100644 .forge-snapshots/PositionManager_unsubscribe.snap diff --git a/.forge-snapshots/PositionManager_subscribe.snap b/.forge-snapshots/PositionManager_subscribe.snap new file mode 100644 index 000000000..dec6506f4 --- /dev/null +++ b/.forge-snapshots/PositionManager_subscribe.snap @@ -0,0 +1 @@ +88474 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap new file mode 100644 index 000000000..a1d32a0b6 --- /dev/null +++ b/.forge-snapshots/PositionManager_unsubscribe.snap @@ -0,0 +1 @@ +62617 \ No newline at end of file diff --git a/test/position-managers/PositionManager.gas.t.sol b/test/position-managers/PositionManager.gas.t.sol index 161321eac..2463b7102 100644 --- 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"); + } } From dc3a62377cef3a12f6a34b658bdac68df85f37e9 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 4 Sep 2024 11:44:48 -0400 Subject: [PATCH 14/14] gas optimize unsubscribe check --- .../PositionManager_unsubscribe.snap | 2 +- src/base/Notifier.sol | 6 ++++-- .../PositionManager.notifier.t.sol | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap index a1d32a0b6..f7ec2b08e 100644 --- a/.forge-snapshots/PositionManager_unsubscribe.snap +++ b/.forge-snapshots/PositionManager_unsubscribe.snap @@ -1 +1 @@ -62617 \ No newline at end of file +62559 \ No newline at end of file diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 4fdf9fe6e..44771d23c 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -64,9 +64,11 @@ abstract contract Notifier is INotifier { } function _unsubscribe(uint256 tokenId, PositionConfig calldata config) internal { - _positionConfigs(tokenId).setUnsubscribe(); + PositionConfigId storage _positionConfig = _positionConfigs(tokenId); + if (!_positionConfig.hasSubscriber()) NotSubscribed.selector.revertWith(); + + _positionConfig.setUnsubscribe(); ISubscriber _subscriber = subscriber[tokenId]; - if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith(); delete subscriber[tokenId]; diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 0bb28dd2a..d06ed52d5 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -441,6 +441,23 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { 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); + } + function test_subscribe_withData() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES);