diff --git a/onchain/src/LiquidityManager.sol b/onchain/src/LiquidityManager.sol index 047450a..3786e35 100644 --- a/onchain/src/LiquidityManager.sol +++ b/onchain/src/LiquidityManager.sol @@ -234,23 +234,29 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { // Transfer fees and record volume for VWAP // Only record VWAP when net ETH inflow (KRK sold out) — prevents sell-back - // activity from diluting the price memory of original KRK distribution - if (fee0 > 0) { - if (token0isWeth) { - IERC20(address(weth)).safeTransfer(feeDestination, fee0); - if (recordVWAP) _recordVolumeAndPrice(currentPrice, fee0); - } else { - IERC20(address(kraiken)).safeTransfer(feeDestination, fee0); + // activity from diluting the price memory of original KRK distribution. + // Skip transfer when feeDestination is self — fees accrue as deployable liquidity. + if (feeDestination != address(this)) { + if (fee0 > 0) { + if (token0isWeth) { + IERC20(address(weth)).safeTransfer(feeDestination, fee0); + } else { + IERC20(address(kraiken)).safeTransfer(feeDestination, fee0); + } + } + if (fee1 > 0) { + if (token0isWeth) { + IERC20(address(kraiken)).safeTransfer(feeDestination, fee1); + } else { + IERC20(address(weth)).safeTransfer(feeDestination, fee1); + } } } - if (fee1 > 0) { - if (token0isWeth) { - IERC20(address(kraiken)).safeTransfer(feeDestination, fee1); - } else { - IERC20(address(weth)).safeTransfer(feeDestination, fee1); - if (recordVWAP) _recordVolumeAndPrice(currentPrice, fee1); - } + // Always record VWAP regardless of fee destination + if (recordVWAP) { + uint256 ethFee = token0isWeth ? fee0 : fee1; + if (ethFee > 0) _recordVolumeAndPrice(currentPrice, ethFee); } } @@ -297,7 +303,9 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle { /// since neither can be sold into the floor — only trader-held KRK matters for scarcity function _getOutstandingSupply() internal view override returns (uint256) { uint256 supply = kraiken.outstandingSupply(); - if (feeDestination != address(0)) { + // Skip subtraction when feeDestination is self: outstandingSupply() already + // excludes kraiken.balanceOf(address(this)), so subtracting again would double-count. + if (feeDestination != address(0) && feeDestination != address(this)) { supply -= kraiken.balanceOf(feeDestination); } (, address stakingPoolAddr) = kraiken.peripheryContracts(); diff --git a/onchain/test/LiquidityManager.t.sol b/onchain/test/LiquidityManager.t.sol index a3907e8..da699a9 100644 --- a/onchain/test/LiquidityManager.t.sol +++ b/onchain/test/LiquidityManager.t.sol @@ -1073,6 +1073,67 @@ contract LiquidityManagerTest is UniSwapHelper { assertTrue(lm.lastRecenterTick() != 0, "lastRecenterTick should have been updated after recenter"); } + /** + * @notice When feeDestination == address(lm), recenter must not transfer fees externally + * and _getOutstandingSupply must not double-subtract LM-held KRK. + * + * Regression test for issue #533: without the fix, the second recenter (which scrapes + * positions and temporarily puts principal KRK into the LM's balance) triggers an + * arithmetic underflow in _getOutstandingSupply because kraiken.outstandingSupply() + * already excludes balanceOf(lm) and the old code subtracted it a second time. + */ + function testSelfFeeDestination_FeesAccrueAsLiquidity() public { + disableAutoSetup(); + + // Deploy a fresh environment where LM's own address is the feeDestination + TestEnvironment selfFeeEnv = new TestEnvironment(makeAddr("unused")); + ( + IUniswapV3Factory _factory, + IUniswapV3Pool _pool, + IWETH9 _weth, + Kraiken _harberg, + , + LiquidityManager _lm, + Optimizer _optimizer, + bool _token0isWeth + ) = selfFeeEnv.setupEnvironmentWithSelfFeeDestination(DEFAULT_TOKEN0_IS_WETH, RECENTER_CALLER); + + // Wire state variables used by buy/sell/recenter helpers + factory = _factory; + pool = _pool; + weth = _weth; + harberg = _harberg; + lm = _lm; + optimizer = _optimizer; + token0isWeth = _token0isWeth; + + assertEq(lm.feeDestination(), address(lm), "precondition: feeDestination must be self"); + + // Fund the trader account + vm.deal(account, 100 ether); + vm.prank(account); + weth.deposit{ value: 100 ether }(); + + // First recenter: no existing positions, amplitude check skipped, positions created + vm.prank(RECENTER_CALLER); + lm.recenter(); + + // Move price up with a buy so the second recenter satisfies amplitude requirement + buyRaw(10 ether); + + // Second recenter: _scrapePositions() burns positions and collects principal KRK + // into the LM's balance. _setPositions() then calls _getOutstandingSupply(). + // Without the fix: outstandingSupply() already excludes balanceOf(lm), and + // the old code subtracts balanceOf(feeDestination)==balanceOf(lm) again → underflow. + // With the fix: the second subtraction is skipped → no underflow. + vm.prank(RECENTER_CALLER); + lm.recenter(); // must not revert + + // Verify no WETH leaked to any external address — the LM kept its own fees + assertEq(weth.balanceOf(makeAddr("unused")), 0, "No WETH should have been sent to any external address"); + assertEq(weth.balanceOf(makeAddr("stakeFeeDestination")), 0, "No WETH to stake fee address"); + } + /** * @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 574985f..52d996b 100644 --- a/onchain/test/helpers/TestBase.sol +++ b/onchain/test/helpers/TestBase.sol @@ -255,6 +255,57 @@ contract TestEnvironment is TestConstants { return (factory, pool, weth, harberg, stake, lm, optimizer, token0isWeth); } + /** + * @notice Deploy all contracts with feeDestination set to the LiquidityManager itself. + * @dev Useful for testing the "fees accrue as liquidity" path introduced in issue #533. + * When feeDestination == address(lm), _scrapePositions skips fee transfers and + * _getOutstandingSupply skips the feeDestination KRK subtraction (already excluded + * by outstandingSupply()). + * @param token0shouldBeWeth Whether WETH should be token0 + * @param recenterCaller Address that will be granted recenter access + */ + function setupEnvironmentWithSelfFeeDestination( + bool token0shouldBeWeth, + address recenterCaller + ) + 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)); + + // Set feeDestination to the LM itself — this is the self-referencing mode under test + lm.setFeeDestination(address(lm)); + + harberg.setStakingPool(address(stake)); + harberg.setLiquidityManager(address(lm)); + vm.deal(address(lm), INITIAL_LM_ETH_BALANCE); + + // feeDestination IS address(lm), so prank as lm to grant recenter access + vm.prank(address(lm)); + lm.setRecenterAccess(recenterCaller); + + 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