From 512640226be64eabb9fd9d31cc42794232ed11b3 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 16:13:44 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20fix:=20Conditional=20lock=20on=20fee?= =?UTF-8?q?Destination=20=E2=80=94=20lock=20when=20set=20to=20contract=20(?= =?UTF-8?q?#580)=20(#580)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `feeDestinationLocked` bool to LiquidityManager - Replace one-shot setter with conditional trapdoor: EOAs may be set repeatedly, but setting a contract address locks permanently - Remove `AddressAlreadySet` error (superseded by the new lock mechanic) - Replace fragile SLOT7 storage hack in red-team.sh with a proper `setFeeDestination()` call using the deployer key - Update tests: replace AddressAlreadySet test with three new tests covering EOA multi-set, contract lock, and locked revert Co-Authored-By: Claude Sonnet 4.6 --- onchain/src/LiquidityManager.sol | 10 +++++++--- onchain/test/LiquidityManager.t.sol | 29 +++++++++++++++++++++++++---- scripts/harb-evaluator/red-team.sh | 19 +++++++------------ 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol index 419e27d..cf717a6 100644 --- a/onchain/src/LiquidityManager.sol +++ b/onchain/src/LiquidityManager.sol @@ -47,6 +47,7 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { address private immutable deployer; address public recenterAccess; address public feeDestination; + bool public feeDestinationLocked; /// @notice Last recenter tick — used to determine net trade direction between recenters int24 public lastRecenterTick; @@ -63,7 +64,6 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { /// @notice Custom errors error ZeroAddressInSetter(); - error AddressAlreadySet(); /// @notice Access control modifier modifier onlyFeeDestination() { @@ -114,13 +114,17 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { if (amount1Owed > 0) IERC20(poolKey.token1).safeTransfer(msg.sender, amount1Owed); } - /// @notice Sets the fee destination address (can only be called once, deployer only) + /// @notice Sets the fee destination address (deployer only) + /// @dev Any EOA can be set repeatedly. Once a contract address is set, locks permanently. /// @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(); - if (feeDestination != address(0)) revert AddressAlreadySet(); + require(!feeDestinationLocked, "fee destination locked"); feeDestination = feeDestination_; + if (feeDestination_.code.length > 0) { + feeDestinationLocked = true; + } } /// @notice Sets recenter access for testing/emergency purposes diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index da699a9..9474da7 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/onchain/test/LiquidityManager.t.sol @@ -1017,14 +1017,35 @@ contract LiquidityManagerTest is UniSwapHelper { } /** - * @notice AddressAlreadySet revert when fee destination is changed after initial set + * @notice EOA destinations can be set repeatedly — no lock until a contract is set */ - function testSetFeeDestinationAlreadySet() public { - // Deploy a fresh LM with this test contract as deployer + function testSetFeeDestinationEOA_MultipleAllowed() public { LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer)); freshLm.setFeeDestination(makeAddr("firstFee")); - vm.expectRevert(LiquidityManager.AddressAlreadySet.selector); + assertFalse(freshLm.feeDestinationLocked(), "should not be locked after EOA set"); freshLm.setFeeDestination(makeAddr("secondFee")); + assertFalse(freshLm.feeDestinationLocked(), "should still not be locked after second EOA set"); + } + + /** + * @notice Setting fee destination to a contract locks it permanently + */ + function testSetFeeDestinationContract_Locks() public { + LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer)); + // address(harberg) is a deployed contract + freshLm.setFeeDestination(address(harberg)); + assertTrue(freshLm.feeDestinationLocked(), "should be locked after contract set"); + assertEq(freshLm.feeDestination(), address(harberg)); + } + + /** + * @notice Once locked, setFeeDestination reverts + */ + function testSetFeeDestinationLocked_Reverts() public { + LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer)); + freshLm.setFeeDestination(address(harberg)); + vm.expectRevert("fee destination locked"); + freshLm.setFeeDestination(makeAddr("anyAddr")); } /** diff --git a/scripts/harb-evaluator/red-team.sh b/scripts/harb-evaluator/red-team.sh index 4aa99b9..2d721af 100755 --- a/scripts/harb-evaluator/red-team.sh +++ b/scripts/harb-evaluator/red-team.sh @@ -130,19 +130,14 @@ log "Granting recenterAccess to account 2 ($RECENTER_ADDR) via feeDestination ($ || die "anvil_stopImpersonatingAccount $FEE_DEST failed" log " recenterAccess granted" -# ── 3b. Override feeDestination to LM itself (fees accrue as liquidity) ──────── -# feeDestination is a one-shot setter, so we override storage directly. -# Slot 7 contains feeDestination (address, lower 20 bytes). -# Upper bytes of slot 7 contain other packed state variables. -# NOTE: This is fragile — storage layout changes break this. Consider making -# feeDestination mutable or using Foundry's vm.store with slot lookup. +# ── 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=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 log "Setting feeDestination to LM ($LM) ..." -SLOT7=$("$CAST" storage "$LM" 7 --rpc-url "$RPC_URL" | sed 's/\[.*//;s/[[:space:]]//g') -UPPER=${SLOT7:0:26} -LM_LOWER=$(echo "$LM" | tr '[:upper:]' '[:lower:]' | sed 's/0x//') -NEW_SLOT7="${UPPER}${LM_LOWER}" -"$CAST" rpc --rpc-url "$RPC_URL" anvil_setStorageAt "$LM" "0x7" "$NEW_SLOT7" \ - || die "anvil_setStorageAt for feeDestination failed" +"$CAST" send --rpc-url "$RPC_URL" --private-key "$DEPLOYER_PK" \ + "$LM" "setFeeDestination(address)" "$LM" >/dev/null 2>&1 \ + || die "setFeeDestination($LM) failed" VERIFY=$("$CAST" call "$LM" "feeDestination()(address)" --rpc-url "$RPC_URL" | sed 's/\[.*//;s/[[:space:]]//g') log " feeDestination set to: $VERIFY" [[ "${VERIFY,,}" == "${LM,,}" ]] || die "feeDestination verification failed: expected $LM, got $VERIFY" From 9ff96ff137d2df33a40c105622386d3fa53bcbd2 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 16:34:27 +0000 Subject: [PATCH 2/3] ci: retrigger after infra failure From b902b89e3bca0d5ca646dcc1010d80b7af83c486 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 17:13:50 +0000 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=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" \