Skip to content

pallet-referenda extracted from governance umbrella PR#2714

Open
l0r1s wants to merge 1 commit into
governance-signed-votingfrom
governance-referenda
Open

pallet-referenda extracted from governance umbrella PR#2714
l0r1s wants to merge 1 commit into
governance-signed-votingfrom
governance-referenda

Conversation

@l0r1s
Copy link
Copy Markdown
Collaborator

@l0r1s l0r1s commented Jun 3, 2026

DO NOT MERGE

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +506 to +510
let now = T::BlockNumberProvider::current_block_number();
let index = ReferendumCount::<T>::get();
ReferendumCount::<T>::put(index.saturating_add(1));
ActiveCount::<T>::put(active.saturating_add(1));
ActivePerProposer::<T>::insert(&proposer, active_per_proposer.saturating_add(1));
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.

[HIGH] Submit can leak active referendum slots on scheduler/preimage failure

submit increments ReferendumCount, ActiveCount, and ActivePerProposer before fallible work runs below (set_alarm, Preimages::bound, or schedule_enactment). This call is not transactional, so if any of those operations returns Err the extrinsic leaves an active slot counted with no ReferendumStatusFor entry to kill or complete. A proposer that can trigger scheduler failure, for example by hitting MaxScheduledPerBlock, can permanently consume their quota and eventually the global governance queue. Move all fallible preimage/scheduler work before these counters are committed, or make the operation transactional and explicitly clean up any scheduled/preimage state on failure.

Comment on lines +1031 to +1033
fn set_alarm(index: ReferendumIndex, when: BlockNumberFor<T>) -> Result<(), DispatchError> {
let call = T::Preimages::bound(CallOf::<T>::from(Call::advance_referendum { index }))?;
let _ = T::Scheduler::cancel_named(alarm_name(index));
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.

[MEDIUM] Replacing an alarm can delete the only wake-up if scheduling fails

set_alarm cancels the existing named alarm before attempting to schedule the replacement. If schedule_named then fails, the referendum is left ongoing with no pending alarm, so it will not auto-advance or expire until another vote or privileged manual recovery happens. This is reachable from on_tally_updated, where a normal vote attempts to replace the deadline alarm with a next-block alarm. Use reschedule_named for existing alarms, or otherwise preserve/restore the old alarm unless the replacement has been scheduled successfully.

return Err(Error::<T>::Unreachable.into());
};

if tally.approval >= *approve_threshold {
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.

[MEDIUM] Pass-or-fail tracks approve when both thresholds are crossed

The state machine checks approval before rejection, but TracksInfo::check_integrity only rejects zero pass/fail thresholds and does not require approve_threshold + reject_threshold > 100%. A valid-looking track with both thresholds at or below 50% can therefore execute a proposal on a split vote where the rejection threshold was also met. Add the same non-overlap invariant used for adjustable tracks, or define an explicit rejection-precedence rule.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: VULNERABLE

Baseline scrutiny: l0r1s has write permission, an established account, substantial prior subtensor history, and no Gittensor allowlist/index hit; branch is governance-referenda -> governance-signed-voting.

The PR does not modify .github/ai-review/* or .github/copilot-instructions.md. Dependency changes are limited to a new in-repo pallet plus existing workspace/path dependencies; I did not find a new external supply-chain dependency. The prior Skeptic findings remain present in the current diff.

Findings

Sev File Finding
HIGH pallets/referenda/src/lib.rs:510 Submit can leak active referendum slots on scheduler/preimage failure inline
MEDIUM pallets/referenda/src/lib.rs:1033 Replacing an alarm can delete the only wake-up if scheduling fails inline
MEDIUM pallets/referenda/src/lib.rs:754 Pass-or-fail tracks approve when both thresholds are crossed inline

Prior-comment reconciliation

  • 559b42bf: not addressed — The active counters are still incremented before set_alarm, Preimages::bound, and schedule_enactment can fail in submit.
  • 6be9f109: not addressedset_alarm still calls cancel_named before attempting schedule_named, so a failed replacement can remove the existing alarm.
  • 133a793d: not addressedadvance_ongoing still checks approval before rejection for PassOrFail, and there is still no pass/fail threshold-sum integrity check.

Conclusion

This looks like a legitimate governance-pallet extraction, but the current implementation still has governance liveness/state-accounting vulnerabilities around fallible scheduler/preimage operations and ambiguous threshold resolution.


📜 Previous run (superseded)
Sev File Finding Status
HIGH pallets/referenda/src/lib.rs:510 Submit can leak active referendum slots on scheduler/preimage failure ➡️ Carried forward to current findings
The active counters are still incremented before set_alarm, Preimages::bound, and schedule_enactment can fail in submit.
MEDIUM pallets/referenda/src/lib.rs:1033 Replacing an alarm can delete the only wake-up if scheduling fails ➡️ Carried forward to current findings
set_alarm still calls cancel_named before attempting schedule_named, so a failed replacement can remove the existing alarm.
MEDIUM pallets/referenda/src/lib.rs:751 Pass-or-fail tracks approve when both thresholds are crossed ➡️ Carried forward to current findings
advance_ongoing still checks approval before rejection for PassOrFail, and there is still no pass/fail threshold-sum integrity check.

# 🔍 AI Review — Auditor (domain review) has not yet run on this PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🔄 AI review updated — Skeptic: VULNERABLE

@l0r1s l0r1s mentioned this pull request Jun 3, 2026
@l0r1s l0r1s marked this pull request as ready for review June 3, 2026 19:49
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +508 to +510
ReferendumCount::<T>::put(index.saturating_add(1));
ActiveCount::<T>::put(active.saturating_add(1));
ActivePerProposer::<T>::insert(&proposer, active_per_proposer.saturating_add(1));
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.

[HIGH] Submit can leak active referendum slots on scheduler/preimage failure

submit increments ReferendumCount, ActiveCount, and ActivePerProposer before the fallible setup at lines 517, 518, and 523. Returning Err from an extrinsic does not automatically roll back these storage writes, so a scheduler/preimage failure can consume global and per-proposer active slots without inserting an ongoing referendum that can later conclude and release them. Move these mutations after all fallible scheduling/preimage work succeeds, or explicitly undo them on every error path before returning.

/// do not need to track whether one is currently pending.
fn set_alarm(index: ReferendumIndex, when: BlockNumberFor<T>) -> Result<(), DispatchError> {
let call = T::Preimages::bound(CallOf::<T>::from(Call::advance_referendum { index }))?;
let _ = T::Scheduler::cancel_named(alarm_name(index));
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.

[MEDIUM] Replacing an alarm can delete the only wake-up if scheduling fails

set_alarm cancels the existing alarm before it knows the replacement can be scheduled. Any call site that re-arms a live referendum can therefore lose the only automatic wake-up if schedule_named fails after this line; the referendum then requires privileged/manual recovery to progress. Schedule the replacement without first deleting the old alarm where possible, or restore/retry the previous alarm if replacement scheduling fails.

Comment on lines +751 to +754
if tally.approval >= *approve_threshold {
Self::do_approve(index, &info, on_approval, *decision_period);
} else if tally.rejection >= *reject_threshold {
Self::do_reject(index);
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.

[MEDIUM] Pass-or-fail tracks approve when both thresholds are crossed

The pass-or-fail branch gives approval priority whenever both approval >= approve_threshold and rejection >= reject_threshold are true. VoteTally stores approval and rejection independently, and this PR only prevents overlapping fast-track/cancel thresholds for Adjustable tracks, not PassOrFail tracks. A misconfigured track with thresholds whose sum is at or below 100% can approve even when the reject threshold is also met. Add an integrity rule that prevents overlapping pass/fail thresholds, or define and enforce an explicit tie/dual-threshold policy.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

🔄 AI review updated — Skeptic: VULNERABLE

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.

1 participant