From 297d7904ab92c6320cf18ac0035bd905b4a39591 Mon Sep 17 00:00:00 2001 From: Akatenvictor <“akatenvictor@gmail.com”> Date: Tue, 29 Sep 2026 19:44:21 +0100 Subject: [PATCH] fix: return ArithmeticOverflow instead of panicking on i128 overflow Release profile sets overflow-checks=true + panic=abort, so any unchecked i128 op (pool.balance += amount, total_deposited += amount, total*fee_bps, distributable*bps, allocated += share, remaining*amount, deadline + GRACE_PERIOD, balance_after - balance_before) aborts the whole transaction. Convert every reachable site in escrow, milestones, maintenance-pool, and common::compute_split to checked_*/saturating_* ops returning a new ArithmeticOverflow error (SplitError::Overflow in common, mapped per-contract), so overflows are graceful errors that leave state untouched rather than aborts. --- contracts/common/src/lib.rs | 6 +++- contracts/common/src/split.rs | 31 +++++++++++----- contracts/escrow/src/error.rs | 4 +++ contracts/escrow/src/lib.rs | 13 +++++-- contracts/maintenance-pool/src/error.rs | 4 +++ contracts/maintenance-pool/src/lib.rs | 34 ++++++++++++++---- contracts/milestones/src/error.rs | 4 +++ contracts/milestones/src/lib.rs | 48 +++++++++++++++++++------ 8 files changed, 115 insertions(+), 29 deletions(-) diff --git a/contracts/common/src/lib.rs b/contracts/common/src/lib.rs index 134a585..c6a1a7a 100644 --- a/contracts/common/src/lib.rs +++ b/contracts/common/src/lib.rs @@ -161,7 +161,11 @@ where let balance_before = token_client.balance(contract_addr); operation(); let balance_after = token_client.balance(contract_addr); - balance_after - balance_before + // A malicious or deflationary token could leave `balance_after < + // balance_before`; a plain `-` would panic (abort) under + // `overflow-checks = true`. Saturate at 0: callers already reject + // `actual_received <= 0` with `InvalidAmount`. + balance_after.saturating_sub(balance_before) } /// Extends a persistent entry's TTL to (approximately) survive until diff --git a/contracts/common/src/split.rs b/contracts/common/src/split.rs index 7dc2999..38063a1 100644 --- a/contracts/common/src/split.rs +++ b/contracts/common/src/split.rs @@ -38,6 +38,10 @@ pub enum SplitError { /// A recipient address equals the contract's own address — a self-payout /// that would strand funds in the contract with no recovery path. SelfPayout, + /// Checked i128 arithmetic overflowed. Returned instead of panicking + /// (release profile sets `overflow-checks = true` + `panic = "abort"`, + /// so an unchecked op would abort the whole transaction). + Overflow, } /// Validates that basis-point splits sum to exactly 10000 and computes the @@ -54,7 +58,9 @@ pub fn compute_split( let mut bps_sum: i128 = 0; for (_, bps) in recipients.iter() { - bps_sum += bps as i128; + bps_sum = bps_sum + .checked_add(bps as i128) + .ok_or(SplitError::Overflow)?; } if bps_sum != BPS_DENOMINATOR { return Err(SplitError::InvalidSplit); @@ -71,18 +77,23 @@ pub fn compute_split( } } - let fee = total * (fee_bps as i128) / BPS_DENOMINATOR; - let distributable = total - fee; + let fee = (total + .checked_mul(fee_bps as i128) + .ok_or(SplitError::Overflow)?) + / BPS_DENOMINATOR; + let distributable = total.checked_sub(fee).ok_or(SplitError::Overflow)?; let mut shares: Vec<(Address, i128)> = Vec::new(env); let mut order: Vec<(u32, i128, Address)> = Vec::new(env); let mut allocated: i128 = 0; for (recipient, bps) in recipients.iter() { - let numerator = distributable * (bps as i128); + let numerator = distributable + .checked_mul(bps as i128) + .ok_or(SplitError::Overflow)?; let share = numerator / BPS_DENOMINATOR; let remainder = numerator % BPS_DENOMINATOR; - allocated += share; + allocated = allocated.checked_add(share).ok_or(SplitError::Overflow)?; shares.push_back((recipient.clone(), share)); order.push_back((order.len(), remainder, recipient)); } @@ -94,13 +105,15 @@ pub fn compute_split( // each award only consumes the selected entry and never changes any // other entry's remainder. `dust` is at most `recipients.len() - 1`, so // the first `dust` sorted entries always exist. - let dust = distributable - allocated; + let dust = distributable + .checked_sub(allocated) + .ok_or(SplitError::Overflow)?; if dust > 0 { sort_remainders_desc(&mut order); for k in 0..dust as u32 { let (index, _, _) = order.get(k).unwrap(); let (recipient, share) = shares.get(index).unwrap(); - shares.set(index, (recipient, share + 1)); + shares.set(index, (recipient, share.checked_add(1).ok_or(SplitError::Overflow)?)); } } @@ -122,7 +135,9 @@ fn remainder_order_less(a: &(u32, i128, Address), b: &(u32, i128, Address)) -> b fn sift_down_remainder_order(order: &mut Vec<(u32, i128, Address)>, start: u32, end: u32) { let mut root = start; loop { - let mut child = 2 * root + 1; + // `order.len()` is bounded by MAX_SPONSORS (20); saturating math + // cannot wrap here but avoids any debug/release overflow panic. + let mut child = root.saturating_mul(2).saturating_add(1); if child >= end { break; } diff --git a/contracts/escrow/src/error.rs b/contracts/escrow/src/error.rs index 71ed7f1..7aea5ed 100644 --- a/contracts/escrow/src/error.rs +++ b/contracts/escrow/src/error.rs @@ -29,4 +29,8 @@ pub enum Error { /// A recipient address equals the contract's own address — a self-payout /// that would strand funds in the contract with no recovery path. SelfPayout = 20, + /// Checked i128 arithmetic overflowed (e.g. contribution total near + /// `i128::MAX`). Returned instead of panicking under release + /// `overflow-checks = true` + `panic = "abort"`. + ArithmeticOverflow = 21, } diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 11b019c..0021ecd 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -261,7 +261,10 @@ mod contract { let contribution_key = DataKey::Contribution(issue_id, index); let mut contribution: Contribution = env.storage().persistent().get(&contribution_key).ok_or(Error::ContributionNotFound)?; - contribution.amount += actual_received; + contribution.amount = contribution + .amount + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; contribution.timestamp = env.ledger().timestamp(); env.storage() .persistent() @@ -281,7 +284,10 @@ mod contract { escrow.contributor_count += 1; } - escrow.amount += actual_received; + escrow.amount = escrow + .amount + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; env.storage().persistent().set(&key, &escrow); extend_ttl(&env, &key); extend_instance_ttl(&env); @@ -324,6 +330,7 @@ mod contract { .map_err(|e| match e { mergefi_common::SplitError::InvalidSplit => Error::InvalidSplit, mergefi_common::SplitError::SelfPayout => Error::SelfPayout, + mergefi_common::SplitError::Overflow => Error::ArithmeticOverflow, })?; let treasury: Address = mergefi_common::require_treasury::(&env).unwrap(); let token_client = token::Client::new(&env, &escrow.token); @@ -375,7 +382,7 @@ mod contract { } let now = env.ledger().timestamp(); - if now < escrow.deadline + GRACE_PERIOD { + if now < escrow.deadline.saturating_add(GRACE_PERIOD) { // Not yet expired + grace period: only the admin may force an early refund. let admin = require_admin(&env)?; admin.require_auth(); diff --git a/contracts/maintenance-pool/src/error.rs b/contracts/maintenance-pool/src/error.rs index a3e7655..736b5e3 100644 --- a/contracts/maintenance-pool/src/error.rs +++ b/contracts/maintenance-pool/src/error.rs @@ -27,4 +27,8 @@ pub enum Error { DepositCountOverflow = 14, /// The pool already holds `MAX_DEPOSITS` deposits (issue #94). TooManyDeposits = 15, + /// Checked i128 arithmetic overflowed (e.g. `balance` / monotonic + /// `total_deposited` near `i128::MAX`). Returned instead of panicking + /// under release `overflow-checks = true` + `panic = "abort"`. + ArithmeticOverflow = 16, } diff --git a/contracts/maintenance-pool/src/lib.rs b/contracts/maintenance-pool/src/lib.rs index 53b7383..e51a66b 100644 --- a/contracts/maintenance-pool/src/lib.rs +++ b/contracts/maintenance-pool/src/lib.rs @@ -138,8 +138,14 @@ mod contract { let token_client = token::Client::new(&env, &token); token_client.transfer(&sponsor, env.current_contract_address(), &amount); - pool.balance += amount; - pool.total_deposited += amount; + pool.balance = pool + .balance + .checked_add(amount) + .ok_or(Error::ArithmeticOverflow)?; + pool.total_deposited = pool + .total_deposited + .checked_add(amount) + .ok_or(Error::ArithmeticOverflow)?; let index = pool.deposit_count; pool.deposit_count = pool .deposit_count @@ -200,15 +206,26 @@ mod contract { let fee_bps: u32 = mergefi_common::get_fee_bps::(&env) .ok_or(Error::NotInitialized)?; - let fee = amount * (fee_bps as i128) / BPS_DENOMINATOR; - let payout = amount - fee; + let fee = amount + .checked_mul(fee_bps as i128) + .ok_or(Error::ArithmeticOverflow)? + / BPS_DENOMINATOR; + let payout = amount + .checked_sub(fee) + .ok_or(Error::ArithmeticOverflow)?; let treasury: Address = mergefi_common::require_treasury::(&env).unwrap(); let token_client = token::Client::new(&env, &pool.token); let contract_address = env.current_contract_address(); - pool.balance -= amount; - pool.total_withdrawn += amount; + pool.balance = pool + .balance + .checked_sub(amount) + .ok_or(Error::ArithmeticOverflow)?; + pool.total_withdrawn = pool + .total_withdrawn + .checked_add(amount) + .ok_or(Error::ArithmeticOverflow)?; pool.last_withdraw_at = env.ledger().timestamp(); env.storage().persistent().set(&pkey, &pool); extend_ttl(&env, &pkey); @@ -289,7 +306,10 @@ mod contract { let token_client = token::Client::new(&env, &pool.token); token_client.transfer(&env.current_contract_address(), &sponsor, &deposit.amount); - pool.balance -= deposit.amount; + pool.balance = pool + .balance + .checked_sub(deposit.amount) + .ok_or(Error::ArithmeticOverflow)?; env.storage().persistent().set(&pkey, &pool); extend_ttl(&env, &pkey); extend_instance_ttl(&env); diff --git a/contracts/milestones/src/error.rs b/contracts/milestones/src/error.rs index 3f7a8be..55f4e55 100644 --- a/contracts/milestones/src/error.rs +++ b/contracts/milestones/src/error.rs @@ -33,4 +33,8 @@ pub enum Error { /// A recipient address equals the contract's own address — a self-payout /// that would strand funds in the contract with no recovery path. SelfPayout = 20, + /// Checked i128 arithmetic overflowed (e.g. budget total near + /// `i128::MAX`). Returned instead of panicking under release + /// `overflow-checks = true` + `panic = "abort"`. + ArithmeticOverflow = 21, } diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 4844660..90c53d7 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -251,7 +251,10 @@ mod contract { .persistent() .get(&contribution_key) .ok_or(Error::ContributionNotFound)?; - contribution.amount += actual_received; + contribution.amount = contribution + .amount + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; contribution.timestamp = env.ledger().timestamp(); env.storage() .persistent() @@ -274,8 +277,14 @@ mod contract { // New funds arrive unallocated: the pool's total *and* its // unallocated remainder both grow by exactly the contribution, so // a later proportional refund treats them like any other share. - milestone.total_budget += actual_received; - milestone.remaining_budget += actual_received; + milestone.total_budget = milestone + .total_budget + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; + milestone.remaining_budget = milestone + .remaining_budget + .checked_add(actual_received) + .ok_or(Error::ArithmeticOverflow)?; env.storage().persistent().set(&mkey, &milestone); extend_ttl(&env, &mkey); @@ -328,7 +337,10 @@ mod contract { return Err(Error::OverAllocation); } - milestone.remaining_budget -= amount; + milestone.remaining_budget = milestone + .remaining_budget + .checked_sub(amount) + .ok_or(Error::ArithmeticOverflow)?; milestone.allocations.set(issue_id, amount); env.storage().persistent().set(&mkey, &milestone); extend_ttl(&env, &mkey); @@ -402,6 +414,7 @@ mod contract { .map_err(|e| match e { mergefi_common::SplitError::InvalidSplit => Error::InvalidSplit, mergefi_common::SplitError::SelfPayout => Error::SelfPayout, + mergefi_common::SplitError::Overflow => Error::ArithmeticOverflow, })?; let treasury: Address = mergefi_common::require_treasury::(&env).unwrap(); let token_client = token::Client::new(&env, &milestone.token); @@ -511,7 +524,10 @@ mod contract { .get(issue_id) .ok_or(Error::IssueNotAllocatedForDeallocate)?; - milestone.remaining_budget += amount; + milestone.remaining_budget = milestone + .remaining_budget + .checked_add(amount) + .ok_or(Error::ArithmeticOverflow)?; milestone.allocations.remove(issue_id); if milestone.closed && milestone.remaining_budget > 0 { @@ -593,7 +609,7 @@ mod contract { } let now = env.ledger().timestamp(); - if now < milestone.deadline + GRACE_PERIOD { + if now < milestone.deadline.saturating_add(GRACE_PERIOD) { return Err(Error::DeadlineNotPassed); } @@ -857,21 +873,33 @@ fn refund_remaining_budget( .persistent() .get(&contribution_key) .ok_or(Error::MilestoneNotFound)?; - let numerator = remaining * contribution.amount; + let numerator = remaining + .checked_mul(contribution.amount) + .ok_or(Error::ArithmeticOverflow)?; let share = numerator / milestone.total_budget; let remainder = numerator % milestone.total_budget; - allocated += share; + allocated = allocated + .checked_add(share) + .ok_or(Error::ArithmeticOverflow)?; shares.push_back((contribution.sponsor.clone(), share)); order.push_back((order.len(), remainder, contribution.sponsor)); } - let dust = remaining - allocated; + let dust = remaining + .checked_sub(allocated) + .ok_or(Error::ArithmeticOverflow)?; if dust > 0 { mergefi_common::sort_remainders_desc(&mut order); for k in 0..dust as u32 { let (index, _, _) = order.get(k).unwrap(); let (recipient, share) = shares.get(index).unwrap(); - shares.set(index, (recipient, share + 1)); + shares.set( + index, + ( + recipient, + share.checked_add(1).ok_or(Error::ArithmeticOverflow)?, + ), + ); } }