From 7636c9d2499b073b6d43cc822d02369f3eab63fc Mon Sep 17 00:00:00 2001 From: merge-test Date: Thu, 23 Jul 2026 17:32:03 +0100 Subject: [PATCH] fix: don't mark refund_claimed when a claim transfers no value (#94) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claim_refund marked donor_record.refund_claimed = true before iterating assets, so a claim in which every pro-rata amount came out to zero (or every accepted asset lacked an issuer) still flagged the donor as refunded and emitted refund_claimed — even though no value moved. The donor could then never retry if conditions changed. The claim now runs in two passes: 1. compute each asset's refund (storage reads + pure math only), and 2. only when at least one transfer will actually happen, mark refund_claimed, verify balances, transfer, and emit events. The mark still precedes every token interaction, preserving the checks-effects-interactions ordering from the Issue #242 reentrancy work. A zero-value claim is now a no-op: no state change, no events, and the donor may claim again later. Regression tests: a zero-transfer claim leaves refund_claimed false, emits nothing, and stays retryable; a zero-numerator (fully released) claim likewise leaves the donor unclaimed. Closes #94 --- campaign/src/lib.rs | 78 ++++++++++++--------- campaign/src/test/claim_refund_tests.rs | 92 ++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 32 deletions(-) diff --git a/campaign/src/lib.rs b/campaign/src/lib.rs index 396593be..a88344a3 100644 --- a/campaign/src/lib.rs +++ b/campaign/src/lib.rs @@ -458,6 +458,9 @@ impl CampaignContract { /// Issue #242 – Reentrancy protection: acquires lock at entry, releases at exit. /// Issue #243 – Authorization: `donor.require_auth()`. /// Issue #244 – Balance verification: checks contract balance before each transfer. + /// Issue #94 – `refund_claimed` is only set (and `refund_claimed` emitted) when + /// at least one transfer actually happens; a zero-value claim leaves the donor + /// unclaimed so they may retry if conditions change. /// /// # Panics /// - `Error::NotInitialized` if campaign not initialized @@ -499,11 +502,10 @@ impl CampaignContract { let refund_numerator = campaign.raised_amount - total_released; let refund_denominator = campaign.raised_amount; - // Mark refund as claimed early to prevent reentrancy - donor_record.refund_claimed = true; - set_donor(&env, &donor, &donor_record); - - // For each asset the donor contributed to, calculate and transfer refund + // Issue #94 – First pass (storage reads + pure math only): compute + // each asset's refund so we know whether this claim moves any value + // BEFORE mutating donor state. No external calls happen here. + let mut pending_refunds: Vec<(Address, i128)> = Vec::new(&env); for asset in campaign.accepted_assets.iter() { let asset_address = match &asset.issuer { Some(addr) => addr.clone(), @@ -524,36 +526,50 @@ impl CampaignContract { ); if refund_amount > 0 { - // Issue #244 – Verify contract balance before transfer - use soroban_sdk::token; - let token_client = token::Client::new(&env, &asset_address); - let contract_balance = - token_client.balance(&env.current_contract_address()); - if contract_balance < refund_amount { - panic_with_error(&env, Error::InsufficientContractBalance); - } - - // Transfer refund to donor - token_client.transfer( - &env.current_contract_address(), - &donor, - &refund_amount, - ); - - // Emit event for this asset's refund - env.events().publish( - ("campaign", "asset_refund"), - (donor.clone(), asset_address, refund_amount), - ); + pending_refunds.push_back((asset_address, refund_amount)); } } } - // Emit overall refund claimed event - env.events().publish( - ("campaign", "refund_claimed"), - (&donor, donor_record.total_donated), - ); + // Issue #94 – Only mark the refund claimed when at least one + // transfer will actually happen. The mark still precedes every + // token interaction (checks-effects-interactions), preserving the + // Issue #242 reentrancy posture. A zero-value claim leaves the + // donor unclaimed so they can retry if conditions change. + if !pending_refunds.is_empty() { + donor_record.refund_claimed = true; + set_donor(&env, &donor, &donor_record); + + for (asset_address, refund_amount) in pending_refunds.iter() { + // Issue #244 – Verify contract balance before transfer + use soroban_sdk::token; + let token_client = token::Client::new(&env, &asset_address); + let contract_balance = + token_client.balance(&env.current_contract_address()); + if contract_balance < refund_amount { + panic_with_error(&env, Error::InsufficientContractBalance); + } + + // Transfer refund to donor + token_client.transfer( + &env.current_contract_address(), + &donor, + &refund_amount, + ); + + // Emit event for this asset's refund + env.events().publish( + ("campaign", "asset_refund"), + (donor.clone(), asset_address, refund_amount), + ); + } + + // Emit overall refund claimed event + env.events().publish( + ("campaign", "refund_claimed"), + (&donor, donor_record.total_donated), + ); + } refresh_report_cache(&env); diff --git a/campaign/src/test/claim_refund_tests.rs b/campaign/src/test/claim_refund_tests.rs index 4604f3f4..6a4916ba 100644 --- a/campaign/src/test/claim_refund_tests.rs +++ b/campaign/src/test/claim_refund_tests.rs @@ -7,7 +7,7 @@ use core::ops::Add; -use soroban_sdk::testutils::{Address as AddressTestUtils, Ledger}; +use soroban_sdk::testutils::{Address as AddressTestUtils, Events, Ledger}; use soroban_sdk::token::{StellarAssetClient, TokenClient}; use soroban_sdk::{log, vec, Address, Env, Vec}; @@ -645,3 +645,93 @@ fn test_claim_refund_ended_full_refund() { let contract_balance = token.balance(&contract_address); assert_eq!(contract_balance, 0); } + +// ─── Issue #94 – zero-value claim must not mark refund_claimed ─────────────── + +/// Issue #94 regression: a claim that transfers nothing (donor has no per-asset +/// donation recorded, so the pro-rata loop finds no value to move) must NOT +/// mark `refund_claimed`, must emit no events, and must remain retryable. +#[test] +fn test_claim_refund_zero_transfer_leaves_unclaimed() { + let env = make_env(); + env.ledger().set_timestamp(BASE); + env.mock_all_auths(); + // One `as_contract` frame per invocation: re-authorizing the same donor + // twice inside a single frame trips "frame is already authorized". + let contract_id = env.register_contract(None, CampaignContract); + + let donor = env.as_contract(&contract_id, || { + let end_time = env.ledger().timestamp() + 1000; + create_test_campaign(&env, CampaignStatus::Cancelled, 1000, end_time, 1); + create_test_milestone(&env, 0, 1000, MilestoneStatus::Locked); + + let donor = Address::generate(&env); + create_test_donor(&env, &donor, 100, false); + // Deliberately no DonorAssetDonation entry: nothing can transfer. + donor + }); + + env.as_contract(&contract_id, || { + CampaignContract::claim_refund(env.clone(), donor.clone()); + + // The donor must NOT be marked claimed — no value moved. + let record = crate::storage::get_donor(&env, &donor).unwrap(); + assert!( + !record.refund_claimed, + "zero-value claim must not set refund_claimed" + ); + + // No refund_claimed / asset_refund event may be emitted for a no-op claim. + assert!( + env.events().all().events().is_empty(), + "zero-value claim must not emit events" + ); + }); + + // And the donor can retry: a second call must not panic with + // RefundAlreadyClaimed and must still leave the record unclaimed. + env.as_contract(&contract_id, || { + CampaignContract::claim_refund(env.clone(), donor.clone()); + let record = crate::storage::get_donor(&env, &donor).unwrap(); + assert!(!record.refund_claimed); + }); +} + +/// Issue #94 regression: when everything raised was already released +/// (refund numerator = 0) the pro-rata refund is 0 for every asset — the claim +/// transfers nothing and must leave `refund_claimed` false. +#[test] +fn test_claim_refund_zero_prorata_leaves_unclaimed() { + let env = make_env(); + env.ledger().set_timestamp(BASE); + env.mock_all_auths(); + with_contract(&env, || { + let end_time = env.ledger().timestamp() + 1000; + let campaign = create_test_campaign(&env, CampaignStatus::Cancelled, 1000, end_time, 1); + // Milestone released in full: raised (1000) == released (1000) → numerator 0. + create_test_milestone(&env, 0, 1000, MilestoneStatus::Released); + + let donor = Address::generate(&env); + create_test_donor(&env, &donor, 100, false); + let token_issuer = campaign + .accepted_assets + .get(0) + .unwrap() + .issuer + .clone() + .unwrap(); + env.storage().persistent().set( + &DataKey::DonorAssetDonation(donor.clone(), token_issuer), + &100i128, + ); + + CampaignContract::claim_refund(env.clone(), donor.clone()); + + let record = crate::storage::get_donor(&env, &donor).unwrap(); + assert!( + !record.refund_claimed, + "claim with zero pro-rata refund must not set refund_claimed" + ); + assert!(env.events().all().events().is_empty()); + }); +}