Skip to content

fix(gov-rotate): clear stale approvals on re-stage (multisig can commit an unsigned member set)#8015

Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Scottcjn:mainfrom
Vyacheslav-Tomashevskiy:fix/gov-rotate-restage-stale-approvals
Open

fix(gov-rotate): clear stale approvals on re-stage (multisig can commit an unsigned member set)#8015
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Scottcjn:mainfrom
Vyacheslav-Tomashevskiy:fix/gov-rotate-restage-stale-approvals

Conversation

@Vyacheslav-Tomashevskiy

Copy link
Copy Markdown
Contributor

Bug: governance-rotation multisig approvals aren't bound to the staged member set

/gov/rotate/approve verifies an Ed25519 signature over the canonical message ROTATE|{epoch}|{threshold}|sha256(members_json), so each approval authorizes exactly one (threshold, member-set) proposal.

But gov_rotate_stage re-stages in place — INSERT OR REPLACE INTO gov_rotation_proposals for the same epoch_effective, then DELETE FROM gov_rotation + DELETE FROM gov_rotation_members, but not gov_rotation_approvals. And gov_rotate_commit only does SELECT COUNT(*) FROM gov_rotation_approvals WHERE epoch_effective=? — it never re-verifies the stored signatures against the current members_json.

So a signature that only ever signed member-set A is counted toward committing member-set B. This defeats the rotation multisig, whose whole purpose is that a single compromised admin key cannot unilaterally rotate the governance signer set (stage is admin-gated; commit is not).

Exploit trace (reproduced against the real endpoints)

  1. Stage epoch E with set A [signer1], threshold 1 → message ROTATE|E|1|sha256(A).
  2. signer1 signs it → /gov/rotate/approveapprovals:1, ready:true.
  3. Re-stage the same epoch E with a different set B [signer2] → signing message changes; the approval row for A is untouched.
  4. /gov/rotate/commit{"committed":1,"ok":true,"approvals":1} — set B committed, though nobody ever signed set B.

Fix

(Re)staging redefines the canonical signing message, so drop any approvals for that epoch when staging:

         c.execute("DELETE FROM gov_rotation WHERE epoch_effective=?", (epoch,))
         c.execute("DELETE FROM gov_rotation_members WHERE epoch_effective=?", (epoch,))
+        c.execute("DELETE FROM gov_rotation_approvals WHERE epoch_effective=?", (epoch,))

Commit then sees 0 approvals for the new set and returns insufficient_approvals until it is signed afresh.

Test

node/tests/test_gov_rotate_restage_stale_approvals.py drives the real Flask endpoints via test_client:

  • controlstage → approve → commit (no re-stage) still succeeds (fix doesn't break legitimate rotations).
  • exploit — approve set A, re-stage set B, then commit must NOT return 200 and must report insufficient_approvals.

Results: fails on current main (commit returns 200 committed:1 with the stale approval); both pass with the fix; mutation (revert the DELETE) fails again. Related governance suites stay green (propose_nonce_replay, vote_race, rip0202_governance_params — 19 passed).

Distinct from the /governance/vote quorum path — this is the gov_rotation staged-multisig subsystem (stage/approve/commit).

…commit an unsigned member set

/gov/rotate/approve verifies an Ed25519 signature over the canonical
message ROTATE|{epoch}|{threshold}|sha256(members_json), so each approval
authorizes exactly one (threshold, member-set) proposal. But gov_rotate_stage
uses INSERT OR REPLACE for the same epoch_effective and deletes gov_rotation +
gov_rotation_members WITHOUT deleting gov_rotation_approvals, and
gov_rotate_commit only does SELECT COUNT(*) over the approvals table -- it
never re-verifies the stored signatures against the current members_json.

Result: stage set A -> signer approves A -> re-stage the SAME epoch with a
different set B -> commit counts A's signature toward B and commits a member
set nobody ever signed. This defeats the rotation multisig, whose whole purpose
is that a single compromised admin key cannot unilaterally rotate the governance
signer set (stage is admin-gated; commit is not).

Fix: (re)staging redefines the signing message, so drop any approvals for that
epoch when staging. A one-line DELETE; commit now sees 0 approvals for the new
set and returns insufficient_approvals until it is signed afresh.

Test: node/tests/test_gov_rotate_restage_stale_approvals.py drives the real
Flask endpoints -- control (stage->approve->commit) still succeeds; the
re-stage-with-different-members exploit must NOT commit. Fails on current main
(commit returns 200 with the stale approval), passes with the fix; mutation
(revert the DELETE) fails again. Related governance suites stay green (19 passed).
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related tests Test suite changes size/M PR: 51-200 lines labels Jul 18, 2026
@Scottcjn

Copy link
Copy Markdown
Owner

Thank you for this run of security work, @Vyacheslav-Tomashevskiy — it's genuinely excellent. 205 RTC paid for 13 accepted findings: #8007, #8010, #8011, #8012, #8013, #8014, #8015, #7977, #7978, #7979 and the three vintage-x86 fixes.

tx 0bdbe2965e91a7d508dcb520441b51ac · pending_id 3745 · to RTCd1554f0f35576faf01d386a6be1c947f560dd0b7 · confirms in ~24h.

The high-severity ones (reward-weight binding #8013, fund-exit freeze #8014, multisig re-stage #8015, airdrop Sybil #8010, bcos cert overwrite #8011) are exactly the kind of consensus-integrity work we most want. We're reviewing each fix for merge/hardening on our side — payment reflects the accepted finding, independent of how we land the final patch. Please keep them coming. — Sophia, Elyan Labs

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

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) BCOS-L2 Beacon Certified Open Source tier BCOS-L2 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants