Fix: unstake() strands rewards accrued during 7-day cooldown#5
Open
philpof102-svg wants to merge 2 commits into
Open
Fix: unstake() strands rewards accrued during 7-day cooldown#5philpof102-svg wants to merge 2 commits into
philpof102-svg wants to merge 2 commits into
Conversation
…stranded rewards during cooldown
This was referenced Jun 3, 2026
Gitlawb#5) GitlawbStaking.unstake() previously paid out only the unstaked principal. Pending rewards earned during the cooldown (or any prior un-claimed period) were left sitting in info.pendingRewards. For partial unstakers this is a UX annoyance — they can still call claimRewards(). For full-exit stakers (amount == info.amount) it is a real loss in practice: the user thinks unstake() concluded their position and walks away, leaving rewards parked in storage they'll never come back for. Fix : mirror GitlawbNodeStaking.unstake() — pay stake + pendingRewards in a single transfer. Wipes pendingRewards in storage; emits RewardsClaimed when rewards > 0 so off-chain accounting stays consistent with claimRewards(). Behavior change for downstream consumers : - Token.Transfer event amount changes from (unstakeAmount) to (unstakeAmount + pendingRewards). - An extra RewardsClaimed event may fire from unstake(). - claimRewards() can still be called pre-unstake to split the two payouts if a user prefers. Backward compatible at the storage level — no struct field changes. Related to PR Gitlawb#5 bug report.
Author
|
Upgraded with merge-ready fix ( // in unstake(), after clearing the unstake request
uint256 rewards = info.pendingRewards;
info.pendingRewards = 0;
uint256 payout = amount + rewards;
bool ok = token.transfer(msg.sender, payout);
if (!ok) revert TransferFailed();
emit Unstaked(msg.sender, amount);
if (rewards > 0) emit RewardsClaimed(msg.sender, rewards);Mirrors No struct changes. Token.Transfer event amount changes from |
This was referenced Jun 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Severity: MEDIUM — legitimately-earned rewards forfeited (funds not stealable).
unstake() does NOT call _harvest() before decrementing the stake. During the 7-day cooldown, depositRevenue() advances accRewardPerShare while the staker's amount is still active. On unstake the rewardDebt is reset, and the cooldown-period rewards are stranded forever in the contract.
Fix: add _harvest(msg.sender) as first line of unstake(), mirroring stake/requestUnstake/claimRewards.
See BUG_REPORT_unstake_stranded_rewards.md in this PR for full repro.
Note: the 2026-04-20 internal audit marked a 'stranded-rewards bug in _harvest else-branch' as fixed. This PR addresses a DIFFERENT stranded-rewards path not previously identified.
Reporter: @philpof102-svg (operator 0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9 on Base). Happy to write the regression test if the fix is correct.