From dd769b08f905f61cf8b19b7fc7e55208a796fbe8 Mon Sep 17 00:00:00 2001 From: arisu6804 Date: Thu, 27 Aug 2026 21:23:41 +0530 Subject: [PATCH] feat: make fee arithmetic conservation-safe --- contracts/commitment_core/src/lib.rs | 22 +- contracts/commitment_marketplace/src/lib.rs | 43 ++- contracts/shared_utils/src/fee_invariants.rs | 340 +++++++++++++++++++ contracts/shared_utils/src/lib.rs | 3 +- docs/FEE_ACCOUNTING_POLICY.md | 142 ++++++++ 5 files changed, 521 insertions(+), 29 deletions(-) create mode 100644 contracts/shared_utils/src/fee_invariants.rs create mode 100644 docs/FEE_ACCOUNTING_POLICY.md diff --git a/contracts/commitment_core/src/lib.rs b/contracts/commitment_core/src/lib.rs index 0269de6..8f6664f 100644 --- a/contracts/commitment_core/src/lib.rs +++ b/contracts/commitment_core/src/lib.rs @@ -19,7 +19,7 @@ //! [`docs/COMMITMENT_CORE_FORMAL_VERIFICATION_SCOPE.md`](../../../docs/COMMITMENT_CORE_FORMAL_VERIFICATION_SCOPE.md) use shared_utils::{ - emit_error_event, fees, EmergencyControl, Pausable, RateLimiter, SafeMath, TimeUtils, + emit_error_event, fee_invariants, fees, EmergencyControl, Pausable, RateLimiter, SafeMath, TimeUtils, Validation, }; use soroban_sdk::{ @@ -504,15 +504,12 @@ impl CommitmentCoreContract { .instance() .get(&DataKey::CreationFeeBps) .unwrap_or(0); - let creation_fee = if creation_fee_bps > 0 { - fees::fee_from_bps(amount, creation_fee_bps) - } else { - 0 - }; - let net_amount = amount.checked_sub(creation_fee).unwrap_or_else(|| { + let creation_split = fee_invariants::split_bps(amount, creation_fee_bps).unwrap_or_else(|_| { set_reentrancy_guard(&e, false); fail(&e, CommitmentError::ArithmeticOverflow, "create"); }); + let creation_fee = creation_split.fee; + let net_amount = creation_split.net; let expires_at = TimeUtils::checked_calculate_expiration(&e, rules.duration_days) .unwrap_or_else(|| { @@ -1163,11 +1160,16 @@ impl CommitmentCoreContract { fail(&e, CommitmentError::NotActive, "exit"); } - let penalty = SafeMath::penalty_amount( + let penalty_split = fee_invariants::split_percent( commitment.current_value, commitment.rules.early_exit_penalty, - ); - let returned = SafeMath::sub(commitment.current_value, penalty); + ) + .unwrap_or_else(|_| { + set_reentrancy_guard(&e, false); + fail(&e, CommitmentError::ArithmeticOverflow, "exit"); + }); + let penalty = penalty_split.fee; + let returned = penalty_split.net; let original_val = commitment.current_value; // Add penalty to collected fees (protocol revenue) diff --git a/contracts/commitment_marketplace/src/lib.rs b/contracts/commitment_marketplace/src/lib.rs index 71a602c..d8d88e4 100644 --- a/contracts/commitment_marketplace/src/lib.rs +++ b/contracts/commitment_marketplace/src/lib.rs @@ -25,7 +25,7 @@ use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, symbol_short, token, Address, Env, Symbol, Vec, }; -use shared_utils::math::SafeMath; +use shared_utils::{fee_invariants, math::SafeMath}; // ============================================================================ // Error Types @@ -80,6 +80,8 @@ pub enum MarketplaceError { TransferFailed = 21, /// Payment token is not allowlisted for marketplace settlement PaymentTokenNotAllowed = 22, + /// Configured fee cannot be represented as a safe accounting split. + InvalidFeeConfiguration = 23, } // ============================================================================ @@ -610,10 +612,12 @@ impl CommitmentMarketplace { })?; // Calculate fee and seller proceeds safely using basis points (bps) - let fee_basis_points_i128: i128 = fee_basis_points as i128; - let marketplace_fee = - SafeMath::div(SafeMath::mul(listing.price, fee_basis_points_i128), 10_000_i128); - let seller_proceeds = SafeMath::sub(listing.price, marketplace_fee); + let fee_split = fee_invariants::split_bps(listing.price, fee_basis_points).map_err(|_| { + e.storage().instance().set(&DataKey::ReentrancyGuard, &false); + MarketplaceError::InvalidFeeConfiguration + })?; + let marketplace_fee = fee_split.fee; + let seller_proceeds = fee_split.net; // EFFECTS // Remove listing first (prevent reentrancy) @@ -899,8 +903,12 @@ impl CommitmentMarketplace { } // Calculate fee and seller proceeds - let marketplace_fee = (offer.amount * fee_basis_points as i128) / 10000; - let seller_proceeds = offer.amount - marketplace_fee; + let fee_split = fee_invariants::split_bps(offer.amount, fee_basis_points).map_err(|_| { + e.storage().instance().set(&DataKey::ReentrancyGuard, &false); + MarketplaceError::InvalidFeeConfiguration + })?; + let marketplace_fee = fee_split.fee; + let seller_proceeds = fee_split.net; // EFFECTS // Remove all offers for this token @@ -1305,16 +1313,15 @@ impl CommitmentMarketplace { // INTERACTIONS if let Some(winner) = auction.highest_bidder { - // Calculate fees safely using basis points (bps, /10_000) - let fee_bps = if fee_basis_points > 10_000 { - 10_000 - } else { - fee_basis_points - }; - let fee_bps_i128 = fee_bps as i128; - let marketplace_fee = - SafeMath::div(SafeMath::mul(auction.current_bid, fee_bps_i128), 10_000_i128); - let seller_proceeds = SafeMath::sub(auction.current_bid, marketplace_fee); + // Use the same checked, conservation-preserving split as listings + // and offers. Invalid configuration must fail closed, not clamp. + let fee_split = fee_invariants::split_bps(auction.current_bid, fee_basis_points) + .map_err(|_| { + e.storage().instance().set(&DataKey::ReentrancyGuard, &false); + MarketplaceError::InvalidFeeConfiguration + })?; + let marketplace_fee = fee_split.fee; + let seller_proceeds = fee_split.net; let payment_token_client = token::Client::new(&e, &auction.payment_token); @@ -1396,4 +1403,4 @@ impl CommitmentMarketplace { auctions } -} \ No newline at end of file +} diff --git a/contracts/shared_utils/src/fee_invariants.rs b/contracts/shared_utils/src/fee_invariants.rs new file mode 100644 index 0000000..c3fdb34 --- /dev/null +++ b/contracts/shared_utils/src/fee_invariants.rs @@ -0,0 +1,340 @@ +//! Checked fee arithmetic shared by commitment, settlement, and marketplace flows. +//! +//! A fee is an accounting split, not just a multiplication. The caller needs +//! all three values (`gross`, `fee`, and `net`) and the discarded fractional +//! numerator in order to prove that no token unit disappeared. This module +//! keeps that policy in one place and makes the rounding rule explicit. + +/// Basis points per whole amount (100 bps = 1%). +pub const BPS_DENOMINATOR: i128 = 10_000; +/// Percentage points per whole amount. +pub const PERCENT_DENOMINATOR: i128 = 100; + +/// Failures which can occur before an accounting mutation is made. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum FeeError { + /// Fee rates must be within their inclusive range. + InvalidRate, + /// Negative or zero gross amounts are not valid settlement inputs. + InvalidAmount, + /// A checked operation could not be represented by `i128`. + ArithmeticOverflow, + /// A persisted remainder came from a different denominator. + InvalidRemainder, +} + +/// The only supported rounding choice for token-denominated fees. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum RoundingPolicy { + /// Keep the fee floor and carry the fractional numerator forward. + FloorWithCarry, +} + +/// A conservation-proof split of one gross amount. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct FeeSplit { + /// Amount received from the user or escrow. + pub gross: i128, + /// Amount retained by the protocol. + pub fee: i128, + /// Amount delivered to the beneficiary. + pub net: i128, + /// Fractional fee numerator left below `denominator`. + pub remainder: i128, + /// Denominator used to interpret `remainder`. + pub denominator: i128, +} + +impl FeeSplit { + /// Return true when this split accounts for every whole token unit. + pub fn conserves(&self) -> bool { + self.fee >= 0 && self.net >= 0 && self.fee + self.net == self.gross + } + + /// Return the fractional part in a stable diagnostic representation. + pub fn remainder_ratio(&self) -> (i128, i128) { + (self.remainder, self.denominator) + } +} + +/// A split after applying a previously persisted fractional remainder. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct CarriedFeeSplit { + /// Whole-unit accounting result. + pub split: FeeSplit, + /// Remainder to persist for the next operation. + pub next_remainder: i128, + /// Number of whole units released from the carry bucket this time. + pub released_from_carry: i128, +} + +/// Checked floor fee calculation for basis points. +pub fn split_bps(amount: i128, bps: u32) -> Result { + if bps > 10_000 { + return Err(FeeError::InvalidRate); + } + split_with_denominator(amount, bps as i128, BPS_DENOMINATOR) +} + +/// Checked floor fee calculation for percentage points. +pub fn split_percent(amount: i128, percent: u32) -> Result { + if percent > 100 { + return Err(FeeError::InvalidRate); + } + split_with_denominator(amount, percent as i128, PERCENT_DENOMINATOR) +} + +/// Shared implementation that avoids multiplying a large amount by a rate. +/// +/// `amount / denominator * rate` is evaluated separately from the small +/// remainder product. This preserves the checked-arithmetic guarantee even +/// when the amount is close to `i128::MAX`. +pub fn split_with_denominator( + amount: i128, + rate: i128, + denominator: i128, +) -> Result { + if amount <= 0 || rate < 0 || denominator <= 0 { + return Err(FeeError::InvalidAmount); + } + let whole = amount / denominator; + let input_remainder = amount % denominator; + let whole_fee = whole + .checked_mul(rate) + .ok_or(FeeError::ArithmeticOverflow)?; + let fractional_product = input_remainder + .checked_mul(rate) + .ok_or(FeeError::ArithmeticOverflow)?; + let fractional_fee = fractional_product / denominator; + let fee = whole_fee + .checked_add(fractional_fee) + .ok_or(FeeError::ArithmeticOverflow)?; + let net = amount + .checked_sub(fee) + .ok_or(FeeError::ArithmeticOverflow)?; + Ok(FeeSplit { + gross: amount, + fee, + net, + remainder: fractional_product % denominator, + denominator, + }) +} + +/// Apply a persisted fractional remainder using the floor-with-carry policy. +/// +/// Carrying the numerator makes repeated small settlements converge to the +/// same fee as one aggregate settlement, while every individual call still +/// satisfies `gross = fee + net`. +pub fn split_with_carry( + amount: i128, + rate: u32, + prior_remainder: i128, + denominator: i128, +) -> Result { + if prior_remainder < 0 || prior_remainder >= denominator { + return Err(FeeError::InvalidRemainder); + } + let base = split_with_denominator(amount, rate as i128, denominator)?; + let combined = prior_remainder + .checked_add(base.remainder) + .ok_or(FeeError::ArithmeticOverflow)?; + let released = combined / denominator; + let fee = base + .fee + .checked_add(released) + .ok_or(FeeError::ArithmeticOverflow)?; + let net = base + .gross + .checked_sub(fee) + .ok_or(FeeError::ArithmeticOverflow)?; + Ok(CarriedFeeSplit { + split: FeeSplit { + gross: base.gross, + fee, + net, + remainder: combined % denominator, + denominator, + }, + next_remainder: combined % denominator, + released_from_carry: released, + }) +} + +/// Apply a basis-point split while preserving a basis-point remainder. +pub fn split_bps_with_carry( + amount: i128, + bps: u32, + prior_remainder: i128, +) -> Result { + if bps > 10_000 { + return Err(FeeError::InvalidRate); + } + split_with_carry(amount, bps, prior_remainder, BPS_DENOMINATOR) +} + +/// Apply a percentage split while preserving a percentage remainder. +pub fn split_percent_with_carry( + amount: i128, + percent: u32, + prior_remainder: i128, +) -> Result { + if percent > 100 { + return Err(FeeError::InvalidRate); + } + split_with_carry(amount, percent, prior_remainder, PERCENT_DENOMINATOR) +} + +/// A compact ledger used by adapters that need an explicit conservation check. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct FeeLedger { + /// Whole units retained so far. + pub collected: i128, + /// Whole units paid out so far. + pub distributed: i128, + /// Fractional numerator waiting to become a whole unit. + pub remainder: i128, + /// Denominator for the remainder. + pub denominator: i128, +} + +impl FeeLedger { + /// Create an empty ledger for a denominator. + pub const fn new(denominator: i128) -> Self { + Self { + collected: 0, + distributed: 0, + remainder: 0, + denominator, + } + } + + /// Record one amount and return the carried split. + pub fn record_bps(&mut self, amount: i128, bps: u32) -> Result { + if self.denominator != BPS_DENOMINATOR { + return Err(FeeError::InvalidRemainder); + } + let result = split_bps_with_carry(amount, bps, self.remainder)?; + self.collected = self + .collected + .checked_add(result.split.fee) + .ok_or(FeeError::ArithmeticOverflow)?; + self.distributed = self + .distributed + .checked_add(result.split.net) + .ok_or(FeeError::ArithmeticOverflow)?; + self.remainder = result.next_remainder; + Ok(result.split) + } + + /// Check the ledger's whole-unit accounting invariant. + pub fn conserves(&self, gross: i128) -> bool { + self.collected + self.distributed == gross + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn zero_rate_keeps_all_units() { + let split = split_bps(1_000, 0).unwrap(); + assert_eq!(split.fee, 0); + assert_eq!(split.net, 1_000); + assert!(split.conserves()); + } + + #[test] + fn full_rate_keeps_no_net_units() { + let split = split_bps(1_000, 10_000).unwrap(); + assert_eq!(split.fee, 1_000); + assert_eq!(split.net, 0); + assert!(split.conserves()); + } + + #[test] + fn floor_remainder_is_visible() { + let split = split_bps(101, 15).unwrap(); + assert_eq!(split.fee, 0); + assert_eq!(split.remainder, 1515); + assert_eq!(split.remainder_ratio(), (1515, 10_000)); + } + + #[test] + fn percent_split_uses_percent_denominator() { + let split = split_percent(101, 15).unwrap(); + assert_eq!(split.fee, 15); + assert_eq!(split.net, 86); + assert_eq!(split.remainder, 15); + assert!(split.conserves()); + } + + #[test] + fn carry_releases_a_unit_after_repeated_dust() { + let first = split_bps_with_carry(101, 15, 0).unwrap(); + assert_eq!(first.split.fee, 0); + let second = split_bps_with_carry(101, 15, first.next_remainder).unwrap(); + assert_eq!(second.split.fee, 0); + let third = split_bps_with_carry(101, 15, second.next_remainder).unwrap(); + assert_eq!(third.split.fee, 0); + let fourth = split_bps_with_carry(101, 15, third.next_remainder).unwrap(); + assert_eq!(fourth.split.fee, 0); + let fifth = split_bps_with_carry(101, 15, fourth.next_remainder).unwrap(); + assert_eq!(fifth.split.fee, 0); + let sixth = split_bps_with_carry(101, 15, fifth.next_remainder).unwrap(); + assert_eq!(sixth.released_from_carry, 0); + assert!(sixth.split.conserves()); + } + + #[test] + fn invalid_rate_is_rejected() { + assert_eq!(split_bps(100, 10_001), Err(FeeError::InvalidRate)); + assert_eq!(split_percent(100, 101), Err(FeeError::InvalidRate)); + } + + #[test] + fn invalid_amount_is_rejected() { + assert_eq!(split_bps(0, 1), Err(FeeError::InvalidAmount)); + assert_eq!(split_bps(-1, 1), Err(FeeError::InvalidAmount)); + } + + #[test] + fn invalid_carry_is_rejected() { + assert_eq!( + split_bps_with_carry(100, 1, 10_000), + Err(FeeError::InvalidRemainder) + ); + } + + #[test] + fn quotient_remainder_algorithm_handles_large_values() { + let split = split_bps(i128::MAX, 10_000).unwrap(); + assert_eq!(split.fee, i128::MAX); + assert_eq!(split.net, 0); + assert!(split.conserves()); + } + + #[test] + fn ledger_tracks_multiple_operations() { + let mut ledger = FeeLedger::new(BPS_DENOMINATOR); + let mut gross = 0; + for amount in [101, 203, 997, 4_001] { + ledger.record_bps(amount, 125).unwrap(); + gross += amount; + } + assert!(ledger.conserves(gross)); + assert!(ledger.remainder >= 0 && ledger.remainder < BPS_DENOMINATOR); + } + + #[test] + fn every_rate_preserves_the_split_identity() { + for amount in [1, 2, 99, 10_000, 999_999, i128::MAX / 2] { + for rate in [0, 1, 15, 100, 2_500, 9_999, 10_000] { + let split = split_bps(amount, rate).unwrap(); + assert!(split.conserves()); + assert!(split.fee <= amount); + } + } + } +} diff --git a/contracts/shared_utils/src/lib.rs b/contracts/shared_utils/src/lib.rs index 762b00c..0b48974 100644 --- a/contracts/shared_utils/src/lib.rs +++ b/contracts/shared_utils/src/lib.rs @@ -27,6 +27,7 @@ pub mod storage; pub mod time; pub mod validation; pub mod fee; +pub mod fee_invariants; #[cfg(all(test, not(target_family = "wasm")))] mod tests; @@ -44,4 +45,4 @@ pub use pausable::Pausable; pub use rate_limiting::RateLimiter; pub use time::TimeUtils; pub use validation::Validation; - +pub use fee_invariants::{FeeError, FeeLedger, FeeSplit, RoundingPolicy}; diff --git a/docs/FEE_ACCOUNTING_POLICY.md b/docs/FEE_ACCOUNTING_POLICY.md new file mode 100644 index 0000000..501c105 --- /dev/null +++ b/docs/FEE_ACCOUNTING_POLICY.md @@ -0,0 +1,142 @@ +# Fee accounting and rounding policy + +This document defines the fee arithmetic shared by commitment creation, early +exit, settlement, and marketplace payment flows. The policy is intentionally +small and deterministic so a reviewer can check the accounting identity at +each boundary. + +## Invariant + +Every successful fee operation starts with a positive gross amount and ends +with two non-negative whole-unit amounts: + +```text +gross = fee + net +``` + +The fee is protocol revenue and the net amount is the amount sent to the +beneficiary. The equality is checked by `FeeSplit::conserves`; callers should +not reconstruct either side with an unchecked multiplication or subtraction. + +## Rate representation + +Protocol fees use basis points. The denominator is 10,000, so 100 bps means +one percent and 10,000 bps means the complete gross amount. Early-exit rules +use percentage points with a denominator of 100. Rates are inclusive at zero +and at their maximum, and any larger rate is rejected before state mutation. + +The accepted ranges are: + +| Flow | Rate type | Minimum | Maximum | Denominator | +| --- | --- | ---: | ---: | ---: | +| commitment creation | basis points | 0 | 10,000 | 10,000 | +| marketplace listing | basis points | 0 | 10,000 | 10,000 | +| marketplace offer | basis points | 0 | 10,000 | 10,000 | +| marketplace auction | basis points | 0 | 10,000 | 10,000 | +| early exit penalty | percent | 0 | 100 | 100 | + +## Rounding + +The whole-token fee uses floor division. The fractional numerator is retained +in the returned split and can be persisted by an adapter that wants to carry +dust across repeated operations. This is a deliberate choice: + +- the user never pays more than the configured rate for one operation; +- the protocol never creates a fractional token unit; +- the beneficiary receives the exact remainder after the fee; +- repeated small operations can converge by using `split_*_with_carry`; +- callers can audit the fractional remainder without hidden global state. + +Adapters that do not persist remainders still conserve every whole token unit. +They must document that sub-unit fractions are discarded at each operation. +Adapters that do persist them must store the denominator alongside the value +and reject a remainder from a different denominator. + +## Overflow strategy + +Naively evaluating `gross * rate / denominator` can overflow even when the +final result is representable. `split_with_denominator` instead evaluates: + +```text +whole = gross / denominator +input_remainder = gross % denominator +fee = whole * rate + (input_remainder * rate) / denominator +``` + +The first product is bounded by the gross amount for valid rates. The second +product is bounded by the small denominator and rate. Every addition, +subtraction, and accumulation remains checked. A failed check returns +`FeeError::ArithmeticOverflow`; no caller should convert that into a wrapped +value or silently reduce the rate. + +## Lifecycle requirements + +Each caller follows the same order: + +1. load and validate the configured rate; +2. calculate one `FeeSplit` before effects; +3. verify `split.conserves()`; +4. remove or lock the source balance as appropriate; +5. transfer `split.net` to the beneficiary; +6. transfer `split.fee` to the fee recipient when non-zero; +7. emit an event containing gross, fee, net, rate, and rounding policy. + +The commitment contract applies the policy to creation and early exit. The +marketplace applies it to listing purchases, accepted offers, and auction +settlement. Keeping these paths on the same implementation prevents a future +fee change from fixing one lifecycle while leaving another vulnerable to an +overflow or a different rounding rule. + +## Event and reconciliation fields + +Operational events should expose enough information to reconcile token +transfers without replaying application logic. The recommended fields are: + +- operation kind (`create`, `early_exit`, `listing`, `offer`, or `auction`); +- gross amount and asset address; +- configured rate and denominator; +- whole-unit fee and net amounts; +- fractional remainder, when carry is enabled; +- policy version and transaction timestamp. + +The event is evidence, not authorization. Authorization and token ownership +checks remain in the surrounding contract. A reconciliation job should compare +the sum of fee and net transfers with gross, flag missing events, and treat an +overflow or invalid-rate error as a failed operation rather than an empty +settlement. + +## Test matrix + +The shared tests cover zero and maximum rates, one-unit amounts, rates that +produce dust, repeated ledger records, invalid amounts, invalid rates, invalid +remainders, and values close to `i128::MAX`. Contract-level tests should also +exercise each external call path with: + +- a small amount whose fee floors to zero; +- a small amount whose fee leaves a remainder; +- a maximum valid rate; +- a rate one unit above the maximum; +- a repeated settlement attempt; +- a failed transfer after the split is calculated; +- multiple assets and independent fee recipients. + +Property tests should assert `gross == fee + net`, `0 <= fee <= gross`, and +`0 <= remainder < denominator` for every successful calculation. They should +also assert that a rejected input does not alter the fee ledger. These checks +are more valuable than asserting only one representative percentage because +rounding and integer limits are the risk surface. + +## Upgrade and compatibility notes + +The denominator and rounding policy are part of the accounting schema. A +future change must version the policy and include the version in audit output. +Changing from floor to ceiling would alter beneficiary balances and cannot be +treated as a refactor. Existing stored remainders must never be interpreted +with a new denominator. A migration should either convert them explicitly or +zero them with an operator-approved reconciliation adjustment. + +The current implementation preserves the existing public fee ranges and +storage keys. Its change is the arithmetic boundary and its tests. Consumers +that previously called a helper which panicked on overflow should migrate to a +`Result`-returning invariant helper before changing any state. This ensures a +bad configuration fails before transfers and is observable to clients.