Skip to content

fix: correct queued slash share accounting - #1759

Open
elhajin wants to merge 14 commits into
mainfrom
fix/rounding-fix
Open

elhajin wants to merge 14 commits into
mainfrom
fix/rounding-fix

Conversation

@elhajin

@elhajin elhajin commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Motivation:

An operator with queued withdrawals could be over-slashed: the protocol burns or redistributes more shares than the withdrawal queue actually backs. The over-count is wei-scale (rounding slack), but it grows with the number of queued stakers.

The root cause is a rounding mismatch between how queued-slashable shares are recorded and how they are later consumed:

  • At queue time, _removeSharesAndQueueWithdrawal recorded the raw scaled value scaleForQueueWithdrawal = floor(depositShares · scalingFactor / WAD) — a single floor.
  • At slash time, _getSlashableSharesInQueue re-multiplies the cumulative recorded value by the operator's magnitude to derive slashable shares.

Because the recorded value skipped the magnitude round-trip, two rounding effects stack. First, per-staker slack: shares that don't divide cleanly against a fractional magnitude round up relative to the backing actually removed from the operator. Second, sum-then-floor vs. floor-then-sum: the queued figure floors once over the cumulative total, while the backing removed is a sum of per-staker floors, and floor(Σ xᵢ) ≥ Σ floor(xᵢ). The recorded amount therefore drifts above the backing removed, and that inflated figure flows straight into totalDepositSharesToSlashincreaseBurnOrRedistributableShares.

Modifications:

  • DelegationManager._removeSharesAndQueueWithdrawal: record the magnitude-round-tripped value withdrawableShares.divWad(slashingFactor) (with slashingFactor == 0 → 0) instead of the raw scaled shares. This ties the recorded amount to the backing actually removed from the operator via _decreaseDelegation, so re-multiplying by magnitude at slash time cannot exceed that backing by construction.
  • Tests:
    • src/test/integration/tests/QueueSlashAccounting.t.sol — two fuzz tests covering deposit→delegate→slash→queue→slash and slash→deposit→queue→redelegate→slash→complete-as-shares, asserting queueSlashable ≤ removedFromOperator (LST).
    • DelegationUnit.t.sol (DelegationManagerUnitTests_slashingShares), including test_slashOperatorShares_BeaconQueueSharesAreBackedAfterCombinedRounding, which exercises the beacon-chain path with a real beaconChainSlashingFactor decrease.

Result:

Recorded queued-slashable shares can no longer exceed the backing removed from the operator. The queue contribution to a slash is bounded by what the queue actually holds, eliminating the over-burn.


For reviewers / auditors:

  • Validation performed: reverting the one-line fix turns the new tests red (LST integration: intermittent 1-wei assertLe failures; beacon-chain unit: queue slashable 1 > 0 removed), and reintroducing it turns them green. The assertions are exact wei bounds — they are the load-bearing guard, so any loosening would let the bug back through.
  • Scope confirmed: _cumulativeScaledSharesHistory has exactly one writer (_addQueuedSlashableShares, one call site), so no sibling path records with the old pattern. The fix is complete, not local.
  • Open for your eyes: realized per-staker impact is direction-confirmed (over-burn) but not quantified end-to-end through completeQueuedWithdrawal — worth pinning a concrete loss figure.
  • Subtle dependency worth a second look: correctness on the beacon-chain path relies on the beaconChainSlashingFactor canceling between the queue-time divide (by full slashingFactor = maxMagnitude · beaconChainSlashingFactor) and the slash-time multiply (by maxMagnitude alone). It cancels today — the beacon test proves it — but the cancellation is implicit, and a future change to how _getSlashableSharesInQueue re-derives operator shares could silently break it. A code comment at that site would help.

@elhajin
elhajin requested a review from a team August 29, 2026 23:19

@antojoseph antojoseph left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants