fix: address review findings for anchorWidth guard (#817)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
openhands
parent
a21cf398bf
commit
aa274fd8ed
3 changed files with 68 additions and 52 deletions
|
|
@ -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");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue