Merge pull request 'fix: _scrapePositions: unguarded safeTransfer to address(0) when feeDestination unset (#624)' (#1029) from fix/issue-624 into master

onchain/src/LiquidityManager.sol

@@ -273,8 +273,8 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
 
         // Transfer fees and record volume for VWAP
         // VWAP is recorded only on sell events (price fell) or at bootstrap — see recenter().
-        // Skip transfer when feeDestination is self — fees accrue as deployable liquidity.
+        // Skip transfer when feeDestination is self or unset — fees accrue as deployable liquidity.
-        if (feeDestination != address(this)) {
+        if (feeDestination != address(this) && feeDestination != address(0)) {
             if (fee0 > 0) {
                 if (token0isWeth) {
                     IERC20(address(weth)).safeTransfer(feeDestination, fee0);

onchain/test/LiquidityManager.t.sol

@@ -1164,6 +1164,62 @@ contract LiquidityManagerTest is UniSwapHelper {
         assertEq(weth.balanceOf(makeAddr("stakeFeeDestination")), 0, "No WETH to stake fee address");
     }
 
+    /**
+     * @notice When feeDestination is unset (address(0)), recenter must not revert.
+     *         Fees accrue as deployable liquidity instead of attempting safeTransfer to address(0).
+     *
+     * Regression test for issue #624: without the fix, _scrapePositions checks
+     * `feeDestination != address(this)` which is true for address(0), causing
+     * safeTransfer to address(0) which reverts in OpenZeppelin ERC20.
+     */
+    function testRecenterWithUnsetFeeDestination_FeesAccrueAsLiquidity() public {
+        disableAutoSetup();
+
+        // Deploy a fresh environment where setFeeDestination is never called
+        TestEnvironment zeroFeeEnv = new TestEnvironment(makeAddr("unused"));
+        (
+            IUniswapV3Factory _factory,
+            IUniswapV3Pool _pool,
+            IWETH9 _weth,
+            Kraiken _harberg,
+            ,
+            LiquidityManager _lm,
+            Optimizer _optimizer,
+            bool _token0isWeth
+        ) = zeroFeeEnv.setupEnvironmentWithUnsetFeeDestination(DEFAULT_TOKEN0_IS_WETH);
+
+        // Wire state variables used by buy/sell helpers
+        factory = _factory;
+        pool = _pool;
+        weth = _weth;
+        harberg = _harberg;
+        lm = _lm;
+        optimizer = _optimizer;
+        token0isWeth = _token0isWeth;
+
+        assertEq(lm.feeDestination(), address(0), "precondition: feeDestination must be unset");
+
+        // Fund the trader account
+        vm.deal(account, 100 ether);
+        vm.prank(account);
+        weth.deposit{ value: 100 ether }();
+
+        // First recenter: creates positions (no scrape needed, no fees yet)
+        vm.prank(RECENTER_CALLER);
+        lm.recenter();
+
+        // Generate fees via a buy
+        buyRaw(10 ether);
+
+        // Warp past cooldown + TWAP settlement
+        vm.warp(block.timestamp + 301);
+
+        // Second recenter: _scrapePositions runs with non-zero fees.
+        // Without the fix this reverts on safeTransfer to address(0).
+        vm.prank(RECENTER_CALLER);
+        lm.recenter(); // must not revert
+    }
+
     /**
      * @notice Deploy a LiquidityManagerHarness and call the exposed abstract functions
      *         to cover _getKraikenToken() and _getWethToken() (lines 270-271, 275-276)

onchain/test/helpers/TestBase.sol

@@ -284,6 +284,46 @@ contract TestEnvironment is TestConstants {
         return (factory, pool, weth, harberg, stake, lm, optimizer, token0isWeth);
     }
 
+    /**
+     * @notice Deploy all contracts WITHOUT calling setFeeDestination — feeDestination stays address(0).
+     * @dev Regression helper for issue #624: verifies that _scrapePositions gracefully handles
+     *      an uninitialized feeDestination instead of attempting safeTransfer to address(0).
+     * @param token0shouldBeWeth Whether WETH should be token0
+     */
+    function setupEnvironmentWithUnsetFeeDestination(bool token0shouldBeWeth)
+        external
+        returns (
+            IUniswapV3Factory _factory,
+            IUniswapV3Pool _pool,
+            IWETH9 _weth,
+            Kraiken _harberg,
+            Stake _stake,
+            LiquidityManager _lm,
+            Optimizer _optimizer,
+            bool _token0isWeth
+        )
+    {
+        // Deploy factory and tokens
+        factory = UniswapHelpers.deployUniswapFactory();
+        _deployTokensWithOrder(token0shouldBeWeth);
+        _createAndInitializePool();
+
+        // Deploy protocol contracts; Stake gets a throwaway feeDestination address
+        address stakeFee = makeAddr("stakeFeeDestination");
+        stake = new Stake(address(harberg), stakeFee);
+        optimizer = Optimizer(address(new MockOptimizer()));
+        optimizer.initialize(address(harberg), address(stake));
+        lm = new LiquidityManager(address(factory), address(weth), address(harberg), address(optimizer));
+
+        // Intentionally do NOT call lm.setFeeDestination() — feeDestination stays address(0)
+
+        harberg.setStakingPool(address(stake));
+        harberg.setLiquidityManager(address(lm));
+        vm.deal(address(lm), INITIAL_LM_ETH_BALANCE);
+
+        return (factory, pool, weth, harberg, stake, lm, optimizer, token0isWeth);
+    }
+
     /**
      * @notice Setup environment with existing factory and specific optimizer
      * @param existingFactory The existing Uniswap factory to use