From b902b89e3bca0d5ca646dcc1010d80b7af83c486 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 17:13:50 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20findings=20=E2=80=94?= =?UTF-8?q?=20CREATE2=20guard,=20transition=20test,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - LiquidityManager.setFeeDestination: add CREATE2 bypass guard — also blocks re-assignment when the current feeDestination has since acquired bytecode (was a plain address when set, contract deployed to it later) - LiquidityManager.setFeeDestination: expand NatSpec to document the EOA-mutability trade-off and the CREATE2 guard explicitly - Test: add testSetFeeDestinationEOAToContract_Locks covering the realistic EOA→contract transition (the primary lock-activation path) - red-team.sh: add comment that DEPLOYER_PK is Anvil account-0 and must only be used against a local ephemeral Anvil instance - ARCHITECTURE.md: document feeDestination conditional-lock semantics and contrast with Kraiken's strictly set-once liquidityManager/stakingPool Co-Authored-By: Claude Sonnet 4.6 --- docs/ARCHITECTURE.md | 1 + onchain/src/LiquidityManager.sol | 17 ++++++++++++++--- onchain/test/LiquidityManager.t.sol | 19 ++++++++++++++++++- scripts/harb-evaluator/red-team.sh | 2 ++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index dd4b178..f42c302 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -34,6 +34,7 @@ Optimizer.sol (UUPS Upgradeable Proxy) - **Optimizer → Stake**: reads sentiment data (% staked, avg tax rate) - **Optimizer upgrades**: UUPS proxy, admin-only `_authorizeUpgrade()` - **feeDestination receives both WETH and KRK fees**: during `recenter()`, Uniswap V3 fee collection produces both tokens. WETH fees AND KRK fees are forwarded to `feeDestination` (see `LiquidityManager._scrapePositions()`). +- **feeDestination is a conditional-lock (not set-once)**: `setFeeDestination()` (deployer-only) allows repeated changes while the destination is an EOA, enabling staged deployment and testing. The moment a contract address is set, `feeDestinationLocked` is set to `true` and no further changes are allowed. A CREATE2 guard also blocks re-assignment if the current destination has since acquired bytecode. This differs from Kraiken's `liquidityManager`/`stakingPool` which are strictly set-once. - **feeDestination KRK excluded from outstanding supply**: `_getOutstandingSupply()` subtracts `kraiken.balanceOf(feeDestination)` before computing scarcity, because protocol-held KRK cannot be sold into the floor and should not inflate the supply count. - **Staking pool KRK excluded from outstanding supply**: `_getOutstandingSupply()` also subtracts `kraiken.balanceOf(stakingPoolAddr)`, because staked KRK is locked and similarly cannot be sold into the floor. diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol index cf717a6..91e2787 100644 --- a/onchain/src/LiquidityManager.sol +++ b/onchain/src/LiquidityManager.sol @@ -114,13 +114,24 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { if (amount1Owed > 0) IERC20(poolKey.token1).safeTransfer(msg.sender, amount1Owed); } - /// @notice Sets the fee destination address (deployer only) - /// @dev Any EOA can be set repeatedly. Once a contract address is set, locks permanently. + /// @notice Sets the fee destination address (deployer only). + /// @dev Conditional trapdoor: EOA addresses can be set repeatedly (allows setup/migration), + /// but once a contract address is assigned, feeDestinationLocked is set and no further + /// changes are allowed. Note: the deployer retains fee-redirect ability while feeDestination + /// is an EOA; this is an intentional trade-off that enables staged deployment. + /// + /// CREATE2 guard: also blocks re-assignment if the current feeDestination has since + /// acquired bytecode (i.e. was a pre-computed EOA when set, later deployed to). /// @param feeDestination_ The address that will receive trading fees function setFeeDestination(address feeDestination_) external { require(msg.sender == deployer, "only deployer"); if (address(0) == feeDestination_) revert ZeroAddressInSetter(); - require(!feeDestinationLocked, "fee destination locked"); + // Block if explicitly locked OR if the current destination has since become a contract + // (guards CREATE2 bypass: address looked like an EOA when set, bytecode deployed later) + require( + !feeDestinationLocked && (feeDestination == address(0) || feeDestination.code.length == 0), + "fee destination locked" + ); feeDestination = feeDestination_; if (feeDestination_.code.length > 0) { feeDestinationLocked = true; diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index 9474da7..5ae78bf 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/onchain/test/LiquidityManager.t.sol @@ -1028,7 +1028,7 @@ contract LiquidityManagerTest is UniSwapHelper { } /** - * @notice Setting fee destination to a contract locks it permanently + * @notice Setting fee destination to a contract locks it permanently (direct path) */ function testSetFeeDestinationContract_Locks() public { LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer)); @@ -1038,6 +1038,23 @@ contract LiquidityManagerTest is UniSwapHelper { assertEq(freshLm.feeDestination(), address(harberg)); } + /** + * @notice Realistic deployment path: set EOA first, then upgrade to a contract — lock triggers on transition + */ + function testSetFeeDestinationEOAToContract_Locks() public { + LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer)); + // Step 1: set to an EOA during setup — allowed, not locked + freshLm.setFeeDestination(makeAddr("treasuryEOA")); + assertFalse(freshLm.feeDestinationLocked(), "not locked after EOA set"); + // Step 2: upgrade to treasury contract once it is deployed — locks permanently + freshLm.setFeeDestination(address(harberg)); + assertTrue(freshLm.feeDestinationLocked(), "locked after contract set"); + assertEq(freshLm.feeDestination(), address(harberg)); + // Step 3: any further attempt reverts + vm.expectRevert("fee destination locked"); + freshLm.setFeeDestination(makeAddr("attacker")); + } + /** * @notice Once locked, setFeeDestination reverts */ diff --git a/scripts/harb-evaluator/red-team.sh b/scripts/harb-evaluator/red-team.sh index 2d721af..dd67906 100755 --- a/scripts/harb-evaluator/red-team.sh +++ b/scripts/harb-evaluator/red-team.sh @@ -133,6 +133,8 @@ log " recenterAccess granted" # ── 3b. Set feeDestination to LM itself (fees accrue as liquidity) ───────────── # setFeeDestination allows repeated EOA sets; setting to a contract locks it permanently. # The deployer (Anvil account 0) deployed LiquidityManager and may call setFeeDestination again. +# DEPLOYER_PK is Anvil's deterministic account-0 key — valid ONLY against a local ephemeral +# Anvil instance. Never run this script against a non-ephemeral or shared-state chain. DEPLOYER_PK=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 log "Setting feeDestination to LM ($LM) ..." "$CAST" send --rpc-url "$RPC_URL" --private-key "$DEPLOYER_PK" \