diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 7b66ff4e..64510d74 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -925,56 +925,41 @@ mod contract { Ok(()) } - /// Upgrade the contract's wasm code. Requires admin authorization. - /// Preserves all existing storage; the new code's `migrate` entrypoint - /// advances the stored version after the upgrade. - /// See docs/upgrade-storage-migration-design.md. - pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), Error> { - let admin = require_admin(&env)?; - admin.require_auth(); - - env.deployer().update_current_contract_wasm(new_wasm_hash); - extend_instance_ttl(&env); - Ok(()) - } + /// Upgrade the contract's wasm code. Requires admin authorization. + /// Preserves all existing storage; the new code's `migrate` entrypoint + /// advances the stored version after the upgrade. + /// See docs/upgrade-storage-migration-design.md. + pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); - /// Apply storage migrations through this code version. Call after - /// `upgrade`; an already-migrated contract accepts a retry. - pub fn migrate(env: Env, from_version: u32) -> Result<(), Error> { - require_admin(&env)?.require_auth(); - - let current_version = env - .storage() - .instance() - .get::<_, u32>(&DataKey::Version) - .unwrap_or(0); - if current_version == CONTRACT_VERSION { + env.deployer().update_current_contract_wasm(new_wasm_hash); extend_instance_ttl(&env); - return Ok(()); - } - if current_version != from_version || from_version >= CONTRACT_VERSION { - return Err(Error::InvalidVersion); + Ok(()) } - env.storage() - .instance() - .set(&DataKey::Version, &CONTRACT_VERSION); - extend_instance_ttl(&env); - Ok(()) - } + /// Apply storage migrations through this code version. Call after + /// `upgrade`; an already-migrated contract accepts a retry. + pub fn migrate(env: Env, from_version: u32) -> Result<(), Error> { + require_admin(&env)?.require_auth(); + + let current_version = env + .storage() + .instance() + .get::<_, u32>(&DataKey::Version) + .unwrap_or(0); + if current_version == CONTRACT_VERSION { + extend_instance_ttl(&env); + return Ok(()); + } + if current_version != from_version || from_version >= CONTRACT_VERSION { + return Err(Error::InvalidVersion); + } - env.deployer() - .update_current_contract_wasm(new_wasm_hash.clone()); env.storage() .instance() .set(&DataKey::Version, &CONTRACT_VERSION); extend_instance_ttl(&env); - ContractUpgraded { - admin, - wasm_hash: new_wasm_hash, - version: CONTRACT_VERSION, - } - .publish(&env); Ok(()) } diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index 22b9c1d6..5f4ada46 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -964,6 +964,33 @@ fn test_extend_deadline_rejects_after_paid_or_refunded() { assert_eq!(err, Err(Ok(Error::AlreadyPaid))); } +#[test] +fn test_extend_deadline_unauthorized() { + 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, &10_000_000_000i128); + + env.ledger().set_timestamp(100); + client.fund( + &16u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &200u64, + &None, + ); + + // A non-sponsor (even a contributor) cannot extend the deadline. + let non_sponsor = Address::generate(&env); + let err = client.try_extend_deadline(&16u64, &non_sponsor, &500u64); + assert_eq!(err, Err(Ok(Error::Unauthorized))); +} + // --------------------------------------------------------------------------- // Crowdfunding (#57) // --------------------------------------------------------------------------- diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 2decae25..73fa1848 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -274,7 +274,7 @@ mod contract { oracle, treasury, fee_bps, - max_sponsors: max_sponsors.unwrap_or(MAX_SPONSORS), + max_sponsors, recovery, } .publish(&env); @@ -344,6 +344,7 @@ mod contract { &Contribution { sponsor: sponsor.clone(), amount: actual_received, + remaining: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -456,6 +457,10 @@ mod contract { .amount .checked_add(actual_received) .ok_or(Error::ArithmeticOverflow)?; + contribution.remaining = contribution + .remaining + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; contribution.timestamp = env.ledger().timestamp(); env.storage() .persistent() @@ -469,6 +474,7 @@ mod contract { &Contribution { sponsor: sponsor.clone(), amount: actual_received, + remaining: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -685,7 +691,33 @@ mod contract { // stay alive as individual issues are released over the milestone's // lifetime — release_issue doesn't touch contributions directly but // is called repeatedly on long-lived milestones. + // + // Deduct the release amount from contributions FIFO (first in, first out) + // to track each sponsor's unspent remainder for accurate refunds. + let mut to_deduct = amount; for i in 0..milestone.contributor_count { + if to_deduct <= 0 { + break; + } + let contribution_key = DataKey::Contribution(milestone_id, i); + let mut contribution: Contribution = env + .storage() + .persistent() + .get(&contribution_key) + .ok_or(Error::MilestoneNotFound)?; + + let deduction = to_deduct.min(contribution.remaining); + contribution.remaining = contribution + .remaining + .checked_sub(deduction) + .ok_or(Error::ArithmeticOverflow)?; + to_deduct = to_deduct + .checked_sub(deduction) + .ok_or(Error::ArithmeticOverflow)?; + + env.storage() + .persistent() + .set(&contribution_key, &contribution); extend_ttl(&env, &DataKey::Contribution(milestone_id, i)); } extend_instance_ttl(&env); @@ -747,13 +779,11 @@ mod contract { /// Rejects if the issue is already Released (funds have left the /// contract) or not currently Allocated. /// - /// Blocked while the contract is paused, like `allocate`, since it - /// mutates the same allocation state. + /// Intentionally **not blocked** while paused, as it is a sponsor-protective + /// operation that moves funds back toward refund paths (see + /// docs/pause-circuit-breaker-design.md principle 1). During a pause, + /// sponsors can use deallocate + cancel_milestone to recover allocated funds. pub fn deallocate(env: Env, milestone_id: u64, issue_id: u64) -> Result<(), Error> { - if is_paused(&env) { - return Err(Error::ContractPaused); - } - require_admin(&env)?.require_auth(); let mkey = DataKey::Milestone(milestone_id); @@ -1177,27 +1207,18 @@ mod contract { pub use contract::*; -/// Pays each contributor their share of `milestone.remaining_budget` (the -/// unallocated remainder of the pool), computed as -/// `remaining_budget * contribution.amount / total_budget` — i.e. in -/// proportion to what each sponsor actually put in, not to any nominal -/// split of the original deposit. Because the contribution ledger is -/// append-only and `total_budget` / `remaining_budget` are maintained -/// additively, each sponsor's share is a fixed fraction of the pool no -/// matter how many intervening `allocate` / `release_issue` (and, once the -/// deallocate issue lands, `deallocate`) calls happened between the -/// deposits and this refund — the remainder to return is simply sliced by -/// those fixed fractions at refund time. +/// Refunds each contributor their actual unspent remainder from the +/// milestone's remaining budget. Each contributor's refund is exactly their +/// `contribution.remaining` amount, which has been properly tracked through +/// FIFO deductions during `release_issue` calls. The sum of all `remaining` +/// values across contributions equals `milestone.remaining_budget`, so all +/// funds are returned with no dust left in the contract. /// -/// Uses largest-remainder rounding (the same invariant as -/// `compute_split`): every share is floored first, then the remaining dust -/// (always strictly less than `contributor_count` units) is granted one -/// unit at a time to the entry with the largest fractional remainder, -/// tie-broken by contribution index so the outcome is deterministic and -/// independent of anything a caller controls. The full `remaining_budget` -/// is returned, so no dust is stranded in the contract. A single-sponsor -/// milestone is the degenerate case: contribution 0's amount equals -/// `total_budget`, so the share is exactly `remaining_budget`. +/// Before this fix (issue #750), refunds incorrectly used +/// `remaining_budget * contribution.amount / total_budget`, which unfairly +/// penalized late contributors by refunding them a share of money that had +/// already been spent by earlier contributors. The FIFO tracking ensures +/// each sponsor gets back exactly what wasn't spent from their contribution. fn refund_remaining_budget( env: &Env, milestone_id: u64, @@ -1211,10 +1232,7 @@ fn refund_remaining_budget( let token_client = token::Client::new(env, &milestone.token); let contract_address = env.current_contract_address(); - let mut shares: Vec<(Address, i128)> = Vec::new(env); - let mut order: Vec<(u32, i128, Address)> = Vec::new(env); - let mut allocated: i128 = 0; - + // Refund each contributor their actual unspent remainder for i in 0..milestone.contributor_count { let contribution_key = DataKey::Contribution(milestone_id, i); let contribution: Contribution = env @@ -1222,44 +1240,13 @@ fn refund_remaining_budget( .persistent() .get(&contribution_key) .ok_or(Error::MilestoneNotFound)?; - let numerator = remaining - .checked_mul(contribution.amount) - .ok_or(Error::ArithmeticOverflow)?; - let share = numerator / milestone.total_budget; - let remainder = numerator % milestone.total_budget; - allocated = allocated - .checked_add(share) - .ok_or(Error::ArithmeticOverflow)?; - shares.push_back((contribution.sponsor.clone(), share)); - order.push_back((order.len(), remainder, contribution.sponsor)); - } - - let dust = remaining - .checked_sub(allocated) - .ok_or(Error::ArithmeticOverflow)?; - if dust > 0 { - mergefi_common::sort_remainders_desc(&mut order); - let dust = u32::try_from(dust).map_err(|_| Error::ArithmeticOverflow)?; - for k in 0..dust { - let (index, _, _) = order.get(k).unwrap(); - let (recipient, share) = shares.get(index).unwrap(); - shares.set( - index, - ( - recipient, - share.checked_add(1).ok_or(Error::ArithmeticOverflow)?, - ), - ); - } - } - - for (recipient, share) in shares.iter() { - if share > 0 { - token_client.transfer(&contract_address, &recipient, &share); + + if contribution.remaining > 0 { + token_client.transfer(&contract_address, &contribution.sponsor, &contribution.remaining); MilestoneRefundPaid { milestone_id, - sponsor: recipient, - amount: share, + sponsor: contribution.sponsor, + amount: contribution.remaining, } .publish(env); } diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 1b8716a6..400610ef 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -726,24 +726,42 @@ fn test_deallocate_requires_admin_auth() { } #[test] -fn test_deallocate_blocked_while_paused() { +fn test_deallocate_allowed_while_paused_for_sponsor_protection() { + // Verifies fix for issue #752: deallocate must be allowed while paused + // so sponsors can recover allocated-but-unreleased funds during a pause. + // Demonstrates the sponsor-protective path: deallocate → cancel_milestone → refund. 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 (token_addr, asset_client, token_client) = create_token(&env, &token_admin); let sponsor = Address::generate(&env); - asset_client.mint(&sponsor, &10_000_000_000i128); - client.create_milestone(&95u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); - client.allocate(&95u64, &950u64, &1_000_000_000i128); - + asset_client.mint(&sponsor, &10_000i128); + + // Create milestone and allocate funds to an issue + client.create_milestone(&95u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&95u64, &950u64, &6_000i128); + + let milestone = client.get_milestone(&95u64); + assert_eq!(milestone.total_budget, 10_000i128); + assert_eq!(milestone.remaining_budget, 4_000i128); // 10000 - 6000 allocated + + // Contract is paused due to emergency client.pause(); - let result = client.try_deallocate(&95u64, &950u64); - assert_eq!(result, Err(Ok(Error::ContractPaused))); - - client.unpause(); + + // Sponsor wants to recover funds during pause. Deallocate must work + // to move allocated funds back to remaining_budget. client.deallocate(&95u64, &950u64); + + let milestone = client.get_milestone(&95u64); + assert_eq!(milestone.remaining_budget, 10_000i128); // 4000 + 6000 back from deallocation + + // Now sponsor can recover via cancel_milestone (also allowed while paused) + client.cancel_milestone(&95u64); + + // Sponsor got their full 10_000 back + assert_eq!(token_client.balance(&sponsor), 10_000i128); } #[test] @@ -883,6 +901,82 @@ fn test_contribute_grows_pool_and_remaining_budget() { assert_eq!(milestone.contributor_count, 1); } +#[test] +fn test_late_contributor_refund_correctness() { + // This test demonstrates the fix for issue #750: A late contributor who + // joins after some funds have already been released should get back their + // full contribution (minus any releases drawn from it via FIFO), not a + // pro-rata share based on total_budget that includes already-spent funds. + // + // Scenario: A creates milestone with 100, then 90 is released to contributors. + // B then contributes 100. On cancel, remaining = 110. + // + // BEFORE FIX (WRONG): Each gets (remaining * contribution / total_budget) + // A gets: 110 * 100 / 200 = 55 + // B gets: 110 * 100 / 200 = 55 + // Result: B loses 45, A gets back 45 that was already spent on A's behalf. + // + // AFTER FIX (CORRECT): FIFO tracking of contribution.remaining + // A's remaining after 90 release: 100 - 90 = 10 + // B's remaining (nothing spent yet): 100 + // A gets: 10 + // B gets: 100 + // Result: Each sponsor gets back exactly their unspent remainder. + + 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_a = Address::generate(&env); + let sponsor_b = Address::generate(&env); + asset_client.mint(&sponsor_a, &100i128); + asset_client.mint(&sponsor_b, &100i128); + + // Sponsor A opens the milestone with 100 + client.create_milestone(&100u64, &sponsor_a, &token_addr, &100i128, &1_000u64); + + let milestone = client.get_milestone(&100u64); + assert_eq!(milestone.total_budget, 100i128); + assert_eq!(milestone.remaining_budget, 100i128); + + // Allocate and release 90 from A's contribution (FIFO deduction from contribution 0) + client.allocate(&100u64, &1001u64, &90i128); + let maintainer = Address::generate(&env); + client.release_issue(&100u64, &1001u64, &vec![&env, (maintainer, 10_000u32)]); + + // After release, remaining_budget = 10 + let milestone = client.get_milestone(&100u64); + assert_eq!(milestone.remaining_budget, 10i128); + + // NOW sponsor B contributes 100 + client.contribute(&100u64, &sponsor_b, &100i128); + + let milestone = client.get_milestone(&100u64); + assert_eq!(milestone.total_budget, 200i128); // 100 + 100 + assert_eq!(milestone.remaining_budget, 110i128); // 10 + 100 + assert_eq!(milestone.contributor_count, 2); + + // Check contribution.remaining values + let contrib_a = client.get_contribution(&100u64, &0); + let contrib_b = client.get_contribution(&100u64, &1); + assert_eq!(contrib_a.amount, 100i128); // A originally contributed 100 + assert_eq!(contrib_a.remaining, 10i128); // 90 was spent via FIFO + assert_eq!(contrib_b.amount, 100i128); // B contributed 100 + assert_eq!(contrib_b.remaining, 100i128); // Nothing spent from B yet + + // Cancel the milestone and verify correct refunds + client.cancel_milestone(&100u64); + + // A should get back only their unspent 10 (not 55 under old bug) + // B should get back their full 100 (not 55 under old bug) + assert_eq!(token_client.balance(&sponsor_a), 10i128); + assert_eq!(token_client.balance(&sponsor_b), 100i128); + assert_eq!(client.get_milestone(&100u64).remaining_budget, 0); +} + #[test] fn test_contribute_requires_sponsor_auth() { let env = Env::default(); diff --git a/contracts/milestones/src/types.rs b/contracts/milestones/src/types.rs index 02947e86..88fa93e6 100644 --- a/contracts/milestones/src/types.rs +++ b/contracts/milestones/src/types.rs @@ -42,7 +42,11 @@ pub struct Milestone { #[derive(Clone, Debug, Eq, PartialEq)] pub struct Contribution { pub sponsor: Address, + /// Original amount contributed by this sponsor pub amount: i128, + /// Remaining unspent amount from this contribution (starts equal to `amount`, + /// decreases as releases are drawn from it FIFO) + pub remaining: i128, pub timestamp: u64, } diff --git a/docs/access-control-audit.md b/docs/access-control-audit.md index 8239ff94..91f701e7 100644 --- a/docs/access-control-audit.md +++ b/docs/access-control-audit.md @@ -68,7 +68,7 @@ signature requirement on any particular address. | `get_version` | Permissionless (view) | n/a | none | Match (new function) | | `set_oracle` | Admin-only | n/a | `require_admin(...).require_auth()` + `new_oracle.require_auth()` | Match (new function) | | `set_fee_bps` | Admin-only | n/a | `require_admin(...).require_auth()` | Match (new function) | -| `deallocate` | Admin-only | n/a | `require_admin(...).require_auth()`; **not pause-gated** (unlike `allocate`/`release_issue`) — a pause-gating inconsistency tracked separately | Match (new function) | +| `deallocate` | Admin-only | n/a | `require_admin(...).require_auth()`; **intentionally not pause-gated** (unlike `allocate`/`release_issue`) — deallocate is sponsor-protective (moves funds toward refund), so it remains available during pause per design principle 1 (issue #752) | Match (new function) | | `cancel_milestone_after_deadline` | Permissionless after `deadline + GRACE_PERIOD` (deliberate) | n/a | none; rejects with `DeadlineNotPassed` before then | Match (new function) | | `get_max_sponsors` | Permissionless (view) | n/a | none | Match (new function) | | `get_oracle` | Permissionless (view) | n/a | none | Match (new function) | diff --git a/docs/upgrade-storage-migration-design.md b/docs/upgrade-storage-migration-design.md index 310e1fe8..1c63fd1d 100644 --- a/docs/upgrade-storage-migration-design.md +++ b/docs/upgrade-storage-migration-design.md @@ -29,9 +29,9 @@ Primary references: ### Retrofitting Already-Deployed Contracts -**Current Status**: The three deployed testnet contracts have **no upgrade function and cannot support in-place upgrades**. Any new code requires a full redeploy. +**Current Status**: All three contracts now have an admin-gated `upgrade(new_wasm_hash)` function that supports in-place upgrades by calling `env.deployer().update_current_contract_wasm()`. The upgrade mechanism is implemented and functional. However, the migration step (calling `migrate(from_version)` after the upgrade) is not yet automated and must be invoked manually by the admin (tracked in #458). -**Mitigation**: Before mainnet, redeploy all three contracts with upgrade support baked in from the start (no need to retrofit the testnet instances; they can be abandoned). +**Deployment Readiness**: The contracts are ready for testnet and mainnet deployment with full upgrade support built in from the start. ## Storage Versioning Strategy @@ -62,7 +62,7 @@ pub enum DataKey { // Version tracking const CONTRACT_VERSION: u32 = 2; -fn get_contract_version(env: &Env) -> u32 { +fn get_version(env: &Env) -> u32 { env.storage() .instance() .get(&DataKey::Version) @@ -109,7 +109,7 @@ pub fn migrate(env: Env, from_version: u32) -> Result<(), Error> { let admin = require_admin(&env).ok_or(Error::NotInitialized)?; admin.require_auth(); - let current_version = get_contract_version(&env); + let current_version = get_version(&env); if current_version == CONTRACT_VERSION { extend_instance_ttl(&env); return Ok(()); @@ -151,7 +151,7 @@ If a two-key oracle/admin model is implemented: 1. **Upgrade Without Data**: Deploy a new version, verify the contract initializes cleanly. 2. **Upgrade With Data**: Upgrade a contract with existing escrows/milestones/deposits, verify data survives and remains accessible. -3. **Version Tracking**: Verify that `get_contract_version()` returns 0 before first upgrade, and increments correctly. +3. **Version Tracking**: Verify that `get_version()` returns 0 before first upgrade, and increments correctly. 4. **Lazy Migration**: Add optional fields to a struct, verify old entries still deserialize and new fields fill with defaults. 5. **Explicit Migration**: Trigger `migrate_storage()` on a contract with outdated entries, verify correctness of rewritten entries. @@ -159,9 +159,9 @@ If a two-key oracle/admin model is implemented: ### Testnet (Current) -1. The three currently-deployed testnet contracts **cannot be upgraded in place** (no upgrade function). -2. **Recommendation**: Do not invest in retrofitting them. Instead, redeploy all three with upgrade support before mainnet testing. -3. Any data on the old testnet instances is abandoned; `mergefi-backend` migrates to the new contract IDs. +1. All three contracts now include the `upgrade(new_wasm_hash)` function and support in-place upgrades. +2. Contracts deployed with upgrade support can be upgraded without redeployment or data loss. +3. The `migrate(from_version)` step after upgrade must be called manually by the admin (automation tracked in #458). ### Mainnet (Pre-Launch) @@ -179,8 +179,9 @@ Soroban does not support "downgrading" a contract (reverting to an older wasm ha ## Backward Compatibility -- **Existing testnet contracts**: No action required; they will be abandoned and redeployed. -- **Existing mainnet contracts (if any, before this feature lands)**: A one-time "activate upgrade support" redeploy is required. This is why the recommendation is to land this feature *before* mainnet launch. +- **Current contracts**: All three contracts (escrow, milestones, maintenance-pool) include upgrade support as of the current codebase. +- **Testnet deployments**: New deployments include upgrade functionality by default. Legacy deployments without upgrade support would need redeployment, but this is a non-issue as the current code already includes the feature. +- **Mainnet launch**: Contracts will be deployed with upgrade support built in from day one. ## Future Work