From 534382f785e74bde919a335557ea08f32103f4b3 Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 18 Mar 2026 11:58:28 +0000 Subject: [PATCH] fix: CREATE2 self-destruct bypass in onchain/src/LiquidityManager.sol (#921) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard blocked setFeeDestination when feeDestination.code.length > 0 but did not persist feeDestinationLocked — a revert undoes all state changes. An attacker could CREATE2-deploy bytecode to the EOA fee destination, triggering the block, then SELFDESTRUCT to clear the code, then call setFeeDestination again successfully (lock was never committed). Fix: detect bytecode at the current feeDestination first; if found, set feeDestinationLocked = true and RETURN (not revert) so the storage write is committed. A subsequent SELFDESTRUCT cannot undo a committed storage slot. Updated NatSpec documents both the protection and the remaining limitation (atomic CREATE2+SELFDESTRUCT in a single tx cannot be detected). Added testSetFeeDestination_CREATE2BytecodeDetection_Locks covering: set EOA → vm.etch (simulate CREATE2 deploy) → verify lock committed → vm.etch empty (simulate selfdestruct) → verify setter still blocked. Co-Authored-By: Claude Sonnet 4.6 --- onchain/src/LiquidityManager.sol | 37 +++++++++++++++++++---------- onchain/test/LiquidityManager.t.sol | 33 +++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol index c224016..34cae67 100644 --- a/onchain/src/LiquidityManager.sol +++ b/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; diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index a546253..8c75a79 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/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) */