[LOW] release_milestone_multi_asset silences asset_raised underflow with .unwrap_or(0).max(0) — should panic with typed Error

[Alqku]

Overview

release_milestone_multi_asset (multi_asset_release.rs lines 188-189) clamps the decremented per-asset raised counter using .checked_sub(...).unwrap_or(0).max(0) — so if for any reason the on-contract asset_raised ledger is less than the amount being subtracted (rounding, off-by-one in a deposition event, an integer-truncation artifact in compute_asset_release, or a future bug elsewhere), the subtraction silently collapses to 0 instead of producing a typed panic. The surrounding pattern in the same function is "panic with Error::Overflow" (see the function body around line 193), so the silent clamp is inconsistent with the typed-error contract and hides an invariant violation.

Evidence

// campaign/src/multi_asset_release.rs
storage_set_asset_raised(env, &token_address, asset_raised);

let new_asset_raised = asset_raised
    .checked_sub(clamped_release)
    .unwrap_or(0)        // <-- silent on underflow
    .max(0);             // <-- second clamp, same intent

Compare with the release_amount calculations just above:

let milestone_release = milestone.target_amount
    .checked_sub(milestone.released_amount)
    .unwrap_or_else(|| panic_with_error!(env, Error::MilestoneReleasedExceedsTarget));

The release_amount path underflows to a typed panic; the asset_raised decrement underflows to 0.

Impact

• A donor / sub-account that donates extra funds directly (outside the canonical donate() path, e.g. a Stellar fee refund or a forwarder) could push the contract's actual STROOP balance for an asset above asset_raised, then a release that clamped_release = asset_balance (not asset_raised) would underflow the subtraction. The ledger becomes permanently out of sync with reality.
• Subsequent refund calculations read storage_get_asset_raised; if it dropped to 0 prematurely, pro-rata refund math starts producing 0s for legitimate donors.
• Indexers that compute asset_raised == contract_balance invariants will report a non-existent mismatch.

Recommended Approach

Replace the silent clamp with a typed panic that mirrors the rest of the function:

let new_asset_raised = asset_raised
    .checked_sub(clamped_release)
    .unwrap_or_else(|| panic_with_error!(env, Error::Overflow));
if new_asset_raised < 0 {
    panic_with_error!(env, Error::Overflow);
}
storage_set_asset_raised(env, &token_address, new_asset_raised);

Or add a new Error::LedgerUnderflow variant (e.g. = 81 like the existing numbered scheme) so the cause is unmistakable in logs. The invariant is: asset_raised >= clamped_release should always hold by construction; if it doesn't, surface the bug loudly rather than panicking under "overflow" attribution.

Acceptance Criteria

• release_milestone_multi_asset does NOT call .unwrap_or(0).max(0) to silence underflow on asset_raised decrement
• The first occurrence of underflow triggers a typed panic with a meaningful error variant
• Test added that forces the underflow (e.g. by injecting funds higher than the recorded ledger) and asserts the typed panic
• Surrounding totals (storage_set_total_raised after the loop) are reconciled consistently with the same approach

Affected Files

• campaign/src/multi_asset_release.rs
• campaign/src/types.rs (if a new Error::LedgerUnderflow variant is added)
• campaign/src/test/release_milestone_tests.rs (or multi_asset_release.rs test mod)

[cybermax4200]

I would like to work on this issue. I have experience with Rust and smart contract development, particularly around preserving accounting invariants and ensuring that state inconsistencies fail loudly rather than being silently masked.

For this task, I will remove the .unwrap_or(0).max(0) behavior in release_milestone_multi_asset and replace it with typed error handling consistent with the rest of the function. The invariant that asset_raised >= clamped_release should always hold, so any violation should surface immediately through a meaningful panic rather than silently resetting the counter to zero.

I will review the surrounding accounting logic to ensure that the per-asset and aggregate totals follow the same error-handling approach. If appropriate, I will introduce a dedicated Error::LedgerUnderflow variant to make the cause explicit in logs and debugging output.

In addition, I will add tests that intentionally trigger the underflow condition and verify that the expected typed error is raised, ensuring that future regressions cannot silently corrupt refund calculations or desynchronize ledger state from contract balances.

My focus will be on preserving contract correctness and making invariant violations observable and debuggable rather than silently recoverable.

[Promise278]

i have experience with system designs and archecture please i would love to work on these

[grantfox-oss]

🦊 GrantFox — @Promise278 has been assigned to this issue as part of the Official Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #48)
2. Your PR will be reviewed by the OrbitChainLabs maintainers

Good luck! Track your progress on GrantFox.