diff --git a/test/mocks/MockReentrantSubscriber.sol b/test/mocks/MockReentrantSubscriber.sol new file mode 100644 index 00000000..ae9dae7d --- /dev/null +++ b/test/mocks/MockReentrantSubscriber.sol @@ -0,0 +1,65 @@ +// 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"; + +/// @notice A subscriber contract that ingests updates from the v4 position manager +contract MockReentrantSubscriber 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, BalanceDelta) external onlyByPosm { + if (actions.length != 0) { + posm.modifyLiquiditiesWithoutUnlock(actions, params); + } + } + + function notifyTransfer(uint256 tokenId, 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.modifyLiquidities.t.sol b/test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol similarity index 53% rename from test/position-managers/PositionManager.modifyLiquidities.t.sol rename to test/position-managers/PositionManager.modifyLiquiditiesWithoutUnlock.t.sol index 33a8fff7..a9a54337 100644 --- a/test/position-managers/PositionManager.modifyLiquidities.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"; @@ -23,6 +24,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 +35,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF uint256 alicePK; address bob; + MockReentrantSubscriber sub; PositionConfig config; function setUp() public { @@ -55,6 +58,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}); } @@ -286,17 +291,246 @@ 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 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 sample a single action + bytes memory calls = getFuzzySingleEncoded(seed, tokenId, config, 10e18, ZERO_BYTES); + + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__SubsciptionReverted.selector, + address(sub), + abi.encodeWithSelector(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(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 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 + + // 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 + 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 (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); + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__ModifyLiquidityNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(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 { + 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); + + // 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( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(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.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( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(sub)) + ) + ); + } else { + vm.expectRevert( + abi.encodeWithSelector( + INotifier.Wrap__TransferNotificationReverted.selector, + address(sub), + abi.encodeWithSelector(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; + 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.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + 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.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + 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.Wrap__FailedHookCall.selector, + address(hookModifyLiquidities), + 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); } 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); + } + } }