From aa274fd8ed8a39d57d0c81c5d5b5084ffff5db0d Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 15 Mar 2026 22:04:13 +0000 Subject: [PATCH] fix: address review findings for anchorWidth guard (#817) Co-Authored-By: Claude Sonnet 4.6 --- onchain/test/LiquidityManager.t.sol | 66 +++++++++++++++---- .../abstracts/ThreePositionStrategy.t.sol | 7 +- tools/push3-evolution/evolution.patch | 47 +++---------- 3 files changed, 68 insertions(+), 52 deletions(-) diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index 1957cee..a546253 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/onchain/test/LiquidityManager.t.sol @@ -1118,12 +1118,12 @@ contract LiquidityManagerTest is UniSwapHelper { } /** - * @notice Optimizer returning anchorWidth > 100 is passed through unchanged (no clamping). - * @dev Verifies that LiquidityManager does NOT clamp oversized anchorWidth values from - * the optimizer. The resulting anchor position tick range must match anchorWidth=150 exactly. + * @notice Optimizer returning anchorWidth=150 is passed through unchanged (below MAX_ANCHOR_WIDTH=1233). + * @dev Verifies that LiquidityManager does NOT clamp values within the safe ceiling. + * The resulting anchor position tick range must match anchorWidth=150 exactly. */ - function testAnchorWidthAbove100IsNotClamped() public { - // Deploy a ConfigurableOptimizer that returns anchorWidth = 150 + function testAnchorWidthBelowMaxIsNotClamped() public { + // Deploy a ConfigurableOptimizer that returns anchorWidth = 150 (well below MAX_ANCHOR_WIDTH=1233) ConfigurableOptimizer highWidthOptimizer = new ConfigurableOptimizer( 0, // capitalInefficiency = 0 (safest) 3e17, // anchorShare = 30% @@ -1149,16 +1149,60 @@ contract LiquidityManagerTest is UniSwapHelper { vm.prank(RECENTER_CALLER); customLm.recenter(); - // Anchor position must exist and its tick range must match anchorWidth=150 (not clamped to 100). + // Anchor position must exist and its tick range must match anchorWidth=150 (not clamped). // The anchor spacing formula is: anchorSpacing = TICK_SPACING + (34 * anchorWidth * TICK_SPACING / 100) - // For anchorWidth=100: anchorSpacing = 200 + 34*100*200/100 = 7000; tickWidth = 2*7000 = 14000 // For anchorWidth=150: anchorSpacing = 200 + 34*150*200/100 = 10400; tickWidth = 2*10400 = 20800 (uint128 liquidity, int24 tickLower, int24 tickUpper) = customLm.positions(ThreePositionStrategy.Stage.ANCHOR); assertTrue(liquidity > 0, "anchor position should have been placed"); int24 tickWidth = tickUpper - tickLower; - int24 expectedUnclamped = 2 * (TICK_SPACING + (34 * int24(150) * TICK_SPACING / 100)); // 20800 - int24 oldClamped100 = 2 * (TICK_SPACING + (34 * int24(100) * TICK_SPACING / 100)); // 14000 - assertEq(tickWidth, expectedUnclamped, "anchorWidth=150 must not be clamped - tick range must match 150"); - assertTrue(tickWidth > oldClamped100, "unclamped width must be wider than the old MAX_ANCHOR_WIDTH=100 range"); + int24 expected150 = 2 * (TICK_SPACING + (34 * int24(150) * TICK_SPACING / 100)); // 20800 + assertEq(tickWidth, expected150, "anchorWidth=150 must not be clamped"); + } + + /** + * @notice Optimizer returning anchorWidth above MAX_ANCHOR_WIDTH (1233) is clamped at recenter(). + * @dev Guards against a buggy or adversarial optimizer causing int24 overflow in the tick + * computation: 34 * 1234 * 200 = 8,391,200 > int24 max (8,388,607). LiquidityManager + * clamps anchorWidth to MAX_ANCHOR_WIDTH before calling _setPositions. + */ + function testAnchorWidthAboveMaxIsClamped() public { + // Deploy a ConfigurableOptimizer that returns anchorWidth = 1234 (one above MAX_ANCHOR_WIDTH=1233) + ConfigurableOptimizer oversizedOptimizer = new ConfigurableOptimizer( + 0, // capitalInefficiency = 0 (safest) + 3e17, // anchorShare = 30% + 1234, // anchorWidth = 1234 > MAX_ANCHOR_WIDTH — would overflow int24 without the clamp + 3e17 // discoveryDepth = 30% + ); + + TestEnvironment clampTestEnv = new TestEnvironment(feeDestination); + ( + , + , + , + , + , + LiquidityManager customLm, + , + ) = clampTestEnv.setupEnvironmentWithOptimizer( + DEFAULT_TOKEN0_IS_WETH, + address(oversizedOptimizer) + ); + + // recenter() must succeed — the clamp in LiquidityManager prevents overflow + vm.prank(RECENTER_CALLER); + customLm.recenter(); + + // Anchor tick range must match anchorWidth=1233 (clamped), NOT 1234 (which would revert). + // anchorSpacing for 1233: 200 + 34*1233*200/100 = 84044. + // _clampToTickSpacing truncates each end to nearest 200: 84044/200*200 = 84000 per side. + // tickWidth = 2 * 84000 = 168000. + // anchorSpacing for 1234: would overflow int24 → panic without clamp. + (uint128 liquidity, int24 tickLower, int24 tickUpper) = customLm.positions(ThreePositionStrategy.Stage.ANCHOR); + assertTrue(liquidity > 0, "anchor position should have been placed after clamp"); + int24 tickWidth = tickUpper - tickLower; + // Compute expected width accounting for _clampToTickSpacing truncation on each tick half + int24 anchorSpacing1233 = TICK_SPACING + (34 * int24(1233) * TICK_SPACING / 100); // 84044 + int24 expectedClamped = 2 * (anchorSpacing1233 / TICK_SPACING * TICK_SPACING); // 2 * 84000 = 168000 + assertEq(tickWidth, expectedClamped, "anchorWidth=1234 must be clamped to 1233"); } } diff --git a/onchain/test/abstracts/ThreePositionStrategy.t.sol b/onchain/test/abstracts/ThreePositionStrategy.t.sol index cfc868d..1292d9a 100644 --- a/onchain/test/abstracts/ThreePositionStrategy.t.sol +++ b/onchain/test/abstracts/ThreePositionStrategy.t.sol @@ -492,9 +492,10 @@ contract ThreePositionStrategyTest is TestConstants { assertGt(pos.liquidity, 0, "Anchor should have positive liquidity"); } - function testAnchorWidthAboveMaxReverts() public { - // anchorWidth = 1234: 34 * 1234 * 200 = 8,391,200 overflows int24 — must not silently produce - // an invalid tick range; Solidity 0.8 arithmetic check causes a panic revert instead. + function testAnchorWidthAboveMaxOverflowsAtStrategyLayer() public { + // Calling the strategy directly with anchorWidth=1234 panics at the int24 multiplication + // (34 * 1234 * 200 = 8,391,200 > int24 max 8,388,607). This demonstrates why + // LiquidityManager clamps anchorWidth to MAX_ANCHOR_WIDTH before calling _setPositions. ThreePositionStrategy.PositionParams memory params = getDefaultParams(); params.anchorWidth = 1234; diff --git a/tools/push3-evolution/evolution.patch b/tools/push3-evolution/evolution.patch index a2f9c69..b08d98e 100644 --- a/tools/push3-evolution/evolution.patch +++ b/tools/push3-evolution/evolution.patch @@ -1,39 +1,10 @@ -diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol -index 0daccf9..e3a9b2f 100644 ---- a/onchain/src/LiquidityManager.sol -+++ b/onchain/src/LiquidityManager.sol -@@ -36,7 +36,7 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { - - /// @notice Maximum anchor width (in ticks) accepted from the optimizer. - /// Any optimizer-returned value above this ceiling is silently clamped down. -- uint24 internal constant MAX_ANCHOR_WIDTH = 100; +diff --git a/onchain/src/abstracts/ThreePositionStrategy.sol b/onchain/src/abstracts/ThreePositionStrategy.sol +index 0000000..0000000 100644 +--- a/onchain/src/abstracts/ThreePositionStrategy.sol ++++ b/onchain/src/abstracts/ThreePositionStrategy.sol +@@ -33,7 +33,7 @@ + /// @notice Maximum safe anchorWidth: ensures 34 * MAX_ANCHOR_WIDTH * TICK_SPACING / 100 fits in int24 + /// @dev With TICK_SPACING=200: 34 * 1233 * 200 = 8,384,400 ≤ int24 max (8,388,607). + /// anchorWidth=1234 produces 8,391,200 which overflows int24 and reverts in Solidity 0.8. +- uint24 internal constant MAX_ANCHOR_WIDTH = 1233; + uint24 internal constant MAX_ANCHOR_WIDTH = type(uint24).max; - - /// @notice Upper bound (inclusive) for scale-1 optimizer parameters: capitalInefficiency, - /// anchorShare, and discoveryDepth. Values above this ceiling are silently clamped. -diff --git a/onchain/src/Optimizer.sol b/onchain/src/Optimizer.sol -index 4efa74c..a29612f 100644 ---- a/onchain/src/Optimizer.sol -+++ b/onchain/src/Optimizer.sol -@@ -136,7 +136,7 @@ contract Optimizer is Initializable, UUPSUpgradeable, IOptimizer { - /// staticcall to actually receive 200 000. Callers with exactly 200–203 k - /// gas will see a spurious bear-defaults fallback. This is not a practical - /// concern from recenter(), which always has abundant gas. -- uint256 internal constant CALCULATE_PARAMS_GAS_LIMIT = 200_000; -+ uint256 internal constant CALCULATE_PARAMS_GAS_LIMIT = 500_000; - - /** - * @notice Initialize the Optimizer. -diff --git a/onchain/test/FitnessEvaluator.t.sol b/onchain/test/FitnessEvaluator.t.sol -index 9434163..5b91eca 100644 ---- a/onchain/test/FitnessEvaluator.t.sol -+++ b/onchain/test/FitnessEvaluator.t.sol -@@ -152,7 +152,7 @@ contract FitnessEvaluator is Test { - /// @dev Must match Optimizer.CALCULATE_PARAMS_GAS_LIMIT. Candidates that exceed - /// this limit would unconditionally produce bear defaults in production and - /// are disqualified (fitness = 0) rather than scored against their theoretical output. -- uint256 internal constant CALCULATE_PARAMS_GAS_LIMIT = 200_000; -+ uint256 internal constant CALCULATE_PARAMS_GAS_LIMIT = 500_000; - - /// @dev Soft gas penalty: wei deducted from fitness per gas unit used by calculateParams. - /// Creates selection pressure toward leaner programs while keeping gas as a