Merge pull request 'fix: CREATE2 self-destruct bypass in onchain/src/LiquidityManager.sol (#921)' (#957) from fix/issue-921 into master

onchain/src/LiquidityManager.sol

@@ -112,23 +112,36 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
     }
 
     /// @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.
+    /// @dev Trapdoor lock: EOA addresses can be set repeatedly (enables staged deployment/migration)
+    ///      until the lock is triggered. The lock fires in two ways:
     ///
-    ///      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).
+    ///      1. Direct assignment: setting feeDestination to a contract address (code.length > 0)
+    ///         immediately and permanently locks further changes. This is the expected production
+    ///         path — set an EOA during development, upgrade to a treasury contract when ready.
+    ///
+    ///      2. Defensive CREATE2 guard: if the current feeDestination was an EOA when set but has
+    ///         since acquired bytecode (e.g. via CREATE2 deployment), the next call to this
+    ///         function permanently sets feeDestinationLocked = true and returns WITHOUT reverting.
+    ///         Not reverting is intentional: only a successful transaction commits the lock to
+    ///         storage, so the lock survives a subsequent SELFDESTRUCT that would otherwise clear
+    ///         the bytecode evidence and re-open the assignment window.
+    ///
+    ///      Remaining limitation: an atomic CREATE2+SELFDESTRUCT within a single transaction
+    ///      (still possible post-EIP-6780 for contracts that deploy and destroy in one call)
+    ///      cannot be detected here because the bytecode is already absent when this function
+    ///      executes. No on-chain guard can close that sub-transaction window.
     /// @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();
-        // 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"
-        );
+        // Defensive CREATE2 guard: if the current destination has acquired bytecode since being
+        // set as an EOA, lock permanently and return WITHOUT reverting so the write is committed
+        // to storage. A subsequent SELFDESTRUCT clears the bytecode but cannot undo this write.
+        if (!feeDestinationLocked && feeDestination != address(0) && feeDestination.code.length > 0) {
+            feeDestinationLocked = true;
+            return;
+        }
+        require(!feeDestinationLocked, "fee destination locked");
         feeDestination = feeDestination_;
         if (feeDestination_.code.length > 0) {
             feeDestinationLocked = true;

onchain/test/LiquidityManager.t.sol

@@ -1023,6 +1023,39 @@ contract LiquidityManagerTest is UniSwapHelper {
         freshLm.setFeeDestination(makeAddr("anyAddr"));
     }
 
+    /**
+     * @notice CREATE2 bypass defense: if the current feeDestination acquires bytecode after being
+     *         set as an EOA, the next setFeeDestination call commits the lock WITHOUT reverting so
+     *         the lock persists even after a subsequent SELFDESTRUCT clears the bytecode.
+     */
+    function testSetFeeDestination_CREATE2BytecodeDetection_Locks() public {
+        LiquidityManager freshLm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer));
+        address eoaAddr = makeAddr("precomputedEOA");
+
+        // Step 1: set to EOA — allowed, lock stays false
+        freshLm.setFeeDestination(eoaAddr);
+        assertFalse(freshLm.feeDestinationLocked(), "not locked after EOA set");
+
+        // Step 2: simulate CREATE2 deploy — bytecode now exists at the fee destination address
+        vm.etch(eoaAddr, hex"00");
+        assertGt(eoaAddr.code.length, 0, "precondition: address has code");
+
+        // Step 3: calling setFeeDestination detects bytecode at the current destination and
+        //         commits feeDestinationLocked = true WITHOUT reverting, so the write survives
+        //         a later SELFDESTRUCT. feeDestination itself is not changed.
+        freshLm.setFeeDestination(makeAddr("attacker"));
+        assertTrue(freshLm.feeDestinationLocked(), "locked after bytecode detected at current feeDestination");
+        assertEq(freshLm.feeDestination(), eoaAddr, "feeDestination must not change when defensive lock triggers");
+
+        // Step 4: simulate SELFDESTRUCT — bytecode gone, but storage lock persists
+        vm.etch(eoaAddr, hex"");
+        assertEq(eoaAddr.code.length, 0, "code cleared (selfdestruct simulation)");
+
+        // Step 5: re-assignment must still be blocked despite cleared bytecode
+        vm.expectRevert("fee destination locked");
+        freshLm.setFeeDestination(makeAddr("attacker"));
+    }
+
     /**
      * @notice When optimizer reverts, the catch block uses default bear params (covers lines 192-201)
      */