fix(staking): preserve fractional rewards across forced settlements - #636
Open
KrickmanC wants to merge 1 commit into
Open
fix(staking): preserve fractional rewards across forced settlements#636KrickmanC wants to merge 1 commit into
KrickmanC wants to merge 1 commit into
Conversation
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
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.
fix(staking): preserve fractional rewards across forced settlements
Summary
This change preserves sub-wei staking rewards across repeated settlement events.
The current reward accounting rounds each settlement down to whole wei:
The user's
userIndexis then advanced even when the calculatedpendingvalue is zero. Any fractional numerator belowPRECISIONis therefore discarded permanently.This becomes externally triggerable because ordinary cSSV transfers settle both the sender and the recipient through
onCSSVTransfer(). A third party can send a minimal cSSV amount to another account and force that account to settle without its approval.The proposed implementation carries the fractional numerator forward between settlements so that reward outcomes remain independent of settlement frequency.
Problem
A staker's cumulative ETH rewards should depend on:
accEthPerShare.They should not depend on how often another account transfers cSSV to them.
Under the existing implementation, repeated settlements can produce a different result from a single cumulative settlement.
For example, if a user earns
0.9 weiduring each of ten reward periods:Because ERC-20 recipients cannot reject incoming transfers, this difference can be induced by an unrelated cSSV holder.
Root Cause
_settleWithBalance()stores only the whole-wei result of the scaled reward calculation:The remainder:
is not persisted.
The function nevertheless advances:
s.userIndex[user] = idx;As a result, the discarded fraction cannot contribute to future rewards.
Changes
1. Preserve the fractional reward numerator
A new per-user remainder mapping is appended to
StorageStaking:Settlement now combines the previously stored remainder with the new scaled reward:
Only the whole-wei portion is added to
accrued; the fractional numerator is retained for the next settlement.2. Preserve upgradeable storage ordering
The new mapping is appended at the end of
StorageStaking. Existing fields are not reordered or resized.3. Add regression coverage
A regression test reproduces the production transfer ordering:
onCSSVTransfer()before balances change;The test compares:
Both accounts accrue the same final reward after the fix.
Behavioral Impact
The change restores the following accounting invariant:
The fix does not change:
It only prevents per-user fractional reward numerators from being discarded.
Security Impact
Without the remainder carry-forward, a third party can reduce another user's cumulative rewards by repeatedly forcing settlement at adversarial times.
The loss per settlement is bounded to less than one wei, and the sender does not receive the discarded value. This is therefore a low-severity accounting and griefing issue rather than a direct theft vector.
Validation
The PR includes a focused unit regression test for repeated forced settlements.
Recommended validation before merge:
npm install npx hardhat test test/unit/SSVStaking/fractionalRewards.test.ts npm run test:unit npm run buildThe complete upstream test suite was not executed from the current integration environment, so CI results should be treated as the authoritative validation.
Compatibility Considerations
Storage layout
The new mapping is appended to the existing staking storage struct. No existing slot assignments are modified.
Gas
Settlement performs one additional mapping read and one mapping write. This affects staking actions and cSSV transfers that trigger settlement.
Zero-balance accounts
The remainder remains associated with the account even when its cSSV balance becomes zero. This avoids destroying already-earned fractional rewards.
The maintainers may want to confirm whether sub-wei remainder should:
The current implementation preserves it, which is the conservative accounting behavior.
View consistency
previewClaimableEth()should eventually includerewardRemainder[user]in the same scaled calculation so the view result remains fully aligned with settlement behavior.That adjustment is intentionally called out for maintainer review because the current PR is focused on preventing state-level reward loss.
Requested Feedback
Please let me know whether this change is useful for the project and whether carrying the fractional numerator across transfers, unstaking operations, and zero-balance periods matches the intended staking-reward semantics.
Feedback would be particularly useful on:
previewClaimableEth()should include the stored remainder in this PR;