diff --git a/.forge-snapshots/poolManager bytecode size.snap b/.forge-snapshots/poolManager bytecode size.snap index c5ac9e9c6..f4e8af3ae 100644 --- a/.forge-snapshots/poolManager bytecode size.snap +++ b/.forge-snapshots/poolManager bytecode size.snap @@ -1 +1 @@ -23805 \ No newline at end of file +23821 \ No newline at end of file diff --git a/.forge-snapshots/swap skips hook call if hook is caller.snap b/.forge-snapshots/swap skips hook call if hook is caller.snap index 3394318c1..705d5eca7 100644 --- a/.forge-snapshots/swap skips hook call if hook is caller.snap +++ b/.forge-snapshots/swap skips hook call if hook is caller.snap @@ -1 +1 @@ -208496 \ No newline at end of file +208547 \ No newline at end of file diff --git a/src/PoolManager.sol b/src/PoolManager.sol index 7a67b866c..83417da01 100644 --- a/src/PoolManager.sol +++ b/src/PoolManager.sol @@ -104,7 +104,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim } /// @inheritdoc IPoolManager - function unlock(bytes calldata data) external override noDelegateCall returns (bytes memory result) { + function unlock(bytes calldata data) external override returns (bytes memory result) { if (Lock.isUnlocked()) AlreadyUnlocked.selector.revertWith(); Lock.unlock(); @@ -150,7 +150,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim PoolKey memory key, IPoolManager.ModifyLiquidityParams memory params, bytes calldata hookData - ) external override onlyWhenUnlocked returns (BalanceDelta callerDelta, BalanceDelta feesAccrued) { + ) external override onlyWhenUnlocked noDelegateCall returns (BalanceDelta callerDelta, BalanceDelta feesAccrued) { PoolId id = key.toId(); Pool.State storage pool = _getPool(id); pool.checkPoolInitialized(); @@ -188,6 +188,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim external override onlyWhenUnlocked + noDelegateCall returns (BalanceDelta swapDelta) { if (params.amountSpecified == 0) SwapAmountCannotBeZero.selector.revertWith(); @@ -249,6 +250,7 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim external override onlyWhenUnlocked + noDelegateCall returns (BalanceDelta delta) { Pool.State storage pool = _getPool(key.toId()); diff --git a/src/test/ProxyPoolManager.sol b/src/test/ProxyPoolManager.sol new file mode 100644 index 000000000..bea23b6f2 --- /dev/null +++ b/src/test/ProxyPoolManager.sol @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.24; + +import {Hooks} from "../libraries/Hooks.sol"; +import {Pool} from "../libraries/Pool.sol"; +import {SafeCast} from "../libraries/SafeCast.sol"; +import {Position} from "../libraries/Position.sol"; +import {LPFeeLibrary} from "../libraries/LPFeeLibrary.sol"; +import {Currency, CurrencyLibrary} from "../types/Currency.sol"; +import {PoolKey} from "../types/PoolKey.sol"; +import {TickMath} from "../libraries/TickMath.sol"; +import {NoDelegateCall} from "../NoDelegateCall.sol"; +import {IHooks} from "../interfaces/IHooks.sol"; +import {IPoolManager} from "../interfaces/IPoolManager.sol"; +import {IUnlockCallback} from "../interfaces/callback/IUnlockCallback.sol"; +import {ProtocolFees} from "../ProtocolFees.sol"; +import {ERC6909Claims} from "../ERC6909Claims.sol"; +import {PoolId, PoolIdLibrary} from "../types/PoolId.sol"; +import {BalanceDelta, BalanceDeltaLibrary, toBalanceDelta} from "../types/BalanceDelta.sol"; +import {BeforeSwapDelta} from "../types/BeforeSwapDelta.sol"; +import {Lock} from "../libraries/Lock.sol"; +import {CurrencyDelta} from "../libraries/CurrencyDelta.sol"; +import {NonZeroDeltaCount} from "../libraries/NonZeroDeltaCount.sol"; +import {Reserves} from "../libraries/Reserves.sol"; +import {Extsload} from "../Extsload.sol"; +import {Exttload} from "../Exttload.sol"; +import {CustomRevert} from "../libraries/CustomRevert.sol"; + +/// @notice A proxy pool manager that delegates calls to the real/delegate pool manager +contract ProxyPoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claims, Extsload, Exttload { + using PoolIdLibrary for PoolKey; + using SafeCast for *; + using Pool for *; + using Hooks for IHooks; + using Position for mapping(bytes32 => Position.Info); + using CurrencyDelta for Currency; + using LPFeeLibrary for uint24; + using Reserves for Currency; + using CustomRevert for bytes4; + + /// @inheritdoc IPoolManager + int24 public constant MAX_TICK_SPACING = TickMath.MAX_TICK_SPACING; + + /// @inheritdoc IPoolManager + int24 public constant MIN_TICK_SPACING = TickMath.MIN_TICK_SPACING; + + mapping(PoolId id => Pool.State) internal _pools; + + address internal immutable _delegateManager; + + constructor(address delegateManager, uint256 controllerGasLimit) ProtocolFees(controllerGasLimit) { + _delegateManager = delegateManager; + } + + /// @notice This will revert if the contract is locked + modifier onlyWhenUnlocked() { + if (!Lock.isUnlocked()) ManagerLocked.selector.revertWith(); + _; + } + + /// @inheritdoc IPoolManager + function unlock(bytes calldata data) external override noDelegateCall returns (bytes memory result) { + if (Lock.isUnlocked()) AlreadyUnlocked.selector.revertWith(); + + Lock.unlock(); + + // the caller does everything in this callback, including paying what they owe via calls to settle + result = IUnlockCallback(msg.sender).unlockCallback(data); + + if (NonZeroDeltaCount.read() != 0) CurrencyNotSettled.selector.revertWith(); + Lock.lock(); + } + + /// @inheritdoc IPoolManager + function initialize(PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData) + external + override + noDelegateCall + returns (int24 tick) + { + // see TickBitmap.sol for overflow conditions that can arise from tick spacing being too large + if (key.tickSpacing > MAX_TICK_SPACING) TickSpacingTooLarge.selector.revertWith(); + if (key.tickSpacing < MIN_TICK_SPACING) TickSpacingTooSmall.selector.revertWith(); + if (key.currency0 >= key.currency1) CurrenciesOutOfOrderOrEqual.selector.revertWith(); + if (!key.hooks.isValidHookAddress(key.fee)) Hooks.HookAddressNotValid.selector.revertWith(address(key.hooks)); + + uint24 lpFee = key.fee.getInitialLPFee(); + + key.hooks.beforeInitialize(key, sqrtPriceX96, hookData); + + PoolId id = key.toId(); + (, uint24 protocolFee) = _fetchProtocolFee(key); + + tick = _pools[id].initialize(sqrtPriceX96, protocolFee, lpFee); + + key.hooks.afterInitialize(key, sqrtPriceX96, tick, hookData); + + // emit all details of a pool key. poolkeys are not saved in storage and must always be provided by the caller + // the key's fee may be a static fee or a sentinel to denote a dynamic fee. + emit Initialize(id, key.currency0, key.currency1, key.fee, key.tickSpacing, key.hooks); + } + + /// @inheritdoc IPoolManager + function modifyLiquidity( + PoolKey memory key, + IPoolManager.ModifyLiquidityParams memory params, + bytes calldata hookData + ) external override onlyWhenUnlocked noDelegateCall returns (BalanceDelta callerDelta, BalanceDelta feesAccrued) { + bytes memory result = _delegateCall( + _delegateManager, abi.encodeWithSelector(this.modifyLiquidity.selector, key, params, hookData) + ); + + return abi.decode(result, (BalanceDelta, BalanceDelta)); + } + + /// @inheritdoc IPoolManager + function swap(PoolKey memory key, IPoolManager.SwapParams memory params, bytes calldata hookData) + external + override + onlyWhenUnlocked + noDelegateCall + returns (BalanceDelta swapDelta) + { + bytes memory result = + _delegateCall(_delegateManager, abi.encodeWithSelector(this.swap.selector, key, params, hookData)); + + return abi.decode(result, (BalanceDelta)); + } + + /// @inheritdoc IPoolManager + function donate(PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData) + external + override + onlyWhenUnlocked + noDelegateCall + returns (BalanceDelta delta) + { + bytes memory result = _delegateCall( + _delegateManager, abi.encodeWithSelector(this.donate.selector, key, amount0, amount1, hookData) + ); + + return abi.decode(result, (BalanceDelta)); + } + + /// @inheritdoc IPoolManager + function sync(Currency currency) public returns (uint256 balance) { + balance = currency.balanceOfSelf(); + currency.setReserves(balance); + } + + /// @inheritdoc IPoolManager + function take(Currency currency, address to, uint256 amount) external override onlyWhenUnlocked noDelegateCall { + _delegateCall(_delegateManager, abi.encodeWithSelector(this.take.selector, currency, to, amount)); + } + + /// @inheritdoc IPoolManager + function settle(Currency currency) + external + payable + override + onlyWhenUnlocked + noDelegateCall + returns (uint256 paid) + { + bytes memory result = _delegateCall(_delegateManager, abi.encodeWithSelector(this.settle.selector, currency)); + + return abi.decode(result, (uint256)); + } + + /// @inheritdoc IPoolManager + function mint(address to, uint256 id, uint256 amount) external override onlyWhenUnlocked noDelegateCall { + _delegateCall(_delegateManager, abi.encodeWithSelector(this.mint.selector, to, id, amount)); + } + + /// @inheritdoc IPoolManager + function burn(address from, uint256 id, uint256 amount) external override onlyWhenUnlocked noDelegateCall { + _delegateCall(_delegateManager, abi.encodeWithSelector(this.burn.selector, from, id, amount)); + } + + /// @inheritdoc IPoolManager + function updateDynamicLPFee(PoolKey memory key, uint24 newDynamicLPFee) external { + if (!key.fee.isDynamicFee() || msg.sender != address(key.hooks)) { + UnauthorizedDynamicLPFeeUpdate.selector.revertWith(); + } + newDynamicLPFee.validate(); + PoolId id = key.toId(); + _pools[id].setLPFee(newDynamicLPFee); + } + + /// @notice Make a delegate call, bubble up any error or return the result + function _delegateCall(address target, bytes memory data) internal returns (bytes memory result) { + (bool success, bytes memory returnData) = target.delegatecall(data); + + if (!success) { + if (returnData.length == 0) { + revert(); + } else { + assembly { + let size := mload(returnData) + revert(add(32, returnData), size) + } + } + } + + return returnData; + } + + /// @notice Implementation of the _getPool function defined in ProtocolFees + function _getPool(PoolId id) internal view override returns (Pool.State storage) { + return _pools[id]; + } +} diff --git a/test/NoDelegateCall.t.sol b/test/NoDelegateCall.t.sol index 4b865841c..61173ecb6 100644 --- a/test/NoDelegateCall.t.sol +++ b/test/NoDelegateCall.t.sol @@ -3,13 +3,26 @@ pragma solidity ^0.8.20; import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol"; import {Test} from "forge-std/Test.sol"; +import {IPoolManager} from "../src/interfaces/IPoolManager.sol"; +import {PoolSwapTest} from "../src/test/PoolSwapTest.sol"; +import {ProxyPoolManager} from "../src/test/ProxyPoolManager.sol"; import {NoDelegateCallTest} from "../src/test/NoDelegateCallTest.sol"; +import {PoolManager} from "../src/PoolManager.sol"; import {NoDelegateCall} from "../src/NoDelegateCall.sol"; +import {Deployers} from "./utils/Deployers.sol"; + +contract TestDelegateCall is Test, Deployers, GasSnapshot { + // override to use ProxyPoolManager + function deployFreshManager() internal virtual override { + IPoolManager delegateManager = new PoolManager(500000); + manager = new ProxyPoolManager(address(delegateManager), 500000); + } -contract TestDelegateCall is Test, GasSnapshot { NoDelegateCallTest noDelegateCallTest; function setUp() public { + deployFreshManagerAndRouters(); + noDelegateCallTest = new NoDelegateCallTest(); } @@ -47,4 +60,25 @@ contract TestDelegateCall is Test, GasSnapshot { // note vm.expectRevert inverts success, so a true result here means it reverted assertTrue(success); } + + function test_modifyLiquidity_noDelegateCall() public { + vm.expectRevert(NoDelegateCall.DelegateCallNotAllowed.selector); + modifyLiquidityRouter.modifyLiquidity(uninitializedKey, LIQUIDITY_PARAMS, ZERO_BYTES); + + vm.expectRevert(NoDelegateCall.DelegateCallNotAllowed.selector); + modifyLiquidityRouter.modifyLiquidity(uninitializedKey, REMOVE_LIQUIDITY_PARAMS, ZERO_BYTES); + } + + function test_swap_noDelegateCall() public { + PoolSwapTest.TestSettings memory testSettings = + PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false}); + + vm.expectRevert(NoDelegateCall.DelegateCallNotAllowed.selector); + swapRouter.swap(key, SWAP_PARAMS, testSettings, ZERO_BYTES); + } + + function test_donate_noDelegateCall() public { + vm.expectRevert(NoDelegateCall.DelegateCallNotAllowed.selector); + donateRouter.donate(key, 100, 200, ZERO_BYTES); + } } diff --git a/test/utils/Deployers.sol b/test/utils/Deployers.sol index e3e3dea00..d043883cd 100644 --- a/test/utils/Deployers.sol +++ b/test/utils/Deployers.sol @@ -91,7 +91,7 @@ contract Deployers { } } - function deployFreshManager() internal { + function deployFreshManager() internal virtual { manager = new PoolManager(500000); }