From 512640226be64eabb9fd9d31cc42794232ed11b3 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 16:13:44 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20fix:=20Conditional=20lock=20on=20feeDest?= =?UTF-8?q?ination=20=E2=80=94=20lock=20when=20set=20to=20contract=20(#580?= =?UTF-8?q?)=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"