From c268069472a245b981944cfe61707255e5d5a9f0 Mon Sep 17 00:00:00 2001 From: phalap1 Date: Sun, 23 Aug 2026 19:30:38 -0400 Subject: [PATCH] fix(milestones): enforce issue_id uniqueness across milestones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `allocate`'s only duplicate guard was `milestone.allocations.contains_key`, scoped to the single `Milestone` record it had loaded, and `IssueStatus` is keyed `(milestone_id, issue_id)`. Neither can see another milestone's state, so `allocate(1, 555, x)` and `allocate(2, 555, y)` both succeeded and both could be released — the same merged work paid for twice, with nothing on-chain objecting. Add `DataKey::GlobalIssueClaim(issue_id) -> milestone_id`, a contract-instance-wide registry keyed by `issue_id` alone, in the spirit of `escrow::DataKey::Escrow(issue_id)`. `allocate` checks it after the per-milestone guard, so a repeat allocation in the same milestone still reports `IssueAlreadyAllocated` while a cross-milestone collision reports the new `IssueClaimedByOtherMilestone`, and writes the claim once every other check has passed. The claim is `deallocate`'s to release when that lands, so a removed allocation frees the issue for legitimate reallocation instead of leaving a permanent false "already claimed". --- README.md | 21 ++++++++++++++-- contracts/milestones/src/error.rs | 4 ++++ contracts/milestones/src/lib.rs | 27 +++++++++++++++++++++ contracts/milestones/src/test.rs | 40 +++++++++++++++++++++++++++++++ contracts/milestones/src/types.rs | 7 ++++++ 5 files changed, 97 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1b76c1f..499693f 100644 --- a/README.md +++ b/README.md @@ -249,8 +249,25 @@ fn get_contribution(env, milestone_id: u64, index: u32) -> Result milestone_id`, is written on + every successful `allocate`; an `allocate` naming an `issue_id` already + claimed by a *different* milestone is rejected with + `IssueClaimedByOtherMilestone`, a distinct error so callers can tell + "this milestone already has it" from "another milestone already has it". + The claim is released by `deallocate` when that lands, so removing an + allocation frees the issue for legitimate reallocation rather than + leaving a permanent false "already claimed". This is scoped to this + contract only; the same `issue_id` being funded through + `mergefi-escrow` as well remains the accepted, backend-mitigated gap + described under "Cross-contract double-funding" above. - `release_issue`: admin-only, same split/fee mechanics as escrow's `release`, but draws from the issue's pre-reserved allocation rather than a fresh deposit. Rejects double release (`IssueAlreadyReleased`). diff --git a/contracts/milestones/src/error.rs b/contracts/milestones/src/error.rs index e0d7d45..7097247 100644 --- a/contracts/milestones/src/error.rs +++ b/contracts/milestones/src/error.rs @@ -17,4 +17,8 @@ pub enum Error { InvalidFee = 11, MilestoneClosed = 12, TooManySponsors = 13, + /// Already allocated in a *different* milestone, as opposed to + /// `IssueAlreadyAllocated`, which means "already allocated in the + /// milestone you are calling against". + IssueClaimedByOtherMilestone = 14, } diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 80c5210..443d7c1 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -173,6 +173,18 @@ impl MilestonesContract { /// `issue_id`. Rejects if the issue is already allocated, the milestone /// is closed, or `amount` exceeds the remaining (unallocated) budget. /// + /// An `issue_id` can be allocated in at most one milestone at a time. + /// `milestone.allocations` and `DataKey::IssueStatus(milestone_id, + /// issue_id)` are both scoped to the single `Milestone` record loaded + /// here, so neither can see that another milestone already committed + /// budget to the same GitHub issue — which let `allocate(1, 555, x)` + /// and `allocate(2, 555, y)` both succeed and both be released, paying + /// twice for one merged PR. `DataKey::GlobalIssueClaim(issue_id)` + /// closes that: an `allocate` from a *different* `milestone_id` is + /// rejected with `IssueClaimedByOtherMilestone`, leaving + /// `IssueAlreadyAllocated` to mean a repeat allocation within the + /// *same* milestone. + /// /// Note: this contract has no visibility into `mergefi-escrow` — /// nothing here stops the same `issue_id` from also being funded via /// `escrow::fund` as a standalone bounty. See README "Why three @@ -198,6 +210,18 @@ impl MilestonesContract { if milestone.allocations.contains_key(issue_id) { return Err(Error::IssueAlreadyAllocated); } + + // Checked after the per-milestone guard above, so a repeat + // allocation within this same milestone keeps reporting + // `IssueAlreadyAllocated` and only a genuine cross-milestone + // collision reaches this branch. + let ckey = DataKey::GlobalIssueClaim(issue_id); + if let Some(claimed_by) = env.storage().persistent().get::(&ckey) { + if claimed_by != milestone_id { + return Err(Error::IssueClaimedByOtherMilestone); + } + } + if amount > milestone.remaining_budget { return Err(Error::OverAllocation); } @@ -213,6 +237,9 @@ impl MilestonesContract { .set(&skey, &IssueStatus::Allocated); extend_ttl(&env, &skey); + env.storage().persistent().set(&ckey, &milestone_id); + extend_ttl(&env, &ckey); + Ok(()) } diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index d7c4b7c..dc76d92 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -559,3 +559,43 @@ fn test_get_contribution_enumerates_each_contributor() { let err = client.try_get_contribution(&57u64, &2u32); assert_eq!(err, Err(Ok(Error::MilestoneNotFound))); } + +#[test] +fn test_allocate_rejects_issue_already_claimed_by_different_milestone() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &20_000i128); + + client.create_milestone(&70u64, &sponsor, &token_addr, &10_000i128); + client.create_milestone(&71u64, &sponsor, &token_addr, &10_000i128); + + client.allocate(&70u64, &555u64, &4_000i128); + + // Same GitHub issue, different milestone: rejected by the global claim + // registry even though milestone 71's own allocations have never seen + // issue 555. + let err = client.try_allocate(&71u64, &555u64, &3_000i128); + assert_eq!(err, Err(Ok(Error::IssueClaimedByOtherMilestone))); + + // The rejected call left milestone 71 untouched, so there is no second + // allocation for `release_issue` to pay out. + let milestone_b = client.get_milestone(&71u64); + assert_eq!(milestone_b.remaining_budget, 10_000i128); + assert_eq!(milestone_b.allocations.len(), 0); + assert_eq!( + client.try_get_issue_status(&71u64, &555u64), + Err(Ok(Error::IssueNotAllocated)) + ); + + // A repeat allocation inside the *same* milestone still reports the + // pre-existing error, so the two collisions stay distinguishable. + assert_eq!( + client.try_allocate(&70u64, &555u64, &1_000i128), + Err(Ok(Error::IssueAlreadyAllocated)) + ); +} diff --git a/contracts/milestones/src/types.rs b/contracts/milestones/src/types.rs index a042a73..65cbdde 100644 --- a/contracts/milestones/src/types.rs +++ b/contracts/milestones/src/types.rs @@ -62,6 +62,13 @@ pub enum DataKey { Milestone(u64), IssueStatus(u64, u64), // (milestone_id, issue_id) Contribution(u64, u32), // (milestone_id, contribution_index) + /// Contract-instance-wide claim registry: `issue_id -> milestone_id`. + /// The one key here scoped by `issue_id` alone, which is what lets + /// `allocate` see that a *different* milestone already committed budget + /// to the same GitHub issue. To be cleared by `deallocate` when that + /// lands, so a removed allocation frees the issue for reallocation + /// instead of leaving a permanent false "already claimed". + GlobalIssueClaim(u64), // issue_id } impl mergefi_common::AdminKey for DataKey {