Skip to content

fix(consensus): screen never-staked slash entries, close the settlement window, unbrick recipient-facing payouts - #169

Merged
Huwonk merged 1 commit into
masterfrom
fix/consensus-audit-2026-08-12
Aug 17, 2026
Merged

fix(consensus): screen never-staked slash entries, close the settlement window, unbrick recipient-facing payouts#169
Huwonk merged 1 commit into
masterfrom
fix/consensus-audit-2026-08-12

Conversation

@Huwonk

@Huwonk Huwonk commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes three findings from the 2026-08-12 internal audit, plus two more that a review pass over those fixes turned up. All five are pre-existing; none were introduced by #165.

1. applySlashes can halt the chain or drain collateral on an unregistered entry (high, latent)

applySlashes accepted a slash entry naming an address that never staked. Nothing on the path caught it. isRetired returns false for an Undefined address by design (it never existed, so there is nothing to have retired), and a zero balance fails the balance > amount test for every amount including zero, so the entry fell through to _consensusBurn:

  • An address that never held a ConsensusNFT reverted _burn with ERC721NonexistentToken. applySlashes runs in the closing block, so that revert means the block cannot be produced.
  • An address that governance whitelisted but that never staked burned successfully, and shipped a full genesis stake amount to Issuance for an address that deposited nothing, drawing on the collateral backing every other validator. Repeat it and the registry stops backing the sum of balances[], at which point the Issuance consolidation inside concludeEpoch reverts and halts the chain anyway.

Both reproduce against the unfixed code, as ERC721NonexistentToken and as a silent balance drain respectively. The entries are now screened by status.

This is latent today because slashing is disabled for the MNO pilot and no slash producer exists in the node (epoch_boundary_slashes() is a hardcoded empty vec). It is a gate on enabling slashing rather than an active risk. applySlashes([]) is nevertheless invoked unconditionally every epoch, and the caller is documented to do no membership filtering, so the contract is the right place for the check.

applyIncentives needs no matching guard: capping weight at the outstanding balance already leaves a zero-balance entry weightless. We wrote that guard, measured it at 432 gas per rewardee on the boundary path, and backed it out; the reason it is safe is now recorded in a comment instead.

mint also now stamps the address onto the still-Undefined record. Burning a whitelisted validator before it ever staked ran _retire against a record whose validatorAddress was still zero, so ValidatorRetired named an address no log-based monitor could attribute.

2. Stake-decrease settlement pushed value before recording the version (medium)

The immediate Staked lane of requestStakeVersionChange debited the balance and pushed the surplus before writing the new stake version. Inside that window the validator read as slashed by exactly the surplus: the balance was already newStakeAmount while the recorded version still resolved to oldStakeAmount. A recipient reentering topUpSlashedStake passed its StakeNotSlashed guard, restored stake it never lost, and left the difference to be booked as rewards applyIncentives never issued.

EIP-7702 is what makes this reachable. For an undelegated validator the recipient is the validator address itself, which before Prague was structurally incapable of reentering.

The version write is hoisted above the settlement, and topUpSlashedStake and activate are additionally nonReentrant. The two guards are not redundant: with only nonReentrant in place, the test that pins the mid-push read still fails.

3. A delegator could permanently strand a validator's stake (medium)

_unstake pushed to the recipient with full gas and no fallback. For a delegated validator that recipient is the delegator, an address the validator can neither change nor remove (delegations clears only inside _burnConsensusNFT). A delegator that was a plain EOA when the delegation was formed can attach a reverting EIP-7702 handler afterwards, bricking the withdrawal permanently and leaving governance only burn, which confiscates the stake rather than returning it. acceptRewardShortfall does not help: it caps the rewards leg and never bypasses the push.

The payout now uses the same bounded stipend as the escrow returns and falls back to a pull-based credit. The credit splits along its two funding sources, stake on the registry and rewards on Issuance, so there is a new claimableRewards mapping alongside claimableRefunds, and claimRefund pays both in the single transfer the push would have made.

Two more from reviewing the above

  • The immediate Staked stake-decrease settlement was the last recipient-facing push in a user operation with no fallback. The same hostile delegator that could no longer strand a withdrawal could still block a version change through it. It now routes through _settleValue, so both settlement lanes for that operation behave identically on a recipient that stops accepting value.
  • claimRefund could complete as a transfer of zero when the only credit left was a reward leg and Issuance had run dry, emitting RefundClaimed(x, 0) for a claim that delivered nothing. It reverts now.

Notes for review

  • Storage. claimableRewards is appended after topUpAuthorityRequired, the tail of the flattened layout, so every existing slot is preserved for an in-place upgrade.
  • acceptRewardShortfall is preserved deliberately. The credit fallback would otherwise have swallowed an underfunded-Issuance revert and silently deferred instead of reverting, which two existing tests pin against. _unstake now surfaces that case explicitly before the push, reproducing Issuance's exact revert args, so accepting the shortfall remains the only way to settle for less than the full amount.
  • A dry reward pool defers, it does not forfeit or block. claimRefund caps the reward leg at Issuance's balance and leaves the remainder credited. Without that, an empty pool would hold up the registry-backed stake leg too.
  • Backing. The registry keeps the stake leg when a push fails (the inner call reverts, so the value never leaves), and Issuance keeps the reward leg. Each credit is backed where it already sits; a test asserts both balances are unmoved.
  • Gas. Boundary costs are unchanged: applyIncentives 356,185 for 100 rewardees, concludeEpoch 33,912,995 for the 1000-entry settlement wave, matching the ceiling documented in invariants.md.
  • Accepted residual. claimStakeRewards still reverts on a recipient that rejects value. It strands nothing: the rewards stay in the ledger and come out through unstake, which now credits.

Testing

235 consensus tests pass at the default 250-run fuzz depth, 335 across the repo. Every new test was confirmed to fail against the unfixed code first:

  • the two applySlashes variants, reproducing the halt and the drain
  • the mid-push version read, and the reentrant topUpSlashedStake exploit
  • the stranded delegated withdrawal, and the stranded stake-decrease surplus
  • the partial reward-leg claim against a dry pool

invariants.md is updated for all five, and artifacts/ConsensusRegistry.json is regenerated with make update-artifacts. The other five artifacts came out with identical runtime bytecode and only source-map churn, so we left them at master.

…nt window, unbrick recipient-facing payouts

Three findings from the 2026-08-12 internal audit, all pre-existing.

applySlashes accepted an entry for an address that never staked. `isRetired`
returns false for an `Undefined` address by design, and a zero balance fails
the `balance > amount` test for every amount including zero, so the entry fell
through to the ejection branch. An address that never held a ConsensusNFT
reverted `_burn` with ERC721NonexistentToken, and since applySlashes runs in
the closing block that revert means the block cannot be produced. An address
that was whitelisted but never staked burned successfully and shipped a full
genesis stake amount to Issuance, drawn from the collateral backing every other
validator, with no balance of its own to draw on. The system caller does no
membership filtering, so both are screened by status here.

applyIncentives needs no matching guard: capping weight at the outstanding
balance already leaves a zero-balance entry weightless. Screening by status
there measured 432 gas per rewardee on the boundary path, so the reason it is
safe is recorded in a comment rather than paid for twice.

`mint` now stamps the address onto the still-`Undefined` record. Burning a
whitelisted validator before it ever stakes ran `_retire` against a record
whose `validatorAddress` was zero, so ValidatorRetired named an address no
log-based monitor could attribute.

The immediate `Staked` lane of requestStakeVersionChange debited the balance
and pushed the surplus before recording the new stake version. Inside that
window the validator read as slashed by exactly the surplus, and a recipient
reentering topUpSlashedStake could restore stake it never lost and book the
difference as rewards applyIncentives never issued. EIP-7702 is what makes this
reachable: the recipient of an undelegated validator is the validator address
itself, which since Prague can carry a delegation designator and execute code
on receiving value. The version write is hoisted above the settlement, and
topUpSlashedStake and activate are additionally nonReentrant.

The unstake payout pushed to the recipient with full gas and no fallback. For a
delegated validator that recipient is the delegator, an address the validator
can neither change nor remove, and a delegator that was a plain EOA when the
delegation was formed can attach a reverting 7702 handler afterwards. That
stranded the stake permanently, leaving governance only `burn`, which
confiscates rather than returns. The payout now pushes with the same bounded
stipend the escrow returns use and falls back to a pull-based credit. The
credit splits along its two funding sources, stake on the registry and rewards
on Issuance, and claimRefund pays both in the single transfer the push would
have made, capping the reward leg at what Issuance holds so a dry reward pool
defers that leg rather than blocking the stake leg. An uncoverable reward leg
still reverts unstake outright, so acceptRewardShortfall remains the only way
to settle for less than the full amount.

A review pass over the above turned up two more, both fixed here. The immediate
`Staked` stake-decrease settlement was the last recipient-facing push in a user
operation with no fallback, so the same hostile delegator that could no longer
strand a withdrawal could still block a version change; it now routes through
`_settleValue`, matching what the boundary lane does for the identical
settlement. And claimRefund could complete as a transfer of zero when the only
credit left was a reward leg and Issuance had run dry, reporting a claim that
delivered nothing; it now reverts instead.
@Huwonk
Huwonk requested a review from grantkee August 13, 2026 03:07
@Huwonk Huwonk self-assigned this Aug 13, 2026
@Huwonk
Huwonk requested a review from chasebrownn August 13, 2026 03:07

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

LGTM @Huwonk - good work

@Huwonk
Huwonk merged commit 0fb6b01 into master Aug 17, 2026
8 checks 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.

3 participants