From fec4ef1e0ddd5f7b3877b5cd6dd7294a052aaedf Mon Sep 17 00:00:00 2001 From: Edukpe David Date: Wed, 29 Jul 2026 22:00:29 +0000 Subject: [PATCH 1/2] fix(#445): replace unchecked i128 addition with checked_add to prevent overflow Replace all unchecked i128 additions in contributions.rs and revenue.rs with checked_add(...).ok_or(Error::Overflow)? to prevent silent wrapping/overflow on high-balance tokens. Fixes: - contributions.rs: check_contribution_caps cap comparison - contributions.rs: update_contribution_accounting (5 locations): amount_raised, effective_amount_raised, contribution, lifetime, total_raised - contributions.rs: contribute() personal cap check - contributions.rs: batch_contribute() personal cap check - revenue.rs: deposit_revenue pool accumulation - revenue.rs: claim_revenue claimed + distributed accumulation - revenue.rs: claim_creator_revenue claimed accumulation Also changes update_contribution_accounting return type from () to Result<(), Error> so overflow errors propagate to callers. Note: verify_with_votes in voting.rs already uses checked_add for total_votes and total_weight (fixed in prior commit 0876e60). --- src/contributions.rs | 46 +++++++++++++++++++++++++++++++++----------- src/revenue.rs | 31 +++++++++++++++++++++++++---- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/contributions.rs b/src/contributions.rs index c436cc52..3e853f71 100644 --- a/src/contributions.rs +++ b/src/contributions.rs @@ -23,7 +23,10 @@ fn check_contribution_caps( amount: i128, ) -> Result<(), Error> { if campaign.max_contribution_per_user > 0 - && current_lifetime_contribution + amount > campaign.max_contribution_per_user + && current_lifetime_contribution + .checked_add(amount) + .ok_or(Error::Overflow)? + > campaign.max_contribution_per_user { return Err(Error::ContributionCapExceeded); } @@ -97,19 +100,40 @@ fn update_contribution_accounting( current: i128, lifetime: i128, amount: i128, -) { - campaign.amount_raised += amount; - campaign.effective_amount_raised += amount; +) -> Result<(), Error> { + campaign.amount_raised = campaign + .amount_raised + .checked_add(amount) + .ok_or(Error::Overflow)?; + campaign.effective_amount_raised = campaign + .effective_amount_raised + .checked_add(amount) + .ok_or(Error::Overflow)?; set_campaign(env, campaign_id, campaign); - set_contribution(env, campaign_id, contributor, current + amount); - set_lifetime_contribution(env, campaign_id, contributor, lifetime + amount); + set_contribution( + env, + campaign_id, + contributor, + current.checked_add(amount).ok_or(Error::Overflow)?, + ); + set_lifetime_contribution( + env, + campaign_id, + contributor, + lifetime.checked_add(amount).ok_or(Error::Overflow)?, + ); if lifetime == 0 { increment_contributor_count(env, campaign_id); } let total_raised = get_total_raised_global(env); - set_total_raised_global(env, total_raised + amount); + set_total_raised_global( + env, + total_raised.checked_add(amount).ok_or(Error::Overflow)?, + ); + + Ok(()) } pub(crate) fn contribute( @@ -145,7 +169,7 @@ pub(crate) fn contribute( check_contribution_caps(&campaign, lifetime, amount)?; if let Some(cap) = get_personal_cap(env, campaign_id, &contributor) { - if current + amount > cap { + if current.checked_add(amount).ok_or(Error::Overflow)? > cap { return Err(Error::ContributionCapExceeded); } } @@ -164,7 +188,7 @@ pub(crate) fn contribute( current, lifetime, amount, - ); + )?; env.events() .publish(("contribution_made", campaign_id, contributor), amount); @@ -217,7 +241,7 @@ pub(crate) fn batch_contribute( check_contribution_caps(&campaign, lifetime, amount)?; if let Some(cap) = get_personal_cap(env, campaign_id, &contributor) { - if current + amount > cap { + if current.checked_add(amount).ok_or(Error::Overflow)? > cap { return Err(Error::ContributionCapExceeded); } } @@ -232,7 +256,7 @@ pub(crate) fn batch_contribute( current, lifetime, amount, - ); + )?; total = total.checked_add(amount).ok_or(Error::Overflow)?; diff --git a/src/revenue.rs b/src/revenue.rs index da075fb1..c046a679 100644 --- a/src/revenue.rs +++ b/src/revenue.rs @@ -38,7 +38,11 @@ pub(crate) fn deposit_revenue(env: &Env, campaign_id: u32, amount: i128) -> Resu client.transfer(&campaign.creator, &env.current_contract_address(), &amount); let current_pool = get_revenue_pool(env, campaign_id); - set_revenue_pool(env, campaign_id, current_pool + amount); + set_revenue_pool( + env, + campaign_id, + current_pool.checked_add(amount).ok_or(Error::Overflow)?, + ); env.events() .publish(("revenue_deposited", campaign_id, campaign.creator), amount); @@ -131,12 +135,25 @@ pub(crate) fn claim_revenue( client.transfer(&env.current_contract_address(), &contributor, &claimable); // Update state only after successful external interaction - set_revenue_claimed(env, campaign_id, &contributor, already_claimed + claimable); + set_revenue_claimed( + env, + campaign_id, + &contributor, + already_claimed + .checked_add(claimable) + .ok_or(Error::Overflow)?, + ); // Track the running sum paid out to contributors, and the count of // distinct contributors who have claimed at least once, so future claims // can detect the last claimant (#526). - set_contributor_revenue_distributed(env, campaign_id, distributed_so_far + claimable); + set_contributor_revenue_distributed( + env, + campaign_id, + distributed_so_far + .checked_add(claimable) + .ok_or(Error::Overflow)?, + ); if is_first_claim { set_contributor_revenue_claimants(env, campaign_id, claimants_so_far + 1); } @@ -187,7 +204,13 @@ pub(crate) fn claim_creator_revenue(env: &Env, campaign_id: u32) -> Result<(), E &claimable, ); - set_creator_revenue_claimed(env, campaign_id, already_claimed + claimable); + set_creator_revenue_claimed( + env, + campaign_id, + already_claimed + .checked_add(claimable) + .ok_or(Error::Overflow)?, + ); env.events().publish( ("creator_revenue_claimed", campaign_id, campaign.creator), From 098af01616cefdc3d5550630b311e74e8c704842 Mon Sep 17 00:00:00 2001 From: Edukpe David Date: Thu, 30 Jul 2026 18:50:07 +0000 Subject: [PATCH 2/2] fix(#442): batch verify_campaigns now returns (verified_ids, failed_ids) --- EVENT_PAYLOADS.md | 4 ++-- src/lib.rs | 34 ++++++++++++++++++++-------------- src/tests/test_lifecycle.rs | 12 +++++++----- src/tests/test_voting.rs | 16 ++++++++++------ 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/EVENT_PAYLOADS.md b/EVENT_PAYLOADS.md index b12f0dd3..2f2d17a8 100644 --- a/EVENT_PAYLOADS.md +++ b/EVENT_PAYLOADS.md @@ -249,8 +249,8 @@ Every `publish(...)` call in the contract, with its topics, data shape, and the | Field | Value | |---------|------------------------------------------------------------| | Topics | `("campaigns_bulk_verified",)` | -| Data | `(verified_count: u32, total: u32)` | -| Source | `lib.rs:1175` — `verify_campaigns()` | +| Data | `(verified_count: u32, failed_count: u32, total: u32)` | +| Source | `lib.rs:256` — `verify_campaigns()` | --- diff --git a/src/lib.rs b/src/lib.rs index d3c21240..4d969280 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -222,7 +222,15 @@ impl ProofOfHeart { voting::admin_verify(&env, campaign_id) } - pub fn verify_campaigns(env: Env, campaign_ids: soroban_sdk::Vec) -> Result { + /// Batch-verifies up to 50 campaigns in a single call. Each campaign is + /// independently verified; failures are collected rather than aborting the + /// batch. Returns `Ok((verified_ids, failed_ids))` so callers can distinguish + /// partial success from total failure (#442). Auth / paused checks still + /// return `Err` and abort the entire batch. + pub fn verify_campaigns( + env: Env, + campaign_ids: soroban_sdk::Vec, + ) -> Result<(soroban_sdk::Vec, soroban_sdk::Vec), Error> { let admin = get_admin(&env); assert_admin(&env, &admin)?; lifecycle::require_not_paused(&env)?; @@ -230,8 +238,8 @@ impl ProofOfHeart { const MAX_BATCH_SIZE: u32 = 50; let batch_size = campaign_ids.len().min(MAX_BATCH_SIZE); - let mut verified_count = 0u32; - let mut first_error: Option = None; + let mut verified_ids = soroban_sdk::Vec::new(&env); + let mut failed_ids = soroban_sdk::Vec::new(&env); bump_instance_ttl(&env); @@ -240,12 +248,10 @@ impl ProofOfHeart { storage::extend_voting_state_ttl(&env, campaign_id); match voting::admin_verify(&env, campaign_id) { Ok(()) => { - verified_count += 1; + verified_ids.push_back(campaign_id); } - Err(e) => { - if first_error.is_none() { - first_error = Some(e); - } + Err(_e) => { + failed_ids.push_back(campaign_id); } } } @@ -253,14 +259,14 @@ impl ProofOfHeart { env.events().publish( ("campaigns_bulk_verified",), - (verified_count, campaign_ids.len()), + ( + verified_ids.len(), + failed_ids.len(), + batch_size, + ), ); - if let Some(err) = first_error { - Err(err) - } else { - Ok(verified_count) - } + Ok((verified_ids, failed_ids)) } pub fn verify_campaign_with_votes(env: Env, campaign_id: u32) -> Result<(), Error> { diff --git a/src/tests/test_lifecycle.rs b/src/tests/test_lifecycle.rs index aa831a23..220a79ee 100644 --- a/src/tests/test_lifecycle.rs +++ b/src/tests/test_lifecycle.rs @@ -377,13 +377,15 @@ fn test_verify_campaigns_extends_ttl_on_failure() { // 3. Verify the campaign successfully first. let ids = soroban_sdk::Vec::from_array(&env, [campaign_id]); - let first_res = client.verify_campaigns(&ids); - assert_eq!(first_res, 1); + let (first_verified, first_failed) = client.verify_campaigns(&ids); + assert_eq!(first_verified.len(), 1); + assert_eq!(first_failed.len(), 0); // Now try to verify the campaign again. - // Since it's already verified, it will fail verification. - let second_res = client.try_verify_campaigns(&ids); - assert!(second_res.is_err()); // verification failed (AdminVerificationConflict error) + // Since it's already verified, it will fail verification and land in failed_ids. + let (second_verified, second_failed) = client.verify_campaigns(&ids); + assert_eq!(second_verified.len(), 0); + assert_eq!(second_failed.len(), 1); // 4. Despite the failure, the voting state TTL should have been extended. let current_ledger = env.ledger().sequence(); diff --git a/src/tests/test_voting.rs b/src/tests/test_voting.rs index 86773b0f..497299a7 100644 --- a/src/tests/test_voting.rs +++ b/src/tests/test_voting.rs @@ -315,8 +315,9 @@ fn test_verify_campaigns_extends_voting_state_ttl() { )); // Bulk verify the campaign - let count = client.verify_campaigns(&soroban_sdk::Vec::from_array(&env, [campaign_id])); - assert_eq!(count, 1); + let (verified, failed) = client.verify_campaigns(&soroban_sdk::Vec::from_array(&env, [campaign_id])); + assert_eq!(verified.len(), 1); + assert_eq!(failed.len(), 0); // Verify campaign is verified (confirming it worked) let campaign = client.get_campaign(&campaign_id); @@ -353,7 +354,7 @@ fn test_vote_on_campaign_after_deadline_returns_deadline_passed() { } #[test] -fn test_verify_campaigns_partial_failure_returns_err() { +fn test_verify_campaigns_partial_failure_returns_ids() { let (env, _admin, creator, _, _, _, _, client) = setup_env(); let campaign_id = client.create_campaign(&make_params( @@ -368,10 +369,13 @@ fn test_verify_campaigns_partial_failure_returns_err() { 0i128, )); - // 999 does not exist — will produce CampaignNotFound + // 999 does not exist — will produce CampaignNotFound, collected as failed_id let ids = soroban_sdk::Vec::from_array(&env, [campaign_id, 999u32]); - let res = client.try_verify_campaigns(&ids); - assert!(res.unwrap_err().is_ok()); // Err variant, inner Ok means contract error + let (verified, failed) = client.verify_campaigns(&ids); + assert_eq!(verified.len(), 1); + assert_eq!(failed.len(), 1); + assert_eq!(verified.get(0).unwrap(), campaign_id); + assert_eq!(failed.get(0).unwrap(), 999u32); } // ── verification via votes ──────────────────────────────────────────────────────