From 089526682e12e3d43d35fb1cf3c76ea1f240b76a Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 22:13:26 -0400 Subject: [PATCH 1/9] this name is cursed and haunted --- ...t.sol => PositionManager.modifyLiquiditiesWithoutUnlock.t.sol} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/position-managers/{PositionManager.modifyLiquidities.t.sol => PositionManager.modifyLiquiditiesWithoutUnlock.t.sol} (100%) diff --git a/test/position-managers/PositionManager.modifyLiquidities.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol similarity index 100% rename from test/position-managers/PositionManager.modifyLiquidities.t.sol rename to test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol From 5d9ccb0d6c6ce1f08b0063fe55f78cdba6d46b3f Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 22:56:58 -0400 Subject: [PATCH 2/9] wip --- test/mocks/MockReentrantSubscriber.sol | 63 +++++++++++++++++++ ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 14 +++++ 2 files changed, 77 insertions(+) create mode 100644 test/mocks/MockReentrantSubscriber.sol diff --git a/test/mocks/MockReentrantSubscriber.sol b/test/mocks/MockReentrantSubscriber.sol new file mode 100644 index 00000000..e1bc5181 --- /dev/null +++ b/test/mocks/MockReentrantSubscriber.sol @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.20; + +import {ISubscriber} from "../../src/interfaces/ISubscriber.sol"; +import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; +import {PositionManager} from "../../src/PositionManager.sol"; + +/// @notice A subscriber contract that ingests updates from the v4 position manager +contract MockSubscriber is ISubscriber { + PositionManager posm; + + bytes actions; + bytes[] params; + + error NotAuthorizedNotifer(address sender); + + error NotImplemented(); + + constructor(PositionManager _posm) { + posm = _posm; + } + + modifier onlyByPosm() { + if (msg.sender != address(posm)) revert NotAuthorizedNotifer(msg.sender); + _; + } + + function notifySubscribe(uint256, PositionConfig memory, bytes memory data) external onlyByPosm { + if (data.length != 0) { + (bytes memory _actions, bytes[] memory _params) = abi.decode(data, (bytes, bytes[])); + posm.modifyLiquiditiesWithoutUnlock(_actions, _params); + } + } + + function notifyUnsubscribe(uint256, PositionConfig memory, bytes memory data) external onlyByPosm { + if (data.length != 0) { + (bytes memory _actions, bytes[] memory _params) = abi.decode(data, (bytes, bytes[])); + posm.modifyLiquiditiesWithoutUnlock(_actions, _params); + } + } + + function notifyModifyLiquidity(uint256, PositionConfig memory, int256) external onlyByPosm { + if (actions.length != 0) { + posm.modifyLiquiditiesWithoutUnlock(actions, params); + } + } + + function notifyTransfer(uint256, address, address) external onlyByPosm { + if (actions.length != 0) { + posm.modifyLiquiditiesWithoutUnlock(actions, params); + } + } + + function setActionsAndParams(bytes memory _actions, bytes[] memory _params) external { + actions = _actions; + params = _params; + } + + function clearActionsAndParams() external { + actions = ""; + params = new bytes[](0); + } +} diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index 6244c456..35eda930 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -259,6 +259,20 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF swap(key, true, -1e18, calls); } + + /// @dev calling modifyLiquiditiesWithoutUnlock without a lock will revert + function test_modifyLiquiditiesWithoutUnlock_revert() public { + bytes memory calls = getMintEncoded(config, 10e18, address(this), ZERO_BYTES); + (bytes memory actions, bytes[] memory params) = abi.decode(calls, (bytes, bytes[])); + vm.expectRevert(IPoolManager.ManagerLocked.selector); + lpm.modifyLiquiditiesWithoutUnlock(actions, params); + } + + /// @dev subscribers cannot re-enter posm + function test_subscriber_reenter_revert() public { + + } + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in beforeRemoveLiquidity function test_hook_increaseLiquidity_reenter_revert() public { uint256 initialLiquidity = 100e18; From c068a08210e2eb953acbce018b5b4e2a9fabdac0 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 23:38:18 -0400 Subject: [PATCH 3/9] test subscribe, unsubscribe attempting to modifyLiquiditiesWithoutUnlock --- test/mocks/MockReentrantSubscriber.sol | 2 +- ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 40 +++++++++++++++++-- test/shared/LiquidityOperations.sol | 20 ++++++++++ 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/test/mocks/MockReentrantSubscriber.sol b/test/mocks/MockReentrantSubscriber.sol index e1bc5181..5cb923f6 100644 --- a/test/mocks/MockReentrantSubscriber.sol +++ b/test/mocks/MockReentrantSubscriber.sol @@ -6,7 +6,7 @@ import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; import {PositionManager} from "../../src/PositionManager.sol"; /// @notice A subscriber contract that ingests updates from the v4 position manager -contract MockSubscriber is ISubscriber { +contract MockReentrantSubscriber is ISubscriber { PositionManager posm; bytes actions; diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index 35eda930..6c0df74c 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -23,6 +23,7 @@ import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; import {Planner, Plan} from "../shared/Planner.sol"; import {PosmTestSetup} from "../shared/PosmTestSetup.sol"; +import {MockReentrantSubscriber} from "../mocks/MockReentrantSubscriber.sol"; contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityFuzzers { using StateLibrary for IPoolManager; @@ -33,6 +34,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF uint256 alicePK; address bob; + MockReentrantSubscriber sub; PositionConfig config; function setUp() public { @@ -55,6 +57,8 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF (key, poolId) = initPool(currency0, currency1, IHooks(hookModifyLiquidities), 3000, SQRT_PRICE_1_1, ZERO_BYTES); + sub = new MockReentrantSubscriber(lpm); + config = PositionConfig({poolKey: key, tickLower: -60, tickUpper: 60}); } @@ -259,7 +263,6 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF swap(key, true, -1e18, calls); } - /// @dev calling modifyLiquiditiesWithoutUnlock without a lock will revert function test_modifyLiquiditiesWithoutUnlock_revert() public { bytes memory calls = getMintEncoded(config, 10e18, address(this), ZERO_BYTES); @@ -268,9 +271,38 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF lpm.modifyLiquiditiesWithoutUnlock(actions, params); } - /// @dev subscribers cannot re-enter posm - function test_subscriber_reenter_revert() public { - + /// @dev subscribers cannot re-enter posm on-subscribe since PM is not unlocked + function test_fuzz_subscriber_subscribe_reenter_revert(uint256 seed) public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, address(this), ZERO_BYTES); + + // approve the subscriber to modify liquidity + lpm.approve(address(sub), tokenId); + + // randomly samples a single action + bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); + + vm.expectRevert(IPoolManager.ManagerLocked.selector); + lpm.subscribe(tokenId, config, address(sub), calls); + } + + /// @dev subscribers cannot re-enter posm on-unsubscribe since PM is not unlocked + function test_fuzz_subscriber_unsubscribe_reenter_revert(uint256 seed) public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, address(this), ZERO_BYTES); + + // approve the subscriber to modify liquidity + lpm.approve(address(sub), tokenId); + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + // randomly samples a single action + bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); + lpm.unsubscribe(tokenId, config, calls); + + // subscriber did not modify liquidity + assertEq(lpm.ownerOf(tokenId), address(this)); // owner still owns the position + assertEq(lpm.nextTokenId(), tokenId + 1); // no new token minted + assertEq(lpm.getPositionLiquidity(tokenId, config), 100e18); // liquidity unchanged } /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in beforeRemoveLiquidity diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol index 65ed085d..66f47114 100644 --- a/test/shared/LiquidityOperations.sol +++ b/test/shared/LiquidityOperations.sol @@ -196,4 +196,24 @@ abstract contract LiquidityOperations is CommonBase { // Close needed on burn in case there is liquidity left in the position. return planner.finalizeModifyLiquidityWithClose(config.poolKey); } + + function getFuzzySingleEncoded( + uint256 seed, + uint256 tokenId, + PositionConfig memory config, + uint256 liquidityChange, + bytes memory hookData + ) internal view returns (bytes memory) { + if (seed % 5 == 0) { + return getMintEncoded(config, liquidityChange, address(this), hookData); + } else if (seed % 5 == 1) { + return getIncreaseEncoded(tokenId, config, liquidityChange, hookData); + } else if (seed % 5 == 2) { + return getDecreaseEncoded(tokenId, config, liquidityChange, hookData); + } else if (seed % 5 == 3) { + return getCollectEncoded(tokenId, config, hookData); + } else if (seed % 5 == 4) { + return getBurnEncoded(tokenId, config, hookData); + } + } } From 26542a04cfd92a62ae3133e5e8584a387536a5e4 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 23:55:35 -0400 Subject: [PATCH 4/9] test hook reentrancy reverts --- ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 73 +++++++++++++++++-- test/shared/HookModifyLiquidities.sol | 29 +++++++- test/shared/HookSavesDelta.sol | 4 +- 3 files changed, 98 insertions(+), 8 deletions(-) diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index 6c0df74c..c2b7d9ce 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -305,17 +305,80 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF assertEq(lpm.getPositionLiquidity(tokenId, config), 100e18); // liquidity unchanged } + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in beforeAddLiquidity + function test_fuzz_hook_beforeAddLiquidity_reenter_revert(uint256 seed) public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + uint256 liquidityToChange = 10e18; + + // a random action be provided as hookData, so beforeAddLiquidity will attempt to modifyLiquidity + bytes memory hookCall = getFuzzySingleEncoded(seed, tokenId, config, liquidityToChange, ZERO_BYTES); + bytes memory calls = getIncreaseEncoded(tokenId, config, liquidityToChange, hookCall); + + // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + ) + ); + lpm.modifyLiquidities(calls, _deadline); + } + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in beforeRemoveLiquidity - function test_hook_increaseLiquidity_reenter_revert() public { + function test_fuzz_hook_beforeRemoveLiquidity_reenter_revert(uint256 seed) public { uint256 initialLiquidity = 100e18; uint256 tokenId = lpm.nextTokenId(); mint(config, initialLiquidity, address(this), ZERO_BYTES); - uint256 newLiquidity = 10e18; + uint256 liquidityToChange = 10e18; + + // a random action be provided as hookData, so beforeAddLiquidity will attempt to modifyLiquidity + bytes memory hookCall = getFuzzySingleEncoded(seed, tokenId, config, liquidityToChange, ZERO_BYTES); + bytes memory calls = getDecreaseEncoded(tokenId, config, liquidityToChange, hookCall); + + // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + ) + ); + lpm.modifyLiquidities(calls, _deadline); + } + + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in afterAddLiquidity + function test_fuzz_hook_afterAddLiquidity_reenter_revert(uint256 seed) public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + uint256 liquidityToChange = 10e18; + + // a random action be provided as hookData, so afterAddLiquidity will attempt to modifyLiquidity + bytes memory hookCall = getFuzzySingleEncoded(seed, tokenId, config, liquidityToChange, ZERO_BYTES); + bytes memory calls = getIncreaseEncoded(tokenId, config, liquidityToChange, hookCall); + + // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + ) + ); + lpm.modifyLiquidities(calls, _deadline); + } + + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in afterRemoveLiquidity + function test_fuzz_hook_afterRemoveLiquidity_reenter_revert(uint256 seed) public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + uint256 liquidityToChange = 10e18; - // to be provided as hookData, so beforeAddLiquidity attempts to increase liquidity - bytes memory hookCall = getIncreaseEncoded(tokenId, config, newLiquidity, ZERO_BYTES); - bytes memory calls = getIncreaseEncoded(tokenId, config, newLiquidity, hookCall); + // a random action be provided as hookData, so afterAddLiquidity will attempt to modifyLiquidity + bytes memory hookCall = getFuzzySingleEncoded(seed, tokenId, config, liquidityToChange, ZERO_BYTES); + bytes memory calls = getDecreaseEncoded(tokenId, config, liquidityToChange, hookCall); // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert( diff --git a/test/shared/HookModifyLiquidities.sol b/test/shared/HookModifyLiquidities.sol index 2e9b5ecf..01efcc58 100644 --- a/test/shared/HookModifyLiquidities.sol +++ b/test/shared/HookModifyLiquidities.sol @@ -8,7 +8,6 @@ import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {BeforeSwapDelta, BeforeSwapDeltaLibrary} from "@uniswap/v4-core/src/types/BeforeSwapDelta.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; import {BalanceDelta, BalanceDeltaLibrary} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; - import {HookSavesDelta} from "./HookSavesDelta.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; @@ -65,6 +64,34 @@ contract HookModifyLiquidities is HookSavesDelta { return this.beforeRemoveLiquidity.selector; } + function afterAddLiquidity( + address sender, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata liqParams, + BalanceDelta delta, + bytes calldata hookData + ) public override returns (bytes4 selector, BalanceDelta returnDelta) { + if (hookData.length > 0) { + (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); + posm.modifyLiquiditiesWithoutUnlock(actions, params); + } + (selector, returnDelta) = super.afterAddLiquidity(sender, key, liqParams, delta, hookData); + } + + function afterRemoveLiquidity( + address sender, + PoolKey calldata key, + IPoolManager.ModifyLiquidityParams calldata liqParams, + BalanceDelta delta, + bytes calldata hookData + ) public override returns (bytes4 selector, BalanceDelta returnDelta) { + if (hookData.length > 0) { + (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); + posm.modifyLiquiditiesWithoutUnlock(actions, params); + } + (selector, returnDelta) = super.afterRemoveLiquidity(sender, key, liqParams, delta, hookData); + } + function approvePosmCurrency(Currency currency) internal { // Because POSM uses permit2, we must execute 2 permits/approvals. // 1. First, the caller must approve permit2 on the token. diff --git a/test/shared/HookSavesDelta.sol b/test/shared/HookSavesDelta.sol index 8ff86ac1..6a1055f9 100644 --- a/test/shared/HookSavesDelta.sol +++ b/test/shared/HookSavesDelta.sol @@ -17,7 +17,7 @@ contract HookSavesDelta is BaseTestHooks { IPoolManager.ModifyLiquidityParams calldata, /* params **/ BalanceDelta delta, bytes calldata /* hookData **/ - ) external override returns (bytes4, BalanceDelta) { + ) public virtual override returns (bytes4, BalanceDelta) { _storeDelta(delta); return (this.afterAddLiquidity.selector, BalanceDeltaLibrary.ZERO_DELTA); } @@ -28,7 +28,7 @@ contract HookSavesDelta is BaseTestHooks { IPoolManager.ModifyLiquidityParams calldata, /* params **/ BalanceDelta delta, bytes calldata /* hookData **/ - ) external override returns (bytes4, BalanceDelta) { + ) public virtual override returns (bytes4, BalanceDelta) { _storeDelta(delta); return (this.afterRemoveLiquidity.selector, BalanceDeltaLibrary.ZERO_DELTA); } From a6265e3744ec9b844e67fd5816865e10bed0b340 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Mon, 5 Aug 2024 01:55:11 -0400 Subject: [PATCH 5/9] test subscriber entrancy with modifyLiquiditiesWithoutUnlock --- ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index c2b7d9ce..0410b8f2 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -279,7 +279,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // approve the subscriber to modify liquidity lpm.approve(address(sub), tokenId); - // randomly samples a single action + // randomly sample a single action bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); vm.expectRevert(IPoolManager.ManagerLocked.selector); @@ -295,7 +295,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF lpm.approve(address(sub), tokenId); lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); - // randomly samples a single action + // randomly sample a single action bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); lpm.unsubscribe(tokenId, config, calls); @@ -305,6 +305,53 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF assertEq(lpm.getPositionLiquidity(tokenId, config), 100e18); // liquidity unchanged } + /// @dev subscribers cannot re-enter posm on-notifyModifyLiquidity because of no reentrancy guards + function test_fuzz_subscriber_notifyModifyLiquidity_reenter_revert(uint256 seed0, uint256 seed1) public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, address(this), ZERO_BYTES); + + // approve the subscriber to modify liquidity + lpm.approve(address(sub), tokenId); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + // randomly sample a single action + bytes memory action = getFuzzySingleEncoded(seed0, tokenId, config, 10e18, ZERO_BYTES); + (bytes memory actions, bytes[] memory params) = abi.decode(action, (bytes, bytes[])); + sub.setActionsAndParams(actions, params); + + // modify the token with an increase + bytes memory calls = getIncreaseEncoded(tokenId, config, 10e18, ZERO_BYTES); + + // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock + vm.expectRevert(ReentrancyLock.ContractLocked.selector); + lpm.modifyLiquidities(calls, _deadline); + } + + /// @dev subscribers cannot re-enter posm on-notifyTransfer because position manager is not unlocked + function test_fuzz_subscriber_notifyTransfer_reenter_revert(uint256 seed) public { + seed = 4; + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, address(this), ZERO_BYTES); + + // approve the subscriber to modify liquidity + lpm.approve(address(sub), tokenId); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + // randomly sample a single action + bytes memory action = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); + (bytes memory actions, bytes[] memory params) = abi.decode(action, (bytes, bytes[])); + sub.setActionsAndParams(actions, params); + + console2.log(lpm.getApproved(tokenId)); + console2.log(address(sub)); + + // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock + vm.expectRevert(IPoolManager.ManagerLocked.selector); + lpm.transferFrom(address(this), alice, tokenId); + } + /// @dev hook cannot re-enter modifyLiquiditiesWithoutUnlock in beforeAddLiquidity function test_fuzz_hook_beforeAddLiquidity_reenter_revert(uint256 seed) public { uint256 initialLiquidity = 100e18; From 5b9667197ef3eaa353c7ce9ff084dd2b9460ee3d Mon Sep 17 00:00:00 2001 From: saucepoint Date: Mon, 5 Aug 2024 02:25:33 -0400 Subject: [PATCH 6/9] testing different subscriber behavior --- test/mocks/MockReentrantSubscriber.sol | 2 +- ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 43 ++++++++++++++++--- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/test/mocks/MockReentrantSubscriber.sol b/test/mocks/MockReentrantSubscriber.sol index 5cb923f6..98390562 100644 --- a/test/mocks/MockReentrantSubscriber.sol +++ b/test/mocks/MockReentrantSubscriber.sol @@ -45,7 +45,7 @@ contract MockReentrantSubscriber is ISubscriber { } } - function notifyTransfer(uint256, address, address) external onlyByPosm { + function notifyTransfer(uint256 tokenId, address, address) external onlyByPosm { if (actions.length != 0) { posm.modifyLiquiditiesWithoutUnlock(actions, params); } diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index 0410b8f2..b5762882 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -320,8 +320,15 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF (bytes memory actions, bytes[] memory params) = abi.decode(action, (bytes, bytes[])); sub.setActionsAndParams(actions, params); - // modify the token with an increase - bytes memory calls = getIncreaseEncoded(tokenId, config, 10e18, ZERO_BYTES); + // modify the token (dont mint) + bytes memory calls; + if (seed1 % 3 == 0) { + calls = getIncreaseEncoded(tokenId, config, 10e18, ZERO_BYTES); + } else if (seed1 % 3 == 1) { + calls = getDecreaseEncoded(tokenId, config, 10e18, ZERO_BYTES); + } else { + calls = getBurnEncoded(tokenId, config, ZERO_BYTES); + } // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert(ReentrancyLock.ContractLocked.selector); @@ -330,7 +337,6 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF /// @dev subscribers cannot re-enter posm on-notifyTransfer because position manager is not unlocked function test_fuzz_subscriber_notifyTransfer_reenter_revert(uint256 seed) public { - seed = 4; uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, address(this), ZERO_BYTES); @@ -344,11 +350,38 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF (bytes memory actions, bytes[] memory params) = abi.decode(action, (bytes, bytes[])); sub.setActionsAndParams(actions, params); - console2.log(lpm.getApproved(tokenId)); - console2.log(address(sub)); + // by setting the subscriber as the recipient of the ERC721 transfer, it will + // have permission to modify its own liquidity. it will still revert + // because the pool manager is not unlocked // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert(IPoolManager.ManagerLocked.selector); + lpm.transferFrom(address(this), address(sub), tokenId); + } + + /// @dev subscribers cannot re-enter posm on-notifyTransfer because it does not have approval anymore + function test_fuzz_subscriber_notifyTransfer_reenter_revertNotApproved(uint256 seed) public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, address(this), ZERO_BYTES); + + // approve the subscriber to modify liquidity + lpm.approve(address(sub), tokenId); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + // randomly sample a single action + bytes memory action = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); + (bytes memory actions, bytes[] memory params) = abi.decode(action, (bytes, bytes[])); + sub.setActionsAndParams(actions, params); + + uint256 actionNumber = uint256(uint8(actions[0])); + if (actionNumber == Actions.DECREASE_LIQUIDITY || actionNumber == Actions.BURN_POSITION) { + // revert because the subscriber loses approval + // ERC721.transferFrom happens before notifyTransfer and resets the approval + vm.expectRevert(abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(sub))); + } else { + vm.expectRevert(IPoolManager.ManagerLocked.selector); + } lpm.transferFrom(address(this), alice, tokenId); } From c31e24c44c6960d029f0f310480e57d2cafa1b63 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Mon, 5 Aug 2024 11:58:56 -0400 Subject: [PATCH 7/9] update revert handlers --- ...nager.modifyLiquiditiesWithoutUnlock.t.sol | 42 ++++++++++++++++--- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index b5762882..ea458b18 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -15,6 +15,7 @@ import {Position} from "@uniswap/v4-core/src/libraries/Position.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; +import {INotifier} from "../../src/interfaces/INotifier.sol"; import {ReentrancyLock} from "../../src/base/ReentrancyLock.sol"; import {Actions} from "../../src/libraries/Actions.sol"; import {PositionManager} from "../../src/PositionManager.sol"; @@ -282,7 +283,13 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // randomly sample a single action bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); - vm.expectRevert(IPoolManager.ManagerLocked.selector); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__SubsciptionReverted.selector, + address(sub), + abi.encodeWithSelector(IPoolManager.ManagerLocked.selector) + ) + ); lpm.subscribe(tokenId, config, address(sub), calls); } @@ -331,7 +338,14 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF } // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock - vm.expectRevert(ReentrancyLock.ContractLocked.selector); + // vm.expectRevert(ReentrancyLock.ContractLocked.selector); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__ModifyLiquidityNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + ) + ); lpm.modifyLiquidities(calls, _deadline); } @@ -355,7 +369,13 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // because the pool manager is not unlocked // should revert because subscriber is re-entering modifyLiquiditiesWithoutUnlock - vm.expectRevert(IPoolManager.ManagerLocked.selector); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(IPoolManager.ManagerLocked.selector) + ) + ); lpm.transferFrom(address(this), address(sub), tokenId); } @@ -378,9 +398,21 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF if (actionNumber == Actions.DECREASE_LIQUIDITY || actionNumber == Actions.BURN_POSITION) { // revert because the subscriber loses approval // ERC721.transferFrom happens before notifyTransfer and resets the approval - vm.expectRevert(abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(sub))); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(sub)) + ) + ); } else { - vm.expectRevert(IPoolManager.ManagerLocked.selector); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(IPoolManager.ManagerLocked.selector) + ) + ); } lpm.transferFrom(address(this), alice, tokenId); } From 39b80b04be569b205421fe0c787f62f05f4a2eb2 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Mon, 5 Aug 2024 15:23:35 -0400 Subject: [PATCH 8/9] update revert signatures --- ...itionManager.modifyLiquiditiesWithoutUnlock.t.sol | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index c70b1a98..edfc1860 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -435,7 +435,9 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert( abi.encodeWithSelector( - Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + Hooks.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) ) ); lpm.modifyLiquidities(calls, _deadline); @@ -456,7 +458,9 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert( abi.encodeWithSelector( - Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + Hooks.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) ) ); lpm.modifyLiquidities(calls, _deadline); @@ -477,7 +481,9 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF // should revert because hook is re-entering modifyLiquiditiesWithoutUnlock vm.expectRevert( abi.encodeWithSelector( - Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + Hooks.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) ) ); lpm.modifyLiquidities(calls, _deadline); From 3f6cbe07acf2dcbdc606022f5703c755a0efac25 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 21 Aug 2024 11:27:00 -0400 Subject: [PATCH 9/9] PR feedback --- test/mocks/MockReentrantSubscriber.sol | 4 +++- ...sitionManager.modifyLiquiditiesWithoutUnlock.t.sol | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/mocks/MockReentrantSubscriber.sol b/test/mocks/MockReentrantSubscriber.sol index 98390562..ae9dae7d 100644 --- a/test/mocks/MockReentrantSubscriber.sol +++ b/test/mocks/MockReentrantSubscriber.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.20; +import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; + import {ISubscriber} from "../../src/interfaces/ISubscriber.sol"; import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; import {PositionManager} from "../../src/PositionManager.sol"; @@ -39,7 +41,7 @@ contract MockReentrantSubscriber is ISubscriber { } } - function notifyModifyLiquidity(uint256, PositionConfig memory, int256) external onlyByPosm { + function notifyModifyLiquidity(uint256, PositionConfig memory, int256, BalanceDelta) external onlyByPosm { if (actions.length != 0) { posm.modifyLiquiditiesWithoutUnlock(actions, params); } diff --git a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index d1f2416b..a9a54337 100644 --- a/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol +++ b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol @@ -321,7 +321,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF } /// @dev subscribers cannot re-enter posm on-unsubscribe since PM is not unlocked - function test_fuzz_subscriber_unsubscribe_reenter_revert(uint256 seed) public { + function test_fuzz_subscriber_unsubscribe_reenter(uint256 seed) public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, address(this), ZERO_BYTES); @@ -337,6 +337,10 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF assertEq(lpm.ownerOf(tokenId), address(this)); // owner still owns the position assertEq(lpm.nextTokenId(), tokenId + 1); // no new token minted assertEq(lpm.getPositionLiquidity(tokenId, config), 100e18); // liquidity unchanged + + // token was unsubscribed + assertEq(address(lpm.subscriber(tokenId)), address(0)); + assertEq(lpm.hasSubscriber(tokenId), false); } /// @dev subscribers cannot re-enter posm on-notifyModifyLiquidity because of no reentrancy guards @@ -422,7 +426,10 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF sub.setActionsAndParams(actions, params); uint256 actionNumber = uint256(uint8(actions[0])); - if (actionNumber == Actions.DECREASE_LIQUIDITY || actionNumber == Actions.BURN_POSITION) { + if ( + actionNumber == Actions.INCREASE_LIQUIDITY || actionNumber == Actions.DECREASE_LIQUIDITY + || actionNumber == Actions.BURN_POSITION + ) { // revert because the subscriber loses approval // ERC721.transferFrom happens before notifyTransfer and resets the approval vm.expectRevert(