Skip to content

fix(staking): prevent consensus pubkey squatting via commit-reveal (#580)#86

Open
ByteYue wants to merge 1 commit intomainfrom
fix/580-consensus-pubkey-squatting-commit-reveal
Open

fix(staking): prevent consensus pubkey squatting via commit-reveal (#580)#86
ByteYue wants to merge 1 commit intomainfrom
fix/580-consensus-pubkey-squatting-commit-reveal

Conversation

@ByteYue
Copy link
Copy Markdown
Contributor

@ByteYue ByteYue commented Apr 21, 2026

Summary

  • Adds a commit-reveal precondition to registerValidator and rotateConsensusKey so a caller must pre-commit keccak256(pubkey, stakePool, chainid) in a strictly earlier block before revealing the pubkey on-chain.
  • Closes the mempool front-run squatting vector identified in gravity-audit#580: the BLS PoP precompile only proves someone holds sk, not this caller holds sk, so combined with Gravity's global _pubkeyToValidator uniqueness map an observer could replay a victim's (pubkey, pop) into their own pool and DoS the victim.
  • Updates affected tests (commit+roll wrapped around existing register/rotate call sites) and adds 8 new test_audit580_* tests that exercise the defense directly, including an end-to-end front-run race scenario.

Why this shape (and not just "bind PoP to stakePool")

The Gravity PoP precompile (gravity-reth/crates/pipe-exec-layer-ext-v2/execute/src/bls_precompile.rs) is hard-coded to 144-byte input (pubkey || pop) with an empty augmentation, signing only pubkey under the IETF POP DST. There is no way to bind the PoP to (operator, stakePool, chainid) without a coordinated change to the precompile contract and the validator keygen toolchain. Commit-reveal moves the binding entirely into the contract layer, is trivially verifiable, and requires no cross-repo change.

Aptos does not need this fix because stake.move keeps ValidatorConfig as a per-pool resource and does not enforce a global pubkey -> pool uniqueness invariant; the squat pattern is specific to Gravity's stricter mapping.

UX impact

  • Registration and key rotation are now two-step: commit, wait ≥1 block, then reveal. One extra tx + one block of latency per validator registration or key rotation.
  • Genesis, voluntary leave, force-leave, and eviction paths are unchanged.
  • Permissioned phase (_allowedPools whitelist) was already protected; this fix makes the future flip to _permissionlessJoinEnabled = true safe by removing the squatting primitive that the whitelist bypass would otherwise re-expose.

Shape of the threat model (why 1-block delay is sufficient)

  • The commit is keccak256(pubkey, stakePool, chainid) — opaque, no pubkey leakage (pk space is 2^384).
  • The commit is keyed by msg.sender; an attacker's commit cannot serve a victim's reveal and vice versa.
  • block.number > committedAt rejects same-block commit+reveal. A block producer who sees victim's reveal in block N+1 can include their own commit in block N+1 but cannot register until block N+2, by which point victim has already claimed _pubkeyToValidator[keccak(pk)].

Test plan

  • forge build (compiles clean, warnings pre-existing)
  • forge test — all 1014 tests pass, including 8 new test_audit580_* tests
  • Updated existing tests at direct registerValidator / rotateConsensusKey call sites to preserve the success paths (via _commitPubkey helper)
  • New test_audit580_frontRun_squattingIsBlocked end-to-end test demonstrates attacker loses the race
  • CI green (forge fmt not run locally per repo convention; apply any CI-log diff by hand before merge)

Fixes: Galxe/gravity-audit#580

🤖 Generated with Claude Code

…580)

## What changed

Adds a commit-reveal precondition to `registerValidator` and
`rotateConsensusKey`:

  1. New state `_pubkeyCommits[msg.sender][commitment] = block.number`
     in `ValidatorManagement`.
  2. New external `commitConsensusPubkey(bytes32 commitment)` that any
     caller can invoke; the commitment is scoped by msg.sender.
  3. Internal `_consumeConsensusPubkeyCommit(stakePool, pubkey)` that
     verifies a matching commit exists for msg.sender and was recorded
     in a strictly earlier block, then consumes it.
  4. Wired into both `registerValidator` (after existing validation,
     before `_createValidatorRecord`) and `rotateConsensusKey` (after
     PoP check). Genesis path is untouched: it bypasses commit-reveal
     because it is already gated by the GENESIS system address.

New errors (`ConsensusPubkeyCommitAlreadyExists`, `...CommitNotFound`,
`...CommitTooRecent`) and a `ConsensusPubkeyCommitted` event.

Existing tests updated to commit + roll one block before register/rotate.
Added 8 new tests in `test_audit580_*` covering:
 - register/rotate revert without a commit
 - same-block commit+reveal is rejected (`CommitTooRecent`)
 - commits are keyed by msg.sender (attacker commit cannot serve victim)
 - end-to-end front-run scenario: attacker observing victim's pubkey
   cannot win the race
 - commit consumed on success, duplicate commit from same sender reverts

## Why this shape

The Gravity BLS PoP precompile (gravity-reth
`bls_precompile.rs`) is hard-coded to 144-byte input (`pubkey || pop`)
with an empty augmentation, signing only `pubkey` under the IETF POP
DST. There is no in-precompile way to bind the PoP to
`(operator, stakePool, chainid)` without a cross-repo change to the
precompile contract and the validator keygen toolchain.

That means the BLS PoP only proves "someone holds sk" — it does not
prove "this caller holds sk". Combined with Gravity's (Aptos-plus)
on-chain `_pubkeyToValidator` uniqueness map, a mempool observer can
replay a victim's `(pubkey, pop)` into their own pool and claim
`_pubkeyToValidator[keccak(pk)]` first, DoSing the victim's
registration (audit finding #580).

Commit-reveal closes this gap entirely on the contract side:
 - The commit is an opaque `keccak256(pubkey, stakePool, chainid)`
   hash; observing it leaks nothing about the pubkey (space is 2^384).
 - Reveal requires a commit in a strictly earlier block from the same
   msg.sender, so a same-block race (commit + reveal atomically after
   seeing the victim's reveal) is forbidden.
 - Attacker needs to commit BEFORE learning the pubkey — impossible
   by construction.

Aptos does not need this fix because `stake.move` keeps `ValidatorConfig`
as a per-pool resource and does not enforce a global `pubkey -> pool`
uniqueness invariant; the squat pattern is specific to Gravity's
stricter mapping.

## Effect

 - Registration and key rotation are now two-step (commit, then reveal
   after ≥1 block). UX cost: one extra tx + one block wait per register
   / rotate per validator.
 - Permissioned phase was already protected by the `_allowedPools`
   whitelist; this fix makes the flip to `_permissionlessJoinEnabled = true`
   safe by removing the front-run squatting primitive that whitelist
   bypass would otherwise re-open.
 - Genesis, voluntary leave, force-leave, and eviction paths are
   untouched.

Fixes: Galxe/gravity-audit#580

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ByteYue ByteYue force-pushed the fix/580-consensus-pubkey-squatting-commit-reveal branch from 38a6dfc to 9e01be7 Compare April 21, 2026 08:05
@github-actions
Copy link
Copy Markdown

🛡️ Security Audit Results

Security Audit Summary

Generated on: Tue Apr 21 09:03:22 UTC 2026
Commit: 0f754e9
Branch: 86/merge

Tools Executed

  • ✅ Slither: 1 reports
  • ✅ Mythril: 1 reports
  • ✅ 4naly3er: 1 reports
  • ✅ Aderyn: 0 reports

Next Steps

  1. Review all generated reports in the audit/ directory
  2. Prioritize High and Medium severity issues
  3. Address findings before deployment
  4. Consider additional manual security review

📁 Full reports available in GitHub Actions Artifacts

@github-actions
Copy link
Copy Markdown

🛡️ Security Audit Results

Security Audit Summary

Generated on: Tue Apr 21 09:03:52 UTC 2026
Commit: 4ada729
Branch: 86/merge

Tools Executed

  • ✅ Slither: 1 reports
  • ✅ Mythril: 1 reports
  • ✅ 4naly3er: 1 reports
  • ✅ Aderyn: 0 reports

Next Steps

  1. Review all generated reports in the audit/ directory
  2. Prioritize High and Medium severity issues
  3. Address findings before deployment
  4. Consider additional manual security review

📁 Full reports available in GitHub Actions Artifacts

Copy link
Copy Markdown
Contributor

@nekomoto911 nekomoto911 left a comment

Choose a reason for hiding this comment

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

LGTM. Commit-reveal correctly substitutes the missing PoP augmentation with a timing-based binding: msg.sender-keyed commits + strict block.number > committedAt ensure that only the holder of sk (who alone can know pk before the reveal tx hits the mempool) can pass the consume check. All post-genesis pubkey write sites (registerValidator, rotateConsensusKey) are covered; genesis exemption is intentional.

test_audit580_frontRun_squattingIsBlocked is the key end-to-end correctness proof — it shows commit-too-recent + pubkey-uniqueness layers compose to close the race.

Storage layout additions (_pubkeyCommits + the slots from #85) should ride the next testnet release tag via make extract-storage-layouts + EpsilonHardforkMigration updates — not a per-PR concern under the current release workflow.

nit: forge fmt before merge.

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