diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol index 084655b..2985536 100644 --- a/onchain/src/LiquidityManager.sol +++ b/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. - if (feeDestination != address(this)) { + // Skip transfer when feeDestination is self or unset — fees accrue as deployable liquidity. + if (feeDestination != address(this) && feeDestination != address(0)) { if (fee0 > 0) { if (token0isWeth) { IERC20(address(weth)).safeTransfer(feeDestination, fee0); diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index b092f14..b3b6149 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/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) diff --git a/onchain/test/helpers/TestBase.sol b/onchain/test/helpers/TestBase.sol index d78ec87..154200d 100644 --- a/onchain/test/helpers/TestBase.sol +++ b/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