From f034243b4b4d3e0998c4f335a8243d6a72c423b6 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 11:37:39 -0400 Subject: [PATCH 1/5] test hook which modifiesLiquidities in beforeSwap --- src/PositionManager.sol | 4 + src/interfaces/IPositionManager.sol | 2 +- .../PositionManager.modifiyLiquidities.t.sol | 98 +++++++++++++++++++ test/shared/HookModifyLiquidities.sol | 53 ++++++++++ test/shared/PosmTestSetup.sol | 17 ++++ 5 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 test/position-managers/PositionManager.modifiyLiquidities.t.sol create mode 100644 test/shared/HookModifyLiquidities.sol diff --git a/src/PositionManager.sol b/src/PositionManager.sol index f2d91a06..dd13cd14 100644 --- a/src/PositionManager.sol +++ b/src/PositionManager.sol @@ -30,6 +30,8 @@ import {INotifier} from "./interfaces/INotifier.sol"; import {Permit2Forwarder} from "./base/Permit2Forwarder.sol"; import {SlippageCheckLibrary} from "./libraries/SlippageCheck.sol"; +import "forge-std/console2.sol"; + contract PositionManager is IPositionManager, ERC721Permit, @@ -105,7 +107,9 @@ contract PositionManager is } function modifyLiquidities(bytes calldata actions, bytes[] calldata params) external payable isNotLocked { + console2.log("GRAHH"); _executeActions(actions, params); + console2.log("HUHHH"); } /// @inheritdoc INotifier diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol index 46772e6d..ba8e003f 100644 --- a/src/interfaces/IPositionManager.sol +++ b/src/interfaces/IPositionManager.sol @@ -16,7 +16,7 @@ interface IPositionManager is INotifier { /// @param deadline is the deadline for the batched actions to be executed function unlockAndModifyLiquidities(bytes calldata payload, uint256 deadline) external payable; - function modifyLiquidities(bytes calldata actions, bytes[] calldata params) external payable; + function modifyLiquidities(bytes memory actions, bytes[] memory params) external payable; function nextTokenId() external view returns (uint256); diff --git a/test/position-managers/PositionManager.modifiyLiquidities.t.sol b/test/position-managers/PositionManager.modifiyLiquidities.t.sol new file mode 100644 index 00000000..995f6751 --- /dev/null +++ b/test/position-managers/PositionManager.modifiyLiquidities.t.sol @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import {PoolManager} from "@uniswap/v4-core/src/PoolManager.sol"; +import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; +import {IHooks} from "@uniswap/v4-core/src/interfaces/IHooks.sol"; +import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; +import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; +import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; +import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {LiquidityAmounts} from "@uniswap/v4-core/test/utils/LiquidityAmounts.sol"; +import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; +import {FixedPointMathLib} from "solmate/src/utils/FixedPointMathLib.sol"; +import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; +import {LPFeeLibrary} from "@uniswap/v4-core/src/libraries/LPFeeLibrary.sol"; +import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"; +import {Position} from "@uniswap/v4-core/src/libraries/Position.sol"; +import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; + +import {IERC20} from "forge-std/interfaces/IERC20.sol"; + +import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; +import {Actions} from "../../src/libraries/Actions.sol"; +import {PositionManager} from "../../src/PositionManager.sol"; +import {DeltaResolver} from "../../src/base/DeltaResolver.sol"; +import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; +import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol"; +import {BaseActionsRouter} from "../../src/base/BaseActionsRouter.sol"; +import {Constants} from "../../src/libraries/Constants.sol"; + +import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; +import {Planner, Plan} from "../shared/Planner.sol"; +import {PosmTestSetup} from "../shared/PosmTestSetup.sol"; +import {ReentrantToken} from "../mocks/ReentrantToken.sol"; +import {ReentrancyLock} from "../../src/base/ReentrancyLock.sol"; + +contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityFuzzers { + using StateLibrary for IPoolManager; + using PoolIdLibrary for PoolKey; + + PoolId poolId; + address alice; + uint256 alicePK; + address bob; + + PositionConfig config; + + function setUp() public { + (alice, alicePK) = makeAddrAndKey("ALICE"); + (bob,) = makeAddrAndKey("BOB"); + + deployFreshManagerAndRouters(); + deployMintAndApprove2Currencies(); + + // Requires currency0 and currency1 to be set in base Deployers contract. + deployAndApprovePosm(manager); + + seedBalance(alice); + approvePosmFor(alice); + + // must deploy after posm + // Deploys a hook which can accesses IPositionManager.modifyLiquidities + deployPosmHookModifyLiquidities(); + seedBalance(address(hookModifyLiquidities)); + + (key, poolId) = initPool(currency0, currency1, IHooks(hookModifyLiquidities), 3000, SQRT_PRICE_1_1, ZERO_BYTES); + + config = PositionConfig({poolKey: key, tickLower: -60, tickUpper: 60}); + } + + function test_hook_increaseLiquidity() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // hook increases liquidity in beforeSwap via hookData + uint256 newLiquidity = 10e18; + bytes memory calls = getIncreaseEncoded(tokenId, config, newLiquidity, ZERO_BYTES); + + swap(key, true, -1e18, calls); + + bytes32 positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenId)); + (uint256 liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + + assertEq(liquidity, initialLiquidity + newLiquidity); + } + + function test_hook_decreaseLiquidity() public {} + function test_hook_collect() public {} + function test_hook_burn() public {} + + function test_hook_increaseLiquidity_revert() public {} + function test_hook_decreaseLiquidity_revert() public {} + function test_hook_collect_revert() public {} + function test_hook_burn_revert() public {} +} diff --git a/test/shared/HookModifyLiquidities.sol b/test/shared/HookModifyLiquidities.sol new file mode 100644 index 00000000..a94bd0d7 --- /dev/null +++ b/test/shared/HookModifyLiquidities.sol @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"; + +import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; +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 {BaseTestHooks} from "@uniswap/v4-core/src/test/BaseTestHooks.sol"; +import {IERC20} from "forge-std/interfaces/IERC20.sol"; + +import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; + +import "forge-std/console2.sol"; + +/// @notice This contract is NOT a production use contract. It is meant to be used in testing to verify the delta amounts against changes in a user's balance. +/// @dev a hook that can modify liquidity in beforeSwap +contract HookModifyLiquidities is BaseTestHooks { + IPositionManager posm; + IAllowanceTransfer permit2; + + function setAddresses(IPositionManager _posm, IAllowanceTransfer _permit2) external { + posm = _posm; + permit2 = _permit2; + } + + function beforeSwap( + address, /* sender **/ + PoolKey calldata key, /* key **/ + IPoolManager.SwapParams calldata, /* params **/ + bytes calldata hookData + ) external override returns (bytes4, BeforeSwapDelta, uint24) { + approvePosmCurrency(key.currency0); + approvePosmCurrency(key.currency1); + + (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); + console2.log("WAAAA"); + posm.modifyLiquidities(actions, params); + console2.log(address(posm)); + return (BaseTestHooks.beforeSwap.selector, BeforeSwapDeltaLibrary.ZERO_DELTA, 0); + } + + 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. + IERC20(Currency.unwrap(currency)).approve(address(permit2), type(uint256).max); + // 2. Then, the caller must approve POSM as a spender of permit2. TODO: This could also be a signature. + permit2.approve(Currency.unwrap(currency), address(posm), type(uint160).max, type(uint48).max); + } +} diff --git a/test/shared/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index 3ae10590..ea444147 100644 --- a/test/shared/PosmTestSetup.sol +++ b/test/shared/PosmTestSetup.sol @@ -14,8 +14,11 @@ import {LiquidityOperations} from "./LiquidityOperations.sol"; import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"; import {DeployPermit2} from "permit2/test/utils/DeployPermit2.sol"; import {HookSavesDelta} from "./HookSavesDelta.sol"; +import {HookModifyLiquidities} from "./HookModifyLiquidities.sol"; import {ERC721PermitHashLibrary} from "../../src/libraries/ERC721PermitHash.sol"; +import "forge-std/console2.sol"; + /// @notice A shared test contract that wraps the v4-core deployers contract and exposes basic liquidity operations on posm. contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; @@ -24,12 +27,26 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { HookSavesDelta hook; address hookAddr = address(uint160(Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)); + HookModifyLiquidities hookModifyLiquidities; + address hookModifyLiquiditiesAddr = address(uint160(Hooks.BEFORE_SWAP_FLAG)); + function deployPosmHookSavesDelta() public { HookSavesDelta impl = new HookSavesDelta(); vm.etch(hookAddr, address(impl).code); hook = HookSavesDelta(hookAddr); } + function deployPosmHookModifyLiquidities() public { + HookModifyLiquidities impl = new HookModifyLiquidities(); + vm.etch(hookModifyLiquiditiesAddr, address(impl).code); + hookModifyLiquidities = HookModifyLiquidities(hookModifyLiquiditiesAddr); + + console2.log(address(lpm)); + + // set posm address since constructor args are not easily copied by vm.etch + hookModifyLiquidities.setAddresses(lpm, permit2); + } + function deployAndApprovePosm(IPoolManager poolManager) public { deployPosm(poolManager); approvePosm(); From 15588ce0dd93d6e835ba1436cb9c54f20e27f720 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 15:11:59 -0400 Subject: [PATCH 2/5] test hook modifying liquidity --- src/PositionManager.sol | 4 - .../PositionManager.modifiyLiquidities.t.sol | 171 +++++++++++++++++- test/position-managers/PositionManager.t.sol | 2 +- test/shared/HookModifyLiquidities.sol | 10 +- test/shared/PosmTestSetup.sol | 7 +- 5 files changed, 170 insertions(+), 24 deletions(-) diff --git a/src/PositionManager.sol b/src/PositionManager.sol index dd13cd14..f2d91a06 100644 --- a/src/PositionManager.sol +++ b/src/PositionManager.sol @@ -30,8 +30,6 @@ import {INotifier} from "./interfaces/INotifier.sol"; import {Permit2Forwarder} from "./base/Permit2Forwarder.sol"; import {SlippageCheckLibrary} from "./libraries/SlippageCheck.sol"; -import "forge-std/console2.sol"; - contract PositionManager is IPositionManager, ERC721Permit, @@ -107,9 +105,7 @@ contract PositionManager is } function modifyLiquidities(bytes calldata actions, bytes[] calldata params) external payable isNotLocked { - console2.log("GRAHH"); _executeActions(actions, params); - console2.log("HUHHH"); } /// @inheritdoc INotifier diff --git a/test/position-managers/PositionManager.modifiyLiquidities.t.sol b/test/position-managers/PositionManager.modifiyLiquidities.t.sol index 995f6751..ceb2175f 100644 --- a/test/position-managers/PositionManager.modifiyLiquidities.t.sol +++ b/test/position-managers/PositionManager.modifiyLiquidities.t.sol @@ -5,6 +5,7 @@ import "forge-std/Test.sol"; import {PoolManager} from "@uniswap/v4-core/src/PoolManager.sol"; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {IHooks} from "@uniswap/v4-core/src/interfaces/IHooks.sol"; +import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol"; import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; @@ -69,6 +70,7 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF config = PositionConfig({poolKey: key, tickLower: -60, tickUpper: 60}); } + /// @dev increasing liquidity without approval is allowable function test_hook_increaseLiquidity() public { uint256 initialLiquidity = 100e18; uint256 tokenId = lpm.nextTokenId(); @@ -87,12 +89,167 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF assertEq(liquidity, initialLiquidity + newLiquidity); } - function test_hook_decreaseLiquidity() public {} - function test_hook_collect() public {} - function test_hook_burn() public {} + /// @dev hook can decrease liquidity with approval + function test_hook_decreaseLiquidity() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // approve the hook for decreasing liquidity + lpm.approve(address(hookModifyLiquidities), tokenId); + + // hook decreases liquidity in beforeSwap via hookData + uint256 liquidityToDecrease = 10e18; + bytes memory calls = getDecreaseEncoded(tokenId, config, liquidityToDecrease, ZERO_BYTES); + + swap(key, true, -1e18, calls); + + bytes32 positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenId)); + (uint256 liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + + assertEq(liquidity, initialLiquidity - liquidityToDecrease); + } + + /// @dev hook can collect liquidity with approval + function test_hook_collect() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); - function test_hook_increaseLiquidity_revert() public {} - function test_hook_decreaseLiquidity_revert() public {} - function test_hook_collect_revert() public {} - function test_hook_burn_revert() public {} + // approve the hook for collecting liquidity + lpm.approve(address(hookModifyLiquidities), tokenId); + + // donate to generate revenue + uint256 feeRevenue0 = 1e18; + uint256 feeRevenue1 = 0.1e18; + donateRouter.donate(config.poolKey, feeRevenue0, feeRevenue1, ZERO_BYTES); + + uint256 balance0HookBefore = currency0.balanceOf(address(hookModifyLiquidities)); + uint256 balance1HookBefore = currency1.balanceOf(address(hookModifyLiquidities)); + + // hook collects liquidity in beforeSwap via hookData + bytes memory calls = getCollectEncoded(tokenId, config, ZERO_BYTES); + swap(key, true, -1e18, calls); + + bytes32 positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenId)); + (uint256 liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + + // liquidity unchanged + assertEq(liquidity, initialLiquidity); + + // hook collected the fee revenue + assertEq(currency0.balanceOf(address(hookModifyLiquidities)), balance0HookBefore + feeRevenue0 - 1 wei); // imprecision, core is keeping 1 wei + assertEq(currency1.balanceOf(address(hookModifyLiquidities)), balance1HookBefore + feeRevenue1 - 1 wei); + } + + /// @dev hook can burn liquidity with approval + function test_hook_burn() public { + // mint some liquidity that is NOT burned in beforeSwap + mint(config, 100e18, address(this), ZERO_BYTES); + + // the position to be burned by the hook + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + // TODO: make this less jank since HookModifyLiquidites also has delta saving capabilities + // BalanceDelta mintDelta = getLastDelta(); + BalanceDelta mintDelta = hookModifyLiquidities.deltas(hookModifyLiquidities.numberDeltasReturned() - 1); + + // approve the hook for burning liquidity + lpm.approve(address(hookModifyLiquidities), tokenId); + + uint256 balance0HookBefore = currency0.balanceOf(address(hookModifyLiquidities)); + uint256 balance1HookBefore = currency1.balanceOf(address(hookModifyLiquidities)); + + // hook burns liquidity in beforeSwap via hookData + bytes memory calls = getBurnEncoded(tokenId, config, ZERO_BYTES); + swap(key, true, -1e18, calls); + + bytes32 positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenId)); + (uint256 liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + + // liquidity burned + assertEq(liquidity, 0); + // 721 will revert if the token does not exist + vm.expectRevert(); + lpm.ownerOf(tokenId); + + // hook claimed the burned liquidity + assertEq( + currency0.balanceOf(address(hookModifyLiquidities)), + balance0HookBefore + uint128(-mintDelta.amount0() - 1 wei) // imprecision since core is keeping 1 wei + ); + assertEq( + currency1.balanceOf(address(hookModifyLiquidities)), + balance1HookBefore + uint128(-mintDelta.amount1() - 1 wei) + ); + } + + // --- Revert Scenarios --- // + /// @dev Hook does not have approval so decreasingly liquidity should revert + function test_hook_decreaseLiquidity_revert() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // hook decreases liquidity in beforeSwap via hookData + uint256 liquidityToDecrease = 10e18; + bytes memory calls = getDecreaseEncoded(tokenId, config, liquidityToDecrease, ZERO_BYTES); + + // should revert because hook is not approved + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, + abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(hookModifyLiquidities)) + ) + ); + swap(key, true, -1e18, calls); + } + + /// @dev hook does not have approval so collecting liquidity should revert + function test_hook_collect_revert() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // donate to generate revenue + uint256 feeRevenue0 = 1e18; + uint256 feeRevenue1 = 0.1e18; + donateRouter.donate(config.poolKey, feeRevenue0, feeRevenue1, ZERO_BYTES); + + // hook collects liquidity in beforeSwap via hookData + bytes memory calls = getCollectEncoded(tokenId, config, ZERO_BYTES); + + // should revert because hook is not approved + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, + abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(hookModifyLiquidities)) + ) + ); + swap(key, true, -1e18, calls); + } + + /// @dev hook does not have approval so burning liquidity should revert + function test_hook_burn_revert() public { + // the position to be burned by the hook + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // hook burns liquidity in beforeSwap via hookData + bytes memory calls = getBurnEncoded(tokenId, config, ZERO_BYTES); + + // should revert because hook is not approved + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, + abi.encodeWithSelector(IPositionManager.NotApproved.selector, address(hookModifyLiquidities)) + ) + ); + swap(key, true, -1e18, calls); + } } diff --git a/test/position-managers/PositionManager.t.sol b/test/position-managers/PositionManager.t.sol index 3475017c..aacf861b 100644 --- a/test/position-managers/PositionManager.t.sol +++ b/test/position-managers/PositionManager.t.sol @@ -344,7 +344,7 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers { assertEq(currency0.balanceOfSelf(), balance0BeforeBurn + uint256(int256(deltaDecrease.amount0()))); assertEq(currency1.balanceOfSelf(), balance1BeforeBurn + uint256(uint128(deltaDecrease.amount1()))); - // OZ 721 will revert if the token does not exist + // 721 will revert if the token does not exist vm.expectRevert(); lpm.ownerOf(1); diff --git a/test/shared/HookModifyLiquidities.sol b/test/shared/HookModifyLiquidities.sol index a94bd0d7..a1e4ebe2 100644 --- a/test/shared/HookModifyLiquidities.sol +++ b/test/shared/HookModifyLiquidities.sol @@ -9,16 +9,14 @@ import {BeforeSwapDelta, BeforeSwapDeltaLibrary} from "@uniswap/v4-core/src/type import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; import {BalanceDelta, BalanceDeltaLibrary} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; -import {BaseTestHooks} from "@uniswap/v4-core/src/test/BaseTestHooks.sol"; +import {HookSavesDelta} from "./HookSavesDelta.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; -import "forge-std/console2.sol"; - /// @notice This contract is NOT a production use contract. It is meant to be used in testing to verify the delta amounts against changes in a user's balance. /// @dev a hook that can modify liquidity in beforeSwap -contract HookModifyLiquidities is BaseTestHooks { +contract HookModifyLiquidities is HookSavesDelta { IPositionManager posm; IAllowanceTransfer permit2; @@ -37,10 +35,8 @@ contract HookModifyLiquidities is BaseTestHooks { approvePosmCurrency(key.currency1); (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); - console2.log("WAAAA"); posm.modifyLiquidities(actions, params); - console2.log(address(posm)); - return (BaseTestHooks.beforeSwap.selector, BeforeSwapDeltaLibrary.ZERO_DELTA, 0); + return (this.beforeSwap.selector, BeforeSwapDeltaLibrary.ZERO_DELTA, 0); } function approvePosmCurrency(Currency currency) internal { diff --git a/test/shared/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index ea444147..00d1c114 100644 --- a/test/shared/PosmTestSetup.sol +++ b/test/shared/PosmTestSetup.sol @@ -17,8 +17,6 @@ import {HookSavesDelta} from "./HookSavesDelta.sol"; import {HookModifyLiquidities} from "./HookModifyLiquidities.sol"; import {ERC721PermitHashLibrary} from "../../src/libraries/ERC721PermitHash.sol"; -import "forge-std/console2.sol"; - /// @notice A shared test contract that wraps the v4-core deployers contract and exposes basic liquidity operations on posm. contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; @@ -28,7 +26,8 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { address hookAddr = address(uint160(Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)); HookModifyLiquidities hookModifyLiquidities; - address hookModifyLiquiditiesAddr = address(uint160(Hooks.BEFORE_SWAP_FLAG)); + address hookModifyLiquiditiesAddr = + address(uint160(Hooks.BEFORE_SWAP_FLAG | Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)); function deployPosmHookSavesDelta() public { HookSavesDelta impl = new HookSavesDelta(); @@ -41,8 +40,6 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { vm.etch(hookModifyLiquiditiesAddr, address(impl).code); hookModifyLiquidities = HookModifyLiquidities(hookModifyLiquiditiesAddr); - console2.log(address(lpm)); - // set posm address since constructor args are not easily copied by vm.etch hookModifyLiquidities.setAddresses(lpm, permit2); } From 87b9da010c08cd9d0ec458db9cbbd21ac991f685 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 15:36:21 -0400 Subject: [PATCH 3/5] minor cleanups --- src/interfaces/IPositionManager.sol | 2 +- .../PositionManager.modifiyLiquidities.t.sol | 13 ------------- test/shared/HookModifyLiquidities.sol | 2 +- test/shared/PosmTestSetup.sol | 1 + 4 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol index ba8e003f..46772e6d 100644 --- a/src/interfaces/IPositionManager.sol +++ b/src/interfaces/IPositionManager.sol @@ -16,7 +16,7 @@ interface IPositionManager is INotifier { /// @param deadline is the deadline for the batched actions to be executed function unlockAndModifyLiquidities(bytes calldata payload, uint256 deadline) external payable; - function modifyLiquidities(bytes memory actions, bytes[] memory params) external payable; + function modifyLiquidities(bytes calldata actions, bytes[] calldata params) external payable; function nextTokenId() external view returns (uint256); diff --git a/test/position-managers/PositionManager.modifiyLiquidities.t.sol b/test/position-managers/PositionManager.modifiyLiquidities.t.sol index ceb2175f..f172c1c5 100644 --- a/test/position-managers/PositionManager.modifiyLiquidities.t.sol +++ b/test/position-managers/PositionManager.modifiyLiquidities.t.sol @@ -10,31 +10,18 @@ import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; -import {LiquidityAmounts} from "@uniswap/v4-core/test/utils/LiquidityAmounts.sol"; -import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; -import {FixedPointMathLib} from "solmate/src/utils/FixedPointMathLib.sol"; import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; -import {LPFeeLibrary} from "@uniswap/v4-core/src/libraries/LPFeeLibrary.sol"; -import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"; import {Position} from "@uniswap/v4-core/src/libraries/Position.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; -import {IERC20} from "forge-std/interfaces/IERC20.sol"; - import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; import {Actions} from "../../src/libraries/Actions.sol"; import {PositionManager} from "../../src/PositionManager.sol"; -import {DeltaResolver} from "../../src/base/DeltaResolver.sol"; import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; -import {SlippageCheckLibrary} from "../../src/libraries/SlippageCheck.sol"; -import {BaseActionsRouter} from "../../src/base/BaseActionsRouter.sol"; -import {Constants} from "../../src/libraries/Constants.sol"; import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; import {Planner, Plan} from "../shared/Planner.sol"; import {PosmTestSetup} from "../shared/PosmTestSetup.sol"; -import {ReentrantToken} from "../mocks/ReentrantToken.sol"; -import {ReentrancyLock} from "../../src/base/ReentrancyLock.sol"; contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityFuzzers { using StateLibrary for IPoolManager; diff --git a/test/shared/HookModifyLiquidities.sol b/test/shared/HookModifyLiquidities.sol index a1e4ebe2..85bdb897 100644 --- a/test/shared/HookModifyLiquidities.sol +++ b/test/shared/HookModifyLiquidities.sol @@ -14,7 +14,7 @@ import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {IPositionManager} from "../../src/interfaces/IPositionManager.sol"; -/// @notice This contract is NOT a production use contract. It is meant to be used in testing to verify the delta amounts against changes in a user's balance. +/// @notice This contract is NOT a production use contract. It is meant to be used in testing to verify that external contracts can modify liquidity without a lock (IPositionManager.modifyLiquidities) /// @dev a hook that can modify liquidity in beforeSwap contract HookModifyLiquidities is HookSavesDelta { IPositionManager posm; diff --git a/test/shared/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index 00d1c114..a1febbde 100644 --- a/test/shared/PosmTestSetup.sol +++ b/test/shared/PosmTestSetup.sol @@ -35,6 +35,7 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { hook = HookSavesDelta(hookAddr); } + /// @dev deploys a special test hook where beforeSwap hookData is used to modify liquidity function deployPosmHookModifyLiquidities() public { HookModifyLiquidities impl = new HookModifyLiquidities(); vm.etch(hookModifyLiquiditiesAddr, address(impl).code); From 1dd5349fb3d6366aa2a4581f5136b181af7247f4 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 16:24:39 -0400 Subject: [PATCH 4/5] test that hooks cannot re-enter modifyLiquidities --- .../PositionManager.modifiyLiquidities.t.sol | 22 ++++++++++++++++ test/shared/HookModifyLiquidities.sol | 26 +++++++++++++++++++ test/shared/PosmTestSetup.sol | 8 ++++-- 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/test/position-managers/PositionManager.modifiyLiquidities.t.sol b/test/position-managers/PositionManager.modifiyLiquidities.t.sol index f172c1c5..9183ece6 100644 --- a/test/position-managers/PositionManager.modifiyLiquidities.t.sol +++ b/test/position-managers/PositionManager.modifiyLiquidities.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 {ReentrancyLock} from "../../src/base/ReentrancyLock.sol"; import {Actions} from "../../src/libraries/Actions.sol"; import {PositionManager} from "../../src/PositionManager.sol"; import {PositionConfig} from "../../src/libraries/PositionConfig.sol"; @@ -239,4 +240,25 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF ); swap(key, true, -1e18, calls); } + + /// @dev hook cannot re-enter modifyLiquidities in beforeRemoveLiquidity + function test_hook_increaseLiquidity_reenter_revert() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + uint256 newLiquidity = 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); + + // should revert because hook is re-entering modifyLiquidities + vm.expectRevert( + abi.encodeWithSelector( + Hooks.FailedHookCall.selector, abi.encodeWithSelector(ReentrancyLock.ContractLocked.selector) + ) + ); + lpm.unlockAndModifyLiquidities(calls, _deadline); + } } diff --git a/test/shared/HookModifyLiquidities.sol b/test/shared/HookModifyLiquidities.sol index 85bdb897..d3b523e8 100644 --- a/test/shared/HookModifyLiquidities.sol +++ b/test/shared/HookModifyLiquidities.sol @@ -39,6 +39,32 @@ contract HookModifyLiquidities is HookSavesDelta { return (this.beforeSwap.selector, BeforeSwapDeltaLibrary.ZERO_DELTA, 0); } + function beforeAddLiquidity( + address, /* sender **/ + PoolKey calldata, /* key **/ + IPoolManager.ModifyLiquidityParams calldata, /* params **/ + bytes calldata hookData + ) external override returns (bytes4) { + if (hookData.length > 0) { + (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); + posm.modifyLiquidities(actions, params); + } + return this.beforeAddLiquidity.selector; + } + + function beforeRemoveLiquidity( + address, /* sender **/ + PoolKey calldata, /* key **/ + IPoolManager.ModifyLiquidityParams calldata, /* params **/ + bytes calldata hookData + ) external override returns (bytes4) { + if (hookData.length > 0) { + (bytes memory actions, bytes[] memory params) = abi.decode(hookData, (bytes, bytes[])); + posm.modifyLiquidities(actions, params); + } + return this.beforeRemoveLiquidity.selector; + } + 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/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index a1febbde..345726c4 100644 --- a/test/shared/PosmTestSetup.sol +++ b/test/shared/PosmTestSetup.sol @@ -26,8 +26,12 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { address hookAddr = address(uint160(Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)); HookModifyLiquidities hookModifyLiquidities; - address hookModifyLiquiditiesAddr = - address(uint160(Hooks.BEFORE_SWAP_FLAG | Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG)); + address hookModifyLiquiditiesAddr = address( + uint160( + Hooks.BEFORE_SWAP_FLAG | Hooks.BEFORE_ADD_LIQUIDITY_FLAG | Hooks.BEFORE_REMOVE_LIQUIDITY_FLAG + | Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.AFTER_REMOVE_LIQUIDITY_FLAG + ) + ); function deployPosmHookSavesDelta() public { HookSavesDelta impl = new HookSavesDelta(); From ea6994936e2adea0f5563533efcde3a0b9637808 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 4 Aug 2024 16:38:10 -0400 Subject: [PATCH 5/5] hook mints liquidity with modifyLiquidities --- .../PositionManager.modifiyLiquidities.t.sol | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/position-managers/PositionManager.modifiyLiquidities.t.sol b/test/position-managers/PositionManager.modifiyLiquidities.t.sol index 9183ece6..a2a85f29 100644 --- a/test/position-managers/PositionManager.modifiyLiquidities.t.sol +++ b/test/position-managers/PositionManager.modifiyLiquidities.t.sol @@ -58,6 +58,36 @@ contract PositionManagerModifyLiquiditiesTest is Test, PosmTestSetup, LiquidityF config = PositionConfig({poolKey: key, tickLower: -60, tickUpper: 60}); } + /// @dev minting liquidity without approval is allowable + function test_hook_mint() public { + uint256 initialLiquidity = 100e18; + uint256 tokenId = lpm.nextTokenId(); + mint(config, initialLiquidity, address(this), ZERO_BYTES); + + // hook mints a new position in beforeSwap via hookData + uint256 hookTokenId = lpm.nextTokenId(); + uint256 newLiquidity = 10e18; + bytes memory calls = getMintEncoded(config, newLiquidity, address(hookModifyLiquidities), ZERO_BYTES); + + swap(key, true, -1e18, calls); + + bytes32 positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(tokenId)); + (uint256 liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + + // original liquidity unchanged + assertEq(liquidity, initialLiquidity); + + // hook minted its own position + positionId = + Position.calculatePositionKey(address(lpm), config.tickLower, config.tickUpper, bytes32(hookTokenId)); + (liquidity,,) = manager.getPositionInfo(config.poolKey.toId(), positionId); + assertEq(liquidity, newLiquidity); + + assertEq(lpm.ownerOf(tokenId), address(this)); // original position owned by this contract + assertEq(lpm.ownerOf(hookTokenId), address(hookModifyLiquidities)); // hook position owned by hook + } + /// @dev increasing liquidity without approval is allowable function test_hook_increaseLiquidity() public { uint256 initialLiquidity = 100e18;