Merge pull request 'fix: fix: Conditional lock on feeDestination — lock when set to contract (#580) (#580)' (#623) from fix/issue-580 into master

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.
 

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,28 @@ 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 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();
-        if (feeDestination != address(0)) revert AddressAlreadySet();
+        // 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;
+        }
     }
 
     /// @notice Sets recenter access for testing/emergency purposes

onchain/test/LiquidityManager.t.sol

@@ -1017,14 +1017,52 @@ 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 (direct path)
+     */
+    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 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
+     */
+    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"));
     }
 
     /**

scripts/harb-evaluator/red-team.sh

@@ -130,19 +130,16 @@ 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 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) ..."
-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"