From c03024fe546e1e2b910f295641fafff79df9d1d8 Mon Sep 17 00:00:00 2001 From: 0x0Louis <107303182+0x0Louis@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:32:23 +0200 Subject: [PATCH 1/2] Fix reentrancy issue --- src/LBHooksBaseRewarder.sol | 8 +- test/LBHooksMCRewarderReentrancy.t.sol | 224 +++++++++++++++++++++++++ 2 files changed, 227 insertions(+), 5 deletions(-) create mode 100644 test/LBHooksMCRewarderReentrancy.t.sol diff --git a/src/LBHooksBaseRewarder.sol b/src/LBHooksBaseRewarder.sol index 3c0118e..233ea54 100644 --- a/src/LBHooksBaseRewarder.sol +++ b/src/LBHooksBaseRewarder.sol @@ -391,18 +391,16 @@ abstract contract LBHooksBaseRewarder is LBBaseHooks, Ownable2StepUpgradeable, C * @param rewards The rewards to claim */ function _claim(address user, uint256[] memory ids, uint256 rewards) internal virtual { - if (rewards == 0) { - _onClaim(user, ids); - } else { + if (rewards > 0) { _totalUnclaimedRewards -= rewards; _unclaimedRewards[user] -= rewards; emit Claim(user, rewards); - _onClaim(user, ids); - TokenHelper.safeTransfer(_getRewardToken(), user, rewards); } + + _onClaim(user, ids); } /** diff --git a/test/LBHooksMCRewarderReentrancy.t.sol b/test/LBHooksMCRewarderReentrancy.t.sol new file mode 100644 index 0000000..dd17e9c --- /dev/null +++ b/test/LBHooksMCRewarderReentrancy.t.sol @@ -0,0 +1,224 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import "./TestHelper.sol"; + +import {ILBPair} from "@lb-protocol/src/interfaces/ILBPair.sol"; + +import "../src/LBHooksMCRewarder.sol"; +import "../src/LBHooksExtraRewarder.sol"; +import "../src/interfaces/ILBHooksBaseRewarder.sol"; + +/** + * @dev Regression tests for the reentrancy issue where a native-paying extra rewarder's + * payout callback reenters the parent MC rewarder while its MOE is still in flight. + * + * The parent settles its MOE (accounting + transfer) before calling the extra rewarder, so + * `balance - _totalUnclaimedRewards` is consistent during any callback. These tests exercise + * both the direct `claim` reentry and the reentry through the unguarded LB hooks. + */ +contract LBHooksMCRewarderReentrancyTest is TestHelper { + LBHooksMCRewarder lbHooks; + LBHooksExtraRewarder lbHooksExtra; + ReentrantClaimer attacker; + + function setUp() public override { + super.setUp(); + + lbHooksManager.setLBHooksParameters( + ILBHooksManager.LBHooksType.MCRewarder, + Hooks.setHooks(hooksParameters, address(new LBHooksMCRewarder(address(lbHooksManager), masterchef, moe))) + ); + lbHooksManager.setLBHooksParameters( + ILBHooksManager.LBHooksType.ExtraRewarder, + Hooks.setHooks(hooksParameters, address(new LBHooksExtraRewarder(address(lbHooksManager)))) + ); + + lbHooks = LBHooksMCRewarder( + payable(address( + lbHooksManager.createLBHooksMCRewarder( + IERC20(address(token0)), IERC20(address(token1)), DEFAULT_BIN_STEP, address(this) + ) + )) + ); + + lbHooksExtra = LBHooksExtraRewarder( + payable(address( + lbHooksManager.createLBHooksExtraRewarder( + IERC20(address(token0)), + IERC20(address(token1)), + DEFAULT_BIN_STEP, + IERC20(address(0)), + address(this) + ) + )) + ); + + vm.label(address(lbHooks), "lbHooksRewarder"); + vm.label(address(lbHooksExtra), "lbHooksExtraRewarder"); + + attacker = new ReentrantClaimer(lbHooks, pair01, ids); + vm.label(address(attacker), "attacker"); + } + + /// @dev Funds the native extra rewarder and seeds the attacker with a 10% liquidity share. + function _setup() internal { + lbHooks.setDeltaBins(0, 1); + lbHooksExtra.setDeltaBins(0, 1); + + vm.deal(address(lbHooksExtra), 100e18); + lbHooksExtra.setRewardPerSecond(1e18, 100); + + _addLiquidity(pair01, address(attacker), DEFAULT_ID, 0, 10e18, 10e18); + _addLiquidity(pair01, bob, DEFAULT_ID, 0, 90e18, 90e18); + + vm.warp(block.timestamp + 10); + } + + function test_HonestClaimWithNativeExtraRewarder() public { + _setup(); + + uint256 pendingMoe = lbHooks.getPendingRewards(address(attacker), ids); + uint256 pendingNative = lbHooksExtra.getPendingRewards(address(attacker), ids); + + assertGt(pendingMoe, 0, "test_HonestClaimWithNativeExtraRewarder::1"); + assertGt(pendingNative, 0, "test_HonestClaimWithNativeExtraRewarder::2"); + + attacker.setMode(ReentrantClaimer.Mode.None); + attacker.claim(); + + assertApproxEqAbs(moe.balanceOf(address(attacker)), pendingMoe, 1, "test_HonestClaimWithNativeExtraRewarder::3"); + assertApproxEqAbs(address(attacker).balance, pendingNative, 1, "test_HonestClaimWithNativeExtraRewarder::4"); + + _assertBobUnharmed(); + } + + function test_ReentrantClaimDoesNotInflateMoe() public { + _setup(); + + uint256 pendingMoe = lbHooks.getPendingRewards(address(attacker), ids); + uint256 pendingNative = lbHooksExtra.getPendingRewards(address(attacker), ids); + + attacker.setMode(ReentrantClaimer.Mode.ReenterClaim); + attacker.claim(); + + // The reentrant claim must not pay the attacker more than its honest share of either token. + assertApproxEqAbs(moe.balanceOf(address(attacker)), pendingMoe, 1, "test_ReentrantClaimDoesNotInflateMoe::1"); + assertApproxEqAbs(address(attacker).balance, pendingNative, 1, "test_ReentrantClaimDoesNotInflateMoe::2"); + + // No phantom pending is observable mid-claim: the MOE is settled before the callback fires. + assertApproxEqAbs(attacker.pendingDuringCallback(), 0, 1, "test_ReentrantClaimDoesNotInflateMoe::3"); + + _assertBobUnharmed(); + } + + function test_ReentrantClaimViaHookDoesNotCorruptAccounting() public { + _setup(); + + uint256 pendingMoe = lbHooks.getPendingRewards(address(attacker), ids); + uint256 pendingNative = lbHooksExtra.getPendingRewards(address(attacker), ids); + + // Reenter through an LB hook (self batchTransferFrom) rather than `claim` directly. + // This path is not covered by a `claim` reentrancy guard, so only the settle-before-call + // ordering protects it. swap/mint/burn reach the same `_updateAccruedRewardsPerShare`. + attacker.setMode(ReentrantClaimer.Mode.ReenterHook); + attacker.claim(); + + assertApproxEqAbs( + moe.balanceOf(address(attacker)), pendingMoe, 1, "test_ReentrantClaimViaHookDoesNotCorruptAccounting::1" + ); + assertApproxEqAbs( + address(attacker).balance, pendingNative, 1, "test_ReentrantClaimViaHookDoesNotCorruptAccounting::2" + ); + assertApproxEqAbs( + attacker.pendingDuringCallback(), 0, 1, "test_ReentrantClaimViaHookDoesNotCorruptAccounting::3" + ); + + // On the vulnerable ordering this desyncs `_totalUnclaimedRewards` above the balance and + // the next pending computation underflows and reverts. + _assertBobUnharmed(); + } + + function test_ReentrantClaimOnlyExtraRewarderEmptyMainPool() public { + // Leave the main pool range empty (default deltaBins 0,0) so the attacker has no MOE to claim, + // exercising the `rewards == 0` branch while the native extra rewarder still fires the callback. + lbHooksExtra.setDeltaBins(0, 1); + + vm.deal(address(lbHooksExtra), 100e18); + lbHooksExtra.setRewardPerSecond(1e18, 100); + + _addLiquidity(pair01, address(attacker), DEFAULT_ID, 0, 10e18, 10e18); + _addLiquidity(pair01, bob, DEFAULT_ID, 0, 90e18, 90e18); + + vm.warp(block.timestamp + 10); + + uint256 pendingNative = lbHooksExtra.getPendingRewards(address(attacker), ids); + assertGt(pendingNative, 0, "test_ReentrantClaimOnlyExtraRewarderEmptyMainPool::1"); + + attacker.setMode(ReentrantClaimer.Mode.ReenterClaim); + attacker.claim(); + + assertEq(moe.balanceOf(address(attacker)), 0, "test_ReentrantClaimOnlyExtraRewarderEmptyMainPool::2"); + assertApproxEqAbs( + address(attacker).balance, pendingNative, 1, "test_ReentrantClaimOnlyExtraRewarderEmptyMainPool::3" + ); + } + + /// @dev Bob keeps his fair share and the accounting stays readable/claimable after the attack. + function _assertBobUnharmed() internal { + uint256 bobPending = lbHooks.getPendingRewards(bob, ids); + assertApproxEqRel(bobPending, 9e18, 1e14, "_assertBobUnharmed::1"); + + vm.prank(bob); + lbHooks.claim(bob, ids); + + assertApproxEqAbs(moe.balanceOf(bob), bobPending, 1, "_assertBobUnharmed::2"); + } +} + +contract ReentrantClaimer { + enum Mode { + None, + ReenterClaim, + ReenterHook + } + + ILBHooksBaseRewarder public immutable lbHooks; + ILBPair public immutable pair; + + uint256[] internal _ids; + Mode internal _mode; + bool internal _entered; + + /// @dev `getPendingRewards` for this contract, read from inside the payout callback. + uint256 public pendingDuringCallback; + + constructor(ILBHooksBaseRewarder lbHooks_, ILBPair pair_, uint256[] memory ids_) { + lbHooks = lbHooks_; + pair = pair_; + _ids = ids_; + } + + function setMode(Mode mode) external { + _mode = mode; + } + + function claim() external { + _entered = false; + lbHooks.claim(address(this), _ids); + } + + receive() external payable { + if (_entered) return; + _entered = true; + + pendingDuringCallback = lbHooks.getPendingRewards(address(this), _ids); + + if (_mode == Mode.ReenterClaim) { + lbHooks.claim(address(this), _ids); + } else if (_mode == Mode.ReenterHook) { + pair.batchTransferFrom(address(this), address(this), _ids, new uint256[](_ids.length)); + } + } +} From 99e0ee9174869bcdfa8acf53e6f68a3a6522188f Mon Sep 17 00:00:00 2001 From: 0x0Louis <107303182+0x0Louis@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:32:39 +0200 Subject: [PATCH 2/2] run fmt --- src/LBHooksBaseRewarder.sol | 10 ++++------ test/LBHooksBaseRewarder.t.sol | 18 ++++++------------ test/LBHooksExtraRewarder.t.sol | 12 ++++-------- test/LBHooksLens.t.sol | 12 ++++-------- test/LBHooksManager.t.sol | 6 ++---- test/LBHooksRewarder.t.sol | 6 ++---- test/LBHooksSimpleRewarder.t.sol | 12 ++++-------- 7 files changed, 26 insertions(+), 50 deletions(-) diff --git a/src/LBHooksBaseRewarder.sol b/src/LBHooksBaseRewarder.sol index 233ea54..09b03a5 100644 --- a/src/LBHooksBaseRewarder.sol +++ b/src/LBHooksBaseRewarder.sol @@ -152,9 +152,8 @@ abstract contract LBHooksBaseRewarder is LBBaseHooks, Ownable2StepUpgradeable, C if (accRewardsPerShareX64 > userAccRewardsPerShareX64) { unchecked { - pendingRewards += (accRewardsPerShareX64 - userAccRewardsPerShareX64).mulShiftRoundDown( - balanceX64, OFFSET_PRECISION - ); + pendingRewards += (accRewardsPerShareX64 - userAccRewardsPerShareX64) + .mulShiftRoundDown(balanceX64, OFFSET_PRECISION); } } } @@ -372,9 +371,8 @@ abstract contract LBHooksBaseRewarder is LBBaseHooks, Ownable2StepUpgradeable, C if (accRewardsPerShareX64 > userAccRewardsPerShareX64) { unchecked { - pendingRewards += (accRewardsPerShareX64 - userAccRewardsPerShareX64).mulShiftRoundDown( - balanceX64, OFFSET_PRECISION - ); + pendingRewards += (accRewardsPerShareX64 - userAccRewardsPerShareX64) + .mulShiftRoundDown(balanceX64, OFFSET_PRECISION); } bin.userAccRewardsPerShareX64[to] = accRewardsPerShareX64; diff --git a/test/LBHooksBaseRewarder.t.sol b/test/LBHooksBaseRewarder.t.sol index dca0c53..d0b385e 100644 --- a/test/LBHooksBaseRewarder.t.sol +++ b/test/LBHooksBaseRewarder.t.sol @@ -18,14 +18,12 @@ contract LBHooksBaseRewarderTest is TestHelper { hooksParameters = Hooks.setHooks(hooksParameters, address(new MockLBHooksRewarder())); hooks = MockLBHooksRewarder( - payable( - _createAndSetLBHooks( + payable(_createAndSetLBHooks( pair01, hooksParameters, abi.encodePacked(address(pair01), address(rewardToken01)), abi.encode(address(this)) - ) - ) + )) ); vm.label(address(hooks), "hooks"); @@ -191,11 +189,9 @@ contract LBHooksBaseRewarderTest is TestHelper { ); hooks = MockLBHooksRewarder( - payable( - _createAndSetLBHooks( + payable(_createAndSetLBHooks( pair01, hooksParameters, abi.encodePacked(pair01, address(0)), abi.encode(address(this)) - ) - ) + )) ); (s, d) = address(hooks).call{value: 1e18}(data); @@ -213,11 +209,9 @@ contract LBHooksBaseRewarderTest is TestHelper { function test_NativeReward() public { hooks = MockLBHooksRewarder( - payable( - _createAndSetLBHooks( + payable(_createAndSetLBHooks( pair01, hooksParameters, abi.encodePacked(pair01, address(0)), abi.encode(address(this)) - ) - ) + )) ); deal(address(hooks), 1e18); diff --git a/test/LBHooksExtraRewarder.t.sol b/test/LBHooksExtraRewarder.t.sol index 708cac3..b2995e8 100644 --- a/test/LBHooksExtraRewarder.t.sol +++ b/test/LBHooksExtraRewarder.t.sol @@ -26,18 +26,15 @@ contract LBHooksExtraRewarderTest is TestHelper { ); lbHooks = LBHooksMCRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksMCRewarder( IERC20(address(token0)), IERC20(address(token1)), DEFAULT_BIN_STEP, address(this) ) - ) - ) + )) ); lbHooksExtra = LBHooksExtraRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksExtraRewarder( IERC20(address(token0)), IERC20(address(token1)), @@ -45,8 +42,7 @@ contract LBHooksExtraRewarderTest is TestHelper { IERC20(address(rewardToken01)), address(this) ) - ) - ) + )) ); vm.label(address(lbHooks), "lbHooksRewarder"); diff --git a/test/LBHooksLens.t.sol b/test/LBHooksLens.t.sol index 09d5f33..e32ecf9 100644 --- a/test/LBHooksLens.t.sol +++ b/test/LBHooksLens.t.sol @@ -27,18 +27,15 @@ contract LBHooksLensTest is TestHelper { ); lbHooks = LBHooksMCRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksMCRewarder( IERC20(address(token0)), IERC20(address(token1)), DEFAULT_BIN_STEP, address(this) ) - ) - ) + )) ); lbHooksExtra = LBHooksExtraRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksExtraRewarder( IERC20(address(token0)), IERC20(address(token1)), @@ -46,8 +43,7 @@ contract LBHooksLensTest is TestHelper { IERC20(address(rewardToken01)), address(this) ) - ) - ) + )) ); lbHooksLens = new LBHooksLens(address(lbHooksManager), address(masterchef)); diff --git a/test/LBHooksManager.t.sol b/test/LBHooksManager.t.sol index d48c02e..7177b70 100644 --- a/test/LBHooksManager.t.sol +++ b/test/LBHooksManager.t.sol @@ -97,13 +97,11 @@ contract LBHooksManagerTest is TestHelper { ); LBHooksMCRewarder lbHooks = LBHooksMCRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksMCRewarder( IERC20(address(token0)), IERC20(address(token1)), DEFAULT_BIN_STEP, address(this) ) - ) - ) + )) ); assertEq( diff --git a/test/LBHooksRewarder.t.sol b/test/LBHooksRewarder.t.sol index 3be73d7..85ace31 100644 --- a/test/LBHooksRewarder.t.sol +++ b/test/LBHooksRewarder.t.sol @@ -19,13 +19,11 @@ contract LBHooksRewarderTest is TestHelper { lbHooksManager.setLBHooksParameters(ILBHooksManager.LBHooksType.MCRewarder, hooksParameters); lbHooks = LBHooksMCRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksMCRewarder( IERC20(address(token0)), IERC20(address(token1)), DEFAULT_BIN_STEP, address(this) ) - ) - ) + )) ); vm.label(address(lbHooks), "lbHooks"); diff --git a/test/LBHooksSimpleRewarder.t.sol b/test/LBHooksSimpleRewarder.t.sol index 0d519d7..6df9f17 100644 --- a/test/LBHooksSimpleRewarder.t.sol +++ b/test/LBHooksSimpleRewarder.t.sol @@ -26,8 +26,7 @@ contract LBHooksSimpleRewarderTest is TestHelper { ); lbHooks = LBHooksSimpleRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksSimpleRewarder( IERC20(address(token0)), IERC20(address(token1)), @@ -35,13 +34,11 @@ contract LBHooksSimpleRewarderTest is TestHelper { IERC20(address(rewardToken01)), address(this) ) - ) - ) + )) ); lbHooksExtra = LBHooksExtraRewarder( - payable( - address( + payable(address( lbHooksManager.createLBHooksExtraRewarder( IERC20(address(token0)), IERC20(address(token1)), @@ -49,8 +46,7 @@ contract LBHooksSimpleRewarderTest is TestHelper { IERC20(address(rewardToken02)), address(this) ) - ) - ) + )) ); vm.label(address(lbHooks), "lbHooksRewarder");