Skip to content

[MEDIUM] DonorRecord::apply_donation uses saturating_add — silently caps total_donated at i128::MAX #36

Description

@Alqku

Overview

DonorRecord::apply_donation (campaign/src/types.rs lines 451–462) uses saturating_add for both total_donated and donation_count. The rest of the contract consistently uses checked_add(...).unwrap_or_else(|| panic_with_error(&env, Error::Overflow)). The saturating variant silently caps total_donated at i128::MAX for any donor who manages to push past the cap, after which every subsequent refund and proportional calculation is wrong (it always reports "fully refunded, no remaining balance"). The whole campaign contract freezes on the canonical Error path elsewhere; the inconsistency is a footgun.

Evidence

// campaign/src/types.rs (impl DonorRecord)
pub fn apply_donation(&mut self, amount: i128, time: u64, ledger: u32, asset: AssetInfo) {
    self.total_donated = self.total_donated.saturating_add(amount);
    self.last_donation_time = time;
    self.last_donation_ledger = ledger;
    self.donation_count = self.donation_count.saturating_add(1);
    self.asset = asset;
}

Compare with campaign/src/lib.rs::donate line 196:

campaign.raised_amount = campaign
    .raised_amount
    .checked_add(amount)
    .unwrap_or_else(|| panic_with_error(&env, Error::Overflow));

And storage_increment_total_raised in storage.rs line 199:

let new_total = current
    .checked_add(delta)
    .unwrap_or_else(|| panic_with_error!(env, Error::Overflow));

Impact

  • Saturating total_donated masks overflow. Once the donor's ledger hits i128::MAX, every later state read is silently wrong.
  • The refund math (campaign/src/lib.rs claim_refund line 401) uses this value: a saturated donor would always see refund = 0 for any further donation, defeating pro-rata refunds entirely.
  • Off-chain indexers that query DonorRecord would have to special-case the saturation; indexed UIs would silently misreport totals.
  • Violates the project-wide "fail loudly on arithmetic overflow" convention documented in Error::Overflow.

Recommended Approach

Either delete apply_donation entirely (it is not currently called by donate — see #38) or replace the saturating lines with typed overflow handling that requires an &Env so it can panic with Error::Overflow:

pub fn apply_donation(&mut self, env: &Env, amount: i128, time: u64, ledger: u32, asset: AssetInfo) {
    self.total_donated = self.total_donated
        .checked_add(amount)
        .unwrap_or_else(|| panic_with_error!(env, Error::Overflow));
    self.last_donation_time = time;
    self.last_donation_ledger = ledger;
    self.donation_count = self.donation_count
        .checked_add(1)
        .unwrap_or_else(|| panic_with_error!(env, Error::Overflow));
    self.asset = asset;
}

If kept, this also fixes the donation_count saturation at the same time (see #49).

Acceptance Criteria

  • DonorRecord::apply_donation no longer uses saturating_add for totals
  • On overflow the function surfaces the typed Error::Overflow panic
  • Existing donate() test paths still pass (whether or not apply_donation is plugged in)
  • Negative-path test exercises overflow of total_donated to i128::MAX
  • Code comment cites this issue so future contributors do not re-saturate

Affected Files

  • campaign/src/types.rs
  • campaign/src/lib.rs (if apply_donation is wired in)
  • campaign/src/test/integration_tests.rs or a new overflow test

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial CampaignCampaign: Official CampaignbugSomething isn't workingcorrectnessIncorrect behavior or state bug

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions