[LOW] WithdrawalLimits::validate uses unchecked already_withdrawn + amount (i128 overflow at large caps)

[Alqku]

Overview

WithdrawalLimits::validate computes already_withdrawn + amount to check the campaign total cap (crates/tools/src/withdrawal_limits.rs line 60), without checked arithmetic. If either operand is large (configurable up to i128::MAX since WithdrawalLimits::new does not bound max_total), the sum overflows i128 and panics or wraps. The contract never observes this overflow in its default paths today (defaults max out at 1000 XLM), but a misconfigured OrbitChainConfig that pushes max_total to billions of stroops could trigger the bug.

Evidence

// crates/tools/src/withdrawal_limits.rs (WithdrawalLimits::validate)
pub fn validate(&self, amount: i128, already_withdrawn: i128) -> Result<()> {
    ...
    if let Some(cap) = self.max_total {
        if already_withdrawn + amount > cap {           // <-- unchecked i128 add
            return Err(anyhow!(
                "Total withdrawn {} would exceed the campaign cap of {}",
                already_withdrawn + amount, cap,
            ));
        }
    }
    Ok(())
}

WithdrawalLimits::new accepts any max: i128 ≥ min with no upper bound (new(min, max, max_total) line 40). The default cap is None, so the path is opt-in, but a future caller forging a large max_total exposes the overflow.

Impact

• An overflowed already_withdrawn + amount either panics (silently killing a withdrawal) or, with default i128 wrap in release builds, produces a wildly wrong comparison that may VASTLY under-report usability — defeating the entire purpose of a cap.
• The same overflow reappears in the error-message formatting, so operators get a confusing number ("-1234…") in their logs when a cap check fires.
• The Result<()> ergonomics invite callers to quietly .ok() the result, swallowing the panic-equivalent path.

Recommended Approach

Use checked arithmetic for both the comparison and the user-facing error message:

let new_total = already_withdrawn
    .checked_add(amount)
    .ok_or_else(|| anyhow!("Withdrawal arithmetic overflow: already_withdrawn + amount exceeds i128"))?;
if new_total > cap {
    return Err(anyhow!(
        "Total withdrawn {} would exceed the campaign cap of {}",
        new_total, cap,
    ));
}

Optionally document an upper bound on WithdrawalLimits::new (e.g. require!(max_total >= 0 && max_total < i128::MAX / 2, ...)) so callers cannot construct a nonsensical cap.

Acceptance Criteria

• + in WithdrawalLimits::validate replaced with checked_add and a typed error on overflow
• Error message uses the pre-computed new_total (no recomputation)
• Test added that exercises overflow at i128::MAX cap and asserts a clear error, not a panic
• If an upper-bound constructor rule is added, release notes / WithdrawalLimits::new docs mention it

Affected Files

• crates/tools/src/withdrawal_limits.rs
• (Possibly crates/tools/tests/integration_test.rs for the new overflow test)

[Tobi-8]

Please maintainer
Assign me to this issue
I can confidently solve it with my experience

[AugistineCreates]

Please assign this issue to me my eta is 4 hours

[grantfox-oss]

🦊 GrantFox — @AugistineCreates 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 #42)
2. Your PR will be reviewed by the OrbitChainLabs maintainers

Good luck! Track your progress on GrantFox.