fix: F-2026-18793 | [Dual Defense] RecomputeBallotQuorum Applies 2/3+1 Even to TSS Ballots - #336
Merged
Merged
Conversation
TSS_KEY ballots use 100% of the DKLS participant set, not 2/3+1; recompute would rewrite both the threshold and the eligible voters. Gate on an explicit default-deny allow-list (INBOUND_TX, OUTBOUND_TX, FUND_MIGRATION).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
F-2026-18793 — Low (Impact 1 / Likelihood 1)
Keeper.RecomputeBallotQuorumis the admin escape hatch for ballots stuck after avalidator-set change. It did two things unconditionally, with no branch on
BallotType:That is only correct for ballots that were created that way. TSS key ballots are not
2/3 ballots —
x/utss/keeper/voting.go:52:with
EligibleVoters = existing.Participants. So recompute on a TSS ballot dropped thethreshold from N to ⌊2N/3⌋+1 and replaced the participant list with whoever is a live
UV now — which can make validators who never took part in that DKLS run eligible to vote
on observing its key.
The eligible-set rewrite is the worse half. A wrong threshold is a counting error; a wrong
eligible set means the ballot no longer attests what it was created to attest.
Admin-gated (
params.Admin != msg.Signer→ reject), requires an explicitballot_id, isnever called from any join/leave/status path, and does not auto-finalize — hence the Low
rating.
Type audit — only
TSS_KEYdivergesINBOUND_TX(2N)/3+1GetEligibleVotersOUTBOUND_TX(2N)/3+1GetEligibleVotersFUND_MIGRATION(2N)/3+1(voting.go:113-119)GetEligibleVoters(voting.go:113)TSS_KEYlen(Participants)= 100%existing.ParticipantsThe fix — default-deny allow-list
Structured as an explicit
switchwith adefault:refusal, not asif ballotType == TSS_KEY { refuse }.Why default-deny rather than a TSS-specific check.
READ_RESULTis not on this branch— it lands with read-state. Under an allow-list it inherits a refusal rather than
silently inheriting a formula that may not apply to it. That is precisely how this bug
arose: the function was written for inbound/outbound and TSS quietly inherited it. Adding
a type to the allow-list is now a deliberate act that forces someone to check how that
type is created.
Why not Hacken's alternative (
votesNeeded = len(preservedTSSParticipants)): it keepsthe eligible-set rewrite in play. And a TSS ballot whose participants changed is not a
quorum problem — the DKLS run itself is invalid. A recomputed threshold would manufacture
an attestation nobody made. The fix there is a fresh keygen round, not a lower bar.
Why
FUND_MIGRATIONis deliberately still allowedRefusing it was considered and rejected. Two facts, both re-verified against this branch:
VoteOnFundMigrationBallot(x/utss/keeper/voting.go:113-119)calls
k.uvalidatorKeeper.GetEligibleVoters(ctx)and computesvotesNeeded := (fundMigrationVotesNumerator*totalValidators)/fundMigrationVotesDenominator + 1with
numerator = 2, denominator = 3— identical to what recompute produces. Thein-repo comment says so: "FundMigration uses 2/3 quorum like outbound observations".
InitiateFundMigration(
x/utss/keeper/msg_initiate_fund_migration.go:56) blocks a replacement while one isPENDING —
"pending migration already exists for chain %s (migration_id: %d, old_key: %s)"—and
grep -rn FailFundMigrationover the repo returns no hits; Hacken rec 4 onF-2026-18142 is unimplemented.
Refusing recompute would therefore remove a hatch that currently works correctly and leave
a stuck fund migration with no way out. That is a regression, not a fix.
Tests
test/integration/uvalidator/recompute_ballot_quorum_test.go:TestRecomputeBallotQuorum_TSSKeyBallot_Refused_StateUnchanged— Hacken rec 3. A TSSballot with 4 of 5 votes and threshold 5; admin recompute must error, must not finalize,
and must leave
VotingThresholdandEligibleVotersbyte-for-byte unchanged. Twovalidators are unbonded first so the live UV set genuinely differs (3 ≠ 5) — otherwise an
unguarded recompute would be a no-op and the test would pass vacuously.
TestRecomputeBallotQuorum_AllowedTypes_StillRecompute— all three allow-listed typesstill recompute (5→2 eligible, threshold 4→2), asserted on the persisted ballot.
TestRecomputeBallotQuorum_UnrecognisedType_Refused— pins the default-deny for bothUNSPECIFIEDand an unmapped enum value standing in for a future type.TestRecomputeBallotQuorum_StatusGuardRunsBeforeTypeGuard— the existing PENDING-onlyguard still runs first.
TestRecomputeBallotQuorum_ZeroEligible_AllowedExpires_RefusedDoesNot— thezero-eligible → EXPIRED path is unchanged for allowed types, and a refused type is not
auto-expired as a side effect.
The refusal tests assert state before the error, deliberately:
require.Errorabortsthe subtest, so an error-first ordering would never reach the assertions that catch a
refusal which had already mutated state.
Mutation-verified. With the
default:refusal changed to fall through to the allowpath, the TSS test fails on the state assertion, not on a missing error:
The other two refusal tests fail the same way (threshold 7→3; status PENDING→EXPIRED).
No existing test recomputed a TSS ballot, so nothing needed weakening — the one other
caller,
test/integration/uexecutor/revert_stuck_inbound_test.go:282, usesINBOUND_TX.Scope
Confined to
RecomputeBallotQuorumand its doc comment — purely additive, no linesremoved. Does not touch ballot creation,
GetEligibleVoters, the 2/3+1 formula, or theexpiry/
ActiveBallotIDsmachinery.