milestones::allocate: issue_id uniqueness is only enforced within a single milestone_id — the same GitHub issue can be paid out from multiple milestones

Aperta
  • Qualcosa non va?
#51 4 commenti 0 reazioni 1 assegnatario Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

@phalap1 ci sta già lavorando.

Dal 17/8/2026.

  • #192 di @phalap1 — aperta

Valutazione

Questa issue non è ancora stata valutata.

Descrizione

bug GrantFox OSS Maybe Rewarded Official Campaign | FWC26 security Third Campaign very hard

Overview

milestones scopes an issue's allocation status by the pair (milestone_id, issue_id), not by issue_id alone: DataKey::IssueStatus(u64, u64) (contracts/milestones/src/types.rs:35), read/written exclusively as DataKey::IssueStatus(milestone_id, issue_id) throughout allocate and release_issue (contracts/milestones/src/lib.rs:121, 148, 179). allocate's only duplicate-allocation guard, milestone.allocations.contains_key(issue_id) (contracts/milestones/src/lib.rs:109), is likewise scoped to the one Milestone record it's operating on — it has no visibility into, and performs no check against, any other milestone's allocations.

Consequence: the same underlying GitHub issue_id can be allocated, and independently released, from multiple different milestones simultaneously, with the contract providing zero defense-in-depth against it. Concretely: allocate(milestone_id=1, issue_id=555, amount=X) and allocate(milestone_id=2, issue_id=555, amount=Y) both succeed unconditionally — nothing in either call inspects milestone 2's state while processing milestone 1's, or vice versa. Both allocations can subsequently be independently released via release_issue(1, 555, ...) and release_issue(2, 555, ...), each paying out its own full amount, for what is, off-chain, the same piece of merged work being paid for twice (or more) over.

This is a real gap even under the codebase's explicit "contracts are deliberately dumb, backend is the trusted oracle" trust model (README: "GitHub remains the source of truth... these contracts are deliberately dumb about that — they only know what the oracle tells them") — that model justifies the contract trusting the admin's judgment about whether work was done; it does not, by itself, justify providing zero structural defense against the admin being tricked, or making an honest mistake, into allocating the same issue_id twice across different milestones, something a single admin key operating many milestones over time is entirely capable of doing accidentally (e.g. a spreadsheet/database error on the backend side associating one GitHub issue with two different release milestones). Compare with escrow::fund, which enforces issue_id uniqueness globally via a single DataKey::Escrow(issue_id) key with no milestone-equivalent scoping concept at all — milestones' weaker, per-milestone-only scoping is a deliberate consequence of its data model (an issue_id legitimately needs to be nameable within many different milestones' allocations maps in principle), but the complete absence of any global "this issue_id is already claimed somewhere" registry is a gap escrow's design doesn't share.

Requirements

  • Add a global (contract-instance-wide) registry — e.g. DataKey::GlobalIssueClaim(issue_id) -> milestone_id — checked and set by allocate, rejecting a second allocate call for an issue_id that's already claimed by a different milestone_id, with a new Error variant distinguishing this from the existing per-milestone IssueAlreadyAllocated.
  • Decide what should happen when an issue's allocation is later removed (once the companion deallocate function from this batch's other issue lands) — the global claim should presumably be released at that point too, so the registry doesn't itself become a source of permanent false "already claimed" rejections.
  • Document, in the README's "Contracts" section for milestones, the intended guarantee explicitly: "an issue_id can be allocated in at most one milestone at a time."

Acceptance Criteria

  • Global issue-claim registry implemented and enforced in allocate
  • New Error variant distinguishing "already allocated in this milestone" from "already allocated in a different milestone"
  • Test: test_allocate_rejects_issue_already_claimed_by_different_milestone — allocate issue_id=X in milestone A, attempt to allocate the same issue_id in milestone B, assert rejection
  • Test: interaction with deallocate (if landed) — deallocating frees the global claim, allowing legitimate reallocation elsewhere
  • README updated with the explicit cross-milestone uniqueness guarantee
  • cargo test --workspace passes

Additional Notes

  • Precise references: contracts/milestones/src/types.rs:35 (IssueStatus(u64, u64) keying, confirming the scoping is milestone-relative, not global); contracts/milestones/src/lib.rs:109 (allocate's sole duplicate check, scoped to the single Milestone record already loaded into milestone — provably has no access to any other milestone's data within that function call).
  • Test sketch: test_double_funding_same_issue_across_two_milestones_pre_fix as a reproduction — create_milestone(1, ...), create_milestone(2, ...), allocate(1, 555, amount_a), allocate(2, 555, amount_b) (both currently succeed), release_issue(1, 555, recipients), release_issue(2, 555, recipients) (both currently succeed), assert the recipient(s) received two separate payouts for the same issue_id — demonstrating the double-payment is real and silent before the fix closes it.
  • Cross-references: #1 (escrow's issue-id squatting — a different mechanism, front-running by an unrelated party, not double-allocation by the legitimate admin across milestones); the companion cross-contract double-funding issue filed in this same batch (this issue's within-milestones-contract scope is the narrower, more directly fixable half of that broader concern); the "no deallocate/reallocate" issue filed in this batch (interacts directly with how a global claim registry should behave on deallocation).
Lingua principale
Rust
Stelle
1
Fork
102
Merge medio
4h 12m
PR unite (30g)
30

Preparare l'ambiente

  • Nessun Dockerfile né file Docker Compose
  • Ha un modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di MergeFi/contracts

Tutte le issue di MergeFi/contracts

Issue simili

Altre issue su Rust

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.