Skip to content

fix(beacon): bound beacon slash to on-beacon principal + make service slash punitive#183

Merged
drewstone merged 1 commit into
mainfrom
fix/beacon-slashing-overslash-and-drain
Jun 19, 2026
Merged

fix(beacon): bound beacon slash to on-beacon principal + make service slash punitive#183
drewstone merged 1 commit into
mainfrom
fix/beacon-slashing-overslash-and-drain

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

Remediates the four beacon-slashing findings (BCN-001..004) plus two further defects found in the same path — one critical (a working PoC was already staged in the tree), one production-breaking. All 329 beacon/audit/security tests pass.

Findings from the report

  • BCN-001 (High) — over-slash via parked ETH. L2SlashingConnector derived the slash base from totalAssetsOf, which includes parked execution-layer ETH (tips, partial withdrawals, exited principal already in pod custody) that cannot be beacon-slashed. A 50% beacon slash on a 32-ETH validator with 968 ETH parked saturated slashBps at 10000 (100% of L2 stake) vs the ~16-ETH real loss. New _podBeaconPrincipal subtracts the pod's parked tally (withdrawableRestakedExecutionLayerGwei) so the slash reflects only on-beacon principal — now 1600 bps, not 10000. Applied to all three call sites (_propagateBeaconSlashing, getPendingSlashAmount, estimatePropagationFee).
  • BCN-002 (Low) — zero-address oracle. setSlashingOracle now rejects address(0) (mirrors transferOwnership).
  • BCN-003 (Low) — unrestricted recoverTokens. Now nonReentrant + rejects a zero recipient, with a documented invariant for when protocol-accounted ERC20s are ever routed into pods.
  • BCN-004 (Info) — arbitrary pod→operator registration. registerPodOperator / batchRegisterPodOperators now validate against the PodManager: the pod must have a known owner and the operator must be registered.

Further defects fixed (same contract path)

  • Critical — slashed principal drainable. After a service slash, completeUndelegation burned escrow beacon shares (dropping totalAssetsOf) while the slashed ETH stayed physically in the pod, so the owner drained it via withdrawNonBeaconChainEth as fake "non-beacon surplus" and recovered the full 32 ETH despite a 50% slash. The manager now reports burned principal to the pod (recordSlashedPrincipalRetained), and withdrawNonBeaconChainEth floors at totalAssetsOf + slashedPrincipalRetainedWei. The slash is now punitive (owner recovers 16.5/32 ETH; the rest is stranded and unextractable).
  • Production-breaking — batch slashing silently no-ops. batchPropagateBeaconSlashing forwarded {value: 0} to each self-call, so with any non-zero bridge fee (OP-Stack/Arbitrum L1→L2) every propagation reverted InsufficientFee, got swallowed by try/catch, and the batch advanced zero slashes. Each self-call is now funded from the call balance (self-refunding between iterations), with the remainder swept back to the caller.

Tests

  • Rewrote both staged PoCs as passing regression tests (BCN001.t.sol, PoCNonBeaconDrainAfterSlash.t.sol).
  • Added BatchPropagateBeaconSlashingFee.t.sol and targeted regressions for BCN-002/003/004.
  • Updated mock-based connector tests (CrossChainSlashingTest, audit BeaconL2) for the new registration validation.
  • 329 beacon + audit + security tests green, 0 failures.

Note: the full ~1855-test suite was not run locally — forge test with no path filter compiles the whole via-IR graph and OOM-kills this box. Changes are confined to src/beacon/*; the only non-beacon consumer (PectraValidatorCapTest) is included in the green run. CI's swap/cache setup can run the full suite.

… slash punitive

Remediates the gist findings (BCN-001..004) plus two further defects found in the
same beacon-slashing path:

- BCN-001 (High): L2SlashingConnector derived the slash base from
  totalAssetsOf, which includes parked execution-layer ETH (tips, partial
  withdrawals, exited principal in pod custody) that cannot be beacon-slashed.
  A 50% beacon slash on 32 ETH + 968 ETH parked saturated slashBps at 10000
  (100% of L2 stake). New _podBeaconPrincipal subtracts the pod's parked tally
  so the slash reflects only on-beacon principal (now 1600 bps, not 10000).

- BCN-002 (Low): setSlashingOracle now rejects address(0).

- BCN-003 (Low): recoverTokens is nonReentrant and rejects a zero recipient.

- BCN-004 (Info): registerPodOperator/batchRegisterPodOperators validate the
  pod has a known owner and the operator is registered with the PodManager,
  closing the arbitrary pod->operator mapping amplification.

- Critical (drain of slashed principal): after a service slash,
  completeUndelegation burned escrow beacon shares (dropping totalAssetsOf)
  while the slashed ETH stayed physically in the pod, so the owner drained it
  via withdrawNonBeaconChainEth and recovered the full principal despite the
  slash. The manager now reports burned principal to the pod
  (recordSlashedPrincipalRetained) and withdrawNonBeaconChainEth floors at
  totalAssetsOf + slashedPrincipalRetainedWei. Slash is now punitive.

- Batch fee bug: batchPropagateBeaconSlashing forwarded {value: 0} to each
  self-call, so with a non-zero bridge fee every propagation reverted
  InsufficientFee, was swallowed by try/catch, and the batch advanced zero
  slashes. Each self-call is now funded from the call balance (self-refunding)
  with the remainder swept back to the caller.

Adds regression tests for every fix; updates mock-based connector tests for the
new registration validation. 329 beacon/audit/security tests green.

@tangletools tangletools 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.

✅ Auto-approved PR — f58b0e80

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-19T10:48:54Z

@tangletools tangletools 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.

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 81.3s (2 bridge agents)
Total 81.3s

💰 Value — sound

Remediates 6 beacon-slashing defects — bounds slashing to on-beacon principal, makes service slash punitive, fixes a production-breaking batch fee bug, and hardens three low-severity validation gaps — all in the grain of the existing pod↔manager architecture.

  • What it does: Fixes six defects in the beacon slashing path: (1) corrects the slash base from totalAssets (which includes parked ETH that can't be beacon-slashed) to on-beacon principal only via a new _podBeaconPrincipal that subtracts withdrawableRestakedExecutionLayerGwei; (2) closes a drain where slashed beacon principal could be extracted as fake "non-beacon surplus" by having the manager report burned
  • Goals it achieves: Eliminates economic over-slash from parked execution-layer ETH inflating the slash base; makes service slashing truly punitive so slashed principal cannot be recovered; makes batch beacon slash propagation functional in production with non-zero bridge fees; hardens the oracle setter, token recovery, and pod-registration entry points against misconfiguration and abuse.
  • Assessment: Each fix addresses a real, independently-verified defect (one with a staged PoC, one production-breaking). The implementation stays within the codebase's established architecture: _podBeaconPrincipal reuses existing pod state (withdrawableRestakedExecutionLayerGwei at ValidatorPod.sol:74) that was already accurately maintained by checkpoint logic; the recordSlashedPrincipalRetained ↔ `slashe
  • Better / existing approach: none — this is the right approach. The _podBeaconPrincipal leverage of existing pod-resident withdrawableRestakedExecutionLayerGwei (ValidatorPod.sol:74) is the correct single-source-of-truth for parked ETH. The recordSlashedPrincipalRetainedslashedPrincipalRetainedWei floor addition is the minimal extension to close the drain without refactoring the broader withdrawal flow. No existing
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 1

🎯 Usefulness — sound

Six fixes across three contracts — corrects over-slash from parked ETH, makes service slash punitive (non-drainable), fixes batch fee-forwarding silent failure, and hardens three admin paths — all wired through existing call sites the deploy script already exercises.

  • Integration: Every new function has an existing caller in the same PR: _podBeaconPrincipal is called at 3 sites in L2SlashingConnector (L2SlashingConnector.sol:302,395,444) — the slashing oracle already drives propagateBeaconSlashing per commit history; recordSlashedPrincipalRetained is called from ValidatorPodManager.completeUndelegation at ValidatorPodManager.sol:753 down the existing undelegation
  • Fit with existing patterns: The _podBeaconPrincipal subtraction of parked ETH mirrors the existing _newlyWithdrawableGwei/_finalizeCheckpoint pattern that already uses withdrawableRestakedExecutionLayerGwei to avoid double-counting (ValidatorPod.sol:322,460-461). The slashedPrincipalRetainedWei monotonic counter follows the same pattern as the existing withdrawableRestakedExecutionLayerGwei accumulator. The `_val
  • Real-world viability: The batch fee-forwarding fix (L2SlashingConnector.sol:220-238) correctly uses a baseline sentinel to isolate call-attributable value from pre-existing contract funds; the try/catch rollback on caught reverts preserves value integrity per Solidity semantics. The _podBeaconPrincipal underflow guard (totalAssets > parkedWei ? totalAssets - parkedWei : 0) correctly degrades to zero-slash when al
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added test/beacon/BCN001.t.sol

  •    // 32 ETH on-beacon principal + 968 ETH parked execution-layer ETH = 1000 ETH total.
    

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260619T105215Z

@drewstone drewstone merged commit 17528ae into main Jun 19, 2026
1 check passed
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.

2 participants