diff --git a/snapshots/BaseActionsRouterTest.json b/snapshots/BaseActionsRouterTest.json index 1a9c92163..ce8326011 100644 --- a/snapshots/BaseActionsRouterTest.json +++ b/snapshots/BaseActionsRouterTest.json @@ -1,3 +1,3 @@ { - "BaseActionsRouter_mock10commands": "33725" + "BaseActionsRouter_mock10commands": "60677" } \ No newline at end of file diff --git a/snapshots/PaymentsTests.json b/snapshots/PaymentsTests.json index 9a12dc616..e8fdbdeb5 100644 --- a/snapshots/PaymentsTests.json +++ b/snapshots/PaymentsTests.json @@ -1,6 +1,6 @@ { - "Payments_swap_settleFromCaller_takeAllToMsgSender": "104210", - "Payments_swap_settleFromCaller_takeAllToSpecifiedAddress": "104961", - "Payments_swap_settleWithBalance_takeAllToMsgSender": "95138", - "Payments_swap_settleWithBalance_takeAllToSpecifiedAddress": "95052" + "Payments_swap_settleFromCaller_takeAllToMsgSender": "129642", + "Payments_swap_settleFromCaller_takeAllToSpecifiedAddress": "131705", + "Payments_swap_settleWithBalance_takeAllToMsgSender": "123910", + "Payments_swap_settleWithBalance_takeAllToSpecifiedAddress": "124052" } \ No newline at end of file diff --git a/snapshots/PosMGasTest.json b/snapshots/PosMGasTest.json index d513369d1..e3d6088ce 100644 --- a/snapshots/PosMGasTest.json +++ b/snapshots/PosMGasTest.json @@ -1,41 +1,41 @@ { - "PositionManager_burn_empty": "15061", - "PositionManager_burn_empty_native": "15061", - "PositionManager_burn_nonEmpty_native_withClose": "51029", - "PositionManager_burn_nonEmpty_native_withTakePair": "50330", - "PositionManager_burn_nonEmpty_withClose": "47267", - "PositionManager_burn_nonEmpty_withTakePair": "46568", - "PositionManager_collect_native": "77416", - "PositionManager_collect_sameRange": "73654", - "PositionManager_collect_withClose": "73654", - "PositionManager_collect_withTakePair": "72955", - "PositionManager_decreaseLiquidity_native": "44201", - "PositionManager_decreaseLiquidity_withClose": "40439", - "PositionManager_decreaseLiquidity_withTakePair": "39740", - "PositionManager_decrease_burnEmpty": "49571", - "PositionManager_decrease_burnEmpty_native": "53333", - "PositionManager_decrease_sameRange_allLiquidity": "38726", - "PositionManager_decrease_take_take": "40547", - "PositionManager_increaseLiquidity_erc20_withClose": "49647", - "PositionManager_increaseLiquidity_erc20_withSettlePair": "48903", - "PositionManager_increaseLiquidity_native": "47802", - "PositionManager_increase_autocompoundExactUnclaimedFees": "62979", - "PositionManager_increase_autocompoundExcessFeesCredit": "78466", - "PositionManager_increase_autocompound_clearExcess": "72892", - "PositionManager_mint_native": "338359", - "PositionManager_mint_nativeWithSweep_withClose": "346306", - "PositionManager_mint_nativeWithSweep_withSettlePair": "345389", - "PositionManager_mint_onSameTickLower": "256851", - "PositionManager_mint_onSameTickUpper": "261521", - "PositionManager_mint_sameRange": "162190", - "PositionManager_mint_settleWithBalance_sweep": "384846", - "PositionManager_mint_warmedPool_differentRange": "262082", - "PositionManager_mint_withClose": "393304", - "PositionManager_mint_withSettlePair": "392474", - "PositionManager_multicall_initialize_mint": "426688", - "PositionManager_permit": "53780", - "PositionManager_permit_secondPosition": "29380", - "PositionManager_permit_twice": "7480", - "PositionManager_subscribe": "55708", - "PositionManager_unsubscribe": "26756" + "PositionManager_burn_empty": "50481", + "PositionManager_burn_empty_native": "50481", + "PositionManager_burn_nonEmpty_native_withClose": "125659", + "PositionManager_burn_nonEmpty_native_withTakePair": "125141", + "PositionManager_burn_nonEmpty_withClose": "132521", + "PositionManager_burn_nonEmpty_withTakePair": "132004", + "PositionManager_collect_native": "146388", + "PositionManager_collect_sameRange": "154966", + "PositionManager_collect_withClose": "154966", + "PositionManager_collect_withTakePair": "154331", + "PositionManager_decreaseLiquidity_native": "112056", + "PositionManager_decreaseLiquidity_withClose": "119847", + "PositionManager_decreaseLiquidity_withTakePair": "119212", + "PositionManager_decrease_burnEmpty": "135318", + "PositionManager_decrease_burnEmpty_native": "128456", + "PositionManager_decrease_sameRange_allLiquidity": "132534", + "PositionManager_decrease_take_take": "120467", + "PositionManager_increaseLiquidity_erc20_withClose": "159127", + "PositionManager_increaseLiquidity_erc20_withSettlePair": "158079", + "PositionManager_increaseLiquidity_native": "140942", + "PositionManager_increase_autocompoundExactUnclaimedFees": "136403", + "PositionManager_increase_autocompoundExcessFeesCredit": "177458", + "PositionManager_increase_autocompound_clearExcess": "148084", + "PositionManager_mint_native": "364815", + "PositionManager_mint_nativeWithSweep_withClose": "373334", + "PositionManager_mint_nativeWithSweep_withSettlePair": "372569", + "PositionManager_mint_onSameTickLower": "317687", + "PositionManager_mint_onSameTickUpper": "318357", + "PositionManager_mint_sameRange": "243926", + "PositionManager_mint_settleWithBalance_sweep": "419134", + "PositionManager_mint_warmedPool_differentRange": "323718", + "PositionManager_mint_withClose": "420240", + "PositionManager_mint_withSettlePair": "419310", + "PositionManager_multicall_initialize_mint": "456088", + "PositionManager_permit": "79076", + "PositionManager_permit_secondPosition": "61976", + "PositionManager_permit_twice": "44876", + "PositionManager_subscribe": "88168", + "PositionManager_unsubscribe": "63080" } \ No newline at end of file diff --git a/snapshots/PositionDescriptorTest.json b/snapshots/PositionDescriptorTest.json index d3219789c..21edaf633 100644 --- a/snapshots/PositionDescriptorTest.json +++ b/snapshots/PositionDescriptorTest.json @@ -1,3 +1,3 @@ { - "positionDescriptor bytecode size": "31236" + "positionDescriptor bytecode size": "31336" } \ No newline at end of file diff --git a/snapshots/QuoterTest.json b/snapshots/QuoterTest.json index 21aecf764..53525acd3 100644 --- a/snapshots/QuoterTest.json +++ b/snapshots/QuoterTest.json @@ -1,15 +1,15 @@ { - "Quoter_exactInputSingle_oneForZero_multiplePositions": "121454", - "Quoter_exactInputSingle_zeroForOne_multiplePositions": "126894", - "Quoter_exactOutputSingle_oneForZero": "55727", - "Quoter_exactOutputSingle_zeroForOne": "60138", - "Quoter_quoteExactInput_oneHop_1TickLoaded": "97723", - "Quoter_quoteExactInput_oneHop_initializedAfter": "122646", - "Quoter_quoteExactInput_oneHop_startingInitialized": "46181", - "Quoter_quoteExactInput_twoHops": "177431", - "Quoter_quoteExactOutput_oneHop_1TickLoaded": "97014", - "Quoter_quoteExactOutput_oneHop_2TicksLoaded": "127151", - "Quoter_quoteExactOutput_oneHop_initializedAfter": "97082", - "Quoter_quoteExactOutput_oneHop_startingInitialized": "52493", - "Quoter_quoteExactOutput_twoHops": "176882" + "Quoter_exactInputSingle_oneForZero_multiplePositions": "143930", + "Quoter_exactInputSingle_zeroForOne_multiplePositions": "149382", + "Quoter_exactOutputSingle_oneForZero": "78203", + "Quoter_exactOutputSingle_zeroForOne": "82626", + "Quoter_quoteExactInput_oneHop_1TickLoaded": "120491", + "Quoter_quoteExactInput_oneHop_initializedAfter": "145414", + "Quoter_quoteExactInput_oneHop_startingInitialized": "79437", + "Quoter_quoteExactInput_twoHops": "201179", + "Quoter_quoteExactOutput_oneHop_1TickLoaded": "119782", + "Quoter_quoteExactOutput_oneHop_2TicksLoaded": "149919", + "Quoter_quoteExactOutput_oneHop_initializedAfter": "119850", + "Quoter_quoteExactOutput_oneHop_startingInitialized": "96549", + "Quoter_quoteExactOutput_twoHops": "200630" } \ No newline at end of file diff --git a/snapshots/V4RouterTest.json b/snapshots/V4RouterTest.json index 1d9cce472..f229dbf07 100644 --- a/snapshots/V4RouterTest.json +++ b/snapshots/V4RouterTest.json @@ -1,26 +1,26 @@ { "V4Router_Bytecode": "7063", - "V4Router_ExactIn1Hop_nativeIn": "90557", - "V4Router_ExactIn1Hop_nativeOut": "90874", - "V4Router_ExactIn1Hop_oneForZero": "99212", - "V4Router_ExactIn1Hop_zeroForOne": "104935", - "V4Router_ExactIn2Hops": "152841", - "V4Router_ExactIn2Hops_nativeIn": "144178", - "V4Router_ExactIn3Hops": "200750", - "V4Router_ExactIn3Hops_nativeIn": "192087", - "V4Router_ExactInputSingle": "104210", - "V4Router_ExactInputSingle_nativeIn": "89832", - "V4Router_ExactInputSingle_nativeOut": "90129", - "V4Router_ExactOut1Hop_nativeIn_sweepETH": "96628", - "V4Router_ExactOut1Hop_nativeOut": "91746", - "V4Router_ExactOut1Hop_oneForZero": "100084", - "V4Router_ExactOut1Hop_zeroForOne": "104029", - "V4Router_ExactOut2Hops": "152767", - "V4Router_ExactOut2Hops_nativeIn": "149311", - "V4Router_ExactOut3Hops": "201536", - "V4Router_ExactOut3Hops_nativeIn": "198080", - "V4Router_ExactOut3Hops_nativeOut": "193198", - "V4Router_ExactOutputSingle": "103301", - "V4Router_ExactOutputSingle_nativeIn_sweepETH": "95900", - "V4Router_ExactOutputSingle_nativeOut": "91104" + "V4Router_ExactIn1Hop_nativeIn": "115753", + "V4Router_ExactIn1Hop_nativeOut": "116070", + "V4Router_ExactIn1Hop_oneForZero": "124888", + "V4Router_ExactIn1Hop_zeroForOne": "130611", + "V4Router_ExactIn2Hops": "185452", + "V4Router_ExactIn2Hops_nativeIn": "170594", + "V4Router_ExactIn3Hops": "240296", + "V4Router_ExactIn3Hops_nativeIn": "225438", + "V4Router_ExactInputSingle": "129642", + "V4Router_ExactInputSingle_nativeIn": "114784", + "V4Router_ExactInputSingle_nativeOut": "115069", + "V4Router_ExactOut1Hop_nativeIn_sweepETH": "122016", + "V4Router_ExactOut1Hop_nativeOut": "117134", + "V4Router_ExactOut1Hop_oneForZero": "125952", + "V4Router_ExactOut1Hop_zeroForOne": "129897", + "V4Router_ExactOut2Hops": "183800", + "V4Router_ExactOut2Hops_nativeIn": "175919", + "V4Router_ExactOut3Hops": "237734", + "V4Router_ExactOut3Hops_nativeIn": "229853", + "V4Router_ExactOut3Hops_nativeOut": "217089", + "V4Router_ExactOutputSingle": "128925", + "V4Router_ExactOutputSingle_nativeIn_sweepETH": "121044", + "V4Router_ExactOutputSingle_nativeOut": "116236" } \ No newline at end of file diff --git a/src/PositionManager.sol b/src/PositionManager.sol index 0a67a9d53..428fb4b71 100644 --- a/src/PositionManager.sol +++ b/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); } diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 558e85b7f..2965e5743 100644 --- a/src/base/Notifier.sol +++ b/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); } diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index d6cca9083..3eefba3c5 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -36,6 +36,7 @@ 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 @@ -43,6 +44,7 @@ interface INotifier { /// @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 diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol index fbdfdd5c2..53a2efbee 100644 --- a/src/interfaces/IPositionManager.sol +++ b/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 diff --git a/test/mocks/MockReenterHook.sol b/test/mocks/MockReenterHook.sol new file mode 100644 index 000000000..e6d6f2db5 --- /dev/null +++ b/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; + } +} diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index 40740084f..253afd96f 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/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 {PosmTestSetup} from "../shared/PosmTestSetup.sol"; import {MockSubscriber} from "../mocks/MockSubscriber.sol"; import {ISubscriber} from "../../src/interfaces/ISubscriber.sol"; @@ -19,6 +20,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 { using PoolIdLibrary for PoolKey; @@ -30,10 +32,13 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup { MockReturnDataSubscriber badSubscriber; PositionConfig config; MockRevertSubscriber revertSubscriber; + MockReenterHook reenterHook; address alice = makeAddr("ALICE"); address bob = makeAddr("BOB"); + PositionConfig reenterConfig; + function setUp() public { deployFreshManagerAndRouters(); deployMintAndApprove2Currencies(); @@ -48,6 +53,17 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup { 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 } @@ -646,4 +662,69 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup { 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); + } }