From c4210d41c9a0b46f1574ec00ac6b59b00e895a07 Mon Sep 17 00:00:00 2001 From: Hexor Hash Date: Wed, 19 Aug 2026 10:22:46 +0000 Subject: [PATCH] fix(fee-primitives): audit i128 fee arithmetic for overflow and rounding Full audit of every arithmetic expression in fee-primitives/src/lib.rs per #193. - Add overflow-safe checked_mul at both multiplication sites (calculate_fee, convert_currency). On overflow, calculate_fee clamps to config.max_fee (the true fee is necessarily far above it); convert_currency panics with a descriptive message since it has no safe ceiling to fall back to. - Round fee calculation up (ceiling division) so the protocol never under-collects due to truncation; document why convert_currency intentionally keeps floor division (it's a value transform, not a charge). - Validate amount/min_fee/max_fee/base_fee_rate are non-negative, min_fee <= max_fee, and base_fee_rate <= 10_000 bps (100%). - Guarantee the computed fee never exceeds the principal amount, even if min_fee is configured above it. - Add 16 tests: edge cases (zero amount, i128::MAX, invalid config) and 5 property-based tests covering the acceptance-criteria invariants (zero fee at zero amount, no overflow at i128::MAX, fee+principal well-formed, rounding always >= 0 and favors the protocol, fee never exceeds principal). - Fix pre-existing compile errors that meant this crate had never actually built or run its tests: missing `contractimpl` import, private storage.get/set calls (now via storage().persistent()), and contract-exported functions taking references (Soroban contract functions must take owned Val types). - Add fee-primitives to the Cargo workspace members so `cargo test -p fee-primitives` (the acceptance criterion's exact command) works. - Document the rounding convention and invariants in docs/fee-primitives.md. Closes #193 Co-Authored-By: Claude Sonnet 5 --- contracts/Cargo.lock | 360 +++++++++++++++++-- contracts/Cargo.toml | 1 + contracts/fee-primitives/Cargo.toml | 2 +- contracts/fee-primitives/src/lib.rs | 521 ++++++++++++++++++++++++---- docs/fee-primitives.md | 47 +++ 5 files changed, 831 insertions(+), 100 deletions(-) diff --git a/contracts/Cargo.lock b/contracts/Cargo.lock index 1ef959a..851e260 100644 --- a/contracts/Cargo.lock +++ b/contracts/Cargo.lock @@ -2,6 +2,21 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "addr2line" +version = "0.25.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b5d307320b3181d6d7954e663bd7c774a838b8220fe0593c86d9fb09f498b4b" +dependencies = [ + "gimli", +] + +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + [[package]] name = "ahash" version = "0.8.12" @@ -75,7 +90,7 @@ dependencies = [ "educe", "fnv", "hashbrown 0.15.5", - "itertools", + "itertools 0.13.0", "num-bigint", "num-integer", "num-traits", @@ -95,7 +110,7 @@ dependencies = [ "arrayvec", "digest", "educe", - "itertools", + "itertools 0.13.0", "num-bigint", "num-traits", "paste", @@ -186,12 +201,39 @@ version = "1.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" +[[package]] +name = "backtrace" +version = "0.3.76" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb531853791a215d7c62a30daf0dde835f381ab5de4589cfe7c649d2cbe92bd6" +dependencies = [ + "addr2line", + "cfg-if", + "libc", + "miniz_oxide", + "object", + "rustc-demangle", + "windows-link", +] + [[package]] name = "base16ct" version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4c7f02d4ea65f2c1853089ffd8d2787bdbc63de2f0d29dedbcf8ccdfa0ccd4cf" +[[package]] +name = "base32" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23ce669cd6c8588f79e15cf450314f9638f967fc5770ff1c7c1deb0925ea7cfa" + +[[package]] +name = "base64" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" + [[package]] name = "base64" version = "0.22.1" @@ -339,6 +381,16 @@ dependencies = [ "typenum", ] +[[package]] +name = "ctor" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a2785755761f3ddc1492979ce1e48d2c00d09311c39e4466429188f3dd6501" +dependencies = [ + "quote", + "syn", +] + [[package]] name = "ctor" version = "0.5.0" @@ -636,7 +688,7 @@ checksum = "2bfcf67fea2815c2fc3b90873fae90957be12ff417335dfadc7f52927feb03b2" name = "escrow" version = "0.0.0" dependencies = [ - "soroban-sdk", + "soroban-sdk 26.1.0", ] [[package]] @@ -645,6 +697,13 @@ version = "1.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40404c3f5f511ec4da6fe866ddf6a717c309fdbb69fbbad7b0f3edab8f2e835f" +[[package]] +name = "fee-primitives" +version = "0.1.0" +dependencies = [ + "soroban-sdk 21.7.7", +] + [[package]] name = "ff" version = "0.13.1" @@ -721,11 +780,17 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "gimli" +version = "0.32.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e629b9b98ef3dd8afe6ca2bd0f89306cec16d43d907889945bc5d6687f2f13c7" + [[package]] name = "governor" version = "0.0.0" dependencies = [ - "soroban-sdk", + "soroban-sdk 26.1.0", ] [[package]] @@ -868,6 +933,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8e04e2fd2b8188ea827b32ef11de88377086d690286ab35747ef7f9bf3ccb590" +[[package]] +name = "itertools" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57" +dependencies = [ + "either", +] + [[package]] name = "itertools" version = "0.13.0" @@ -950,6 +1024,15 @@ version = "2.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "88904434abc2901f197fe8cc55f0445e7ded921dba5911dad2e2b39b48e663c4" +[[package]] +name = "miniz_oxide" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fa76a2c86f704bdb222d66965fb3d63269ce38518b83cb0575fca855ebb6316" +dependencies = [ + "adler2", +] + [[package]] name = "num-bigint" version = "0.4.6" @@ -995,6 +1078,15 @@ dependencies = [ "autocfg", ] +[[package]] +name = "object" +version = "0.37.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ff76201f031d8863c38aa7f905eca4f53abbfa15f609db4277d44cd8938f33fe" +dependencies = [ + "memchr", +] + [[package]] name = "once_cell" version = "1.21.4" @@ -1023,7 +1115,7 @@ checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" name = "payment_registry" version = "0.0.0" dependencies = [ - "soroban-sdk", + "soroban-sdk 26.1.0", ] [[package]] @@ -1154,6 +1246,12 @@ dependencies = [ "subtle", ] +[[package]] +name = "rustc-demangle" +version = "0.1.28" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b74b56ffa8bb2830709a538c2cbcae9aa062db0d2a42563bfb09bdaae44020eb" + [[package]] name = "rustc_version" version = "0.4.1" @@ -1272,7 +1370,7 @@ version = "3.21.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "76a5c54c7310e7b8b9577c286d7e399ddd876c3e12b3ed917a8aabc4b96e9e8c" dependencies = [ - "base64", + "base64 0.22.1", "bs58", "chrono", "hex", @@ -1348,18 +1446,49 @@ version = "1.15.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ed6a63f02c8539c91a8685a86f4099661ba3da017932f6ebbea6de3f0fa7c90" +[[package]] +name = "soroban-builtin-sdk-macros" +version = "21.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f57a68ef8777e28e274de0f3a88ad9a5a41d9a2eb461b4dd800b086f0e83b80" +dependencies = [ + "itertools 0.11.0", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "soroban-builtin-sdk-macros" version = "26.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "35a3a2b57b132b800e132d2c81e1818359bb2cf787ca39c61c151d6bd0798403" dependencies = [ - "itertools", + "itertools 0.13.0", "proc-macro2", "quote", "syn", ] +[[package]] +name = "soroban-env-common" +version = "21.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2fd1c89463835fe6da996318156d39f424b4f167c725ec692e5a7a2d4e694b3d" +dependencies = [ + "arbitrary", + "crate-git-revision", + "ethnum", + "num-derive", + "num-traits", + "serde", + "soroban-env-macros 21.2.1", + "soroban-wasmi", + "static_assertions", + "stellar-xdr 21.2.0", + "wasmparser", +] + [[package]] name = "soroban-env-common" version = "26.1.3" @@ -1372,23 +1501,66 @@ dependencies = [ "num-derive", "num-traits", "serde", - "soroban-env-macros", + "soroban-env-macros 26.1.3", "soroban-wasmi", "static_assertions", - "stellar-xdr", + "stellar-xdr 26.0.1", "wasmparser", ] +[[package]] +name = "soroban-env-guest" +version = "21.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6bfb2536811045d5cd0c656a324cbe9ce4467eb734c7946b74410d90dea5d0ce" +dependencies = [ + "soroban-env-common 21.2.1", + "static_assertions", +] + [[package]] name = "soroban-env-guest" version = "26.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "15aeed6d7a4dc4d3bba65e2ac92f7eeaa664900bbc82e1055e024bf637d74ed3" dependencies = [ - "soroban-env-common", + "soroban-env-common 26.1.3", "static_assertions", ] +[[package]] +name = "soroban-env-host" +version = "21.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b7a32c28f281c423189f1298960194f0e0fc4eeb72378028171e556d8cd6160" +dependencies = [ + "backtrace", + "curve25519-dalek", + "ecdsa", + "ed25519-dalek", + "elliptic-curve", + "generic-array", + "getrandom", + "hex-literal", + "hmac", + "k256", + "num-derive", + "num-integer", + "num-traits", + "p256", + "rand", + "rand_chacha", + "sec1", + "sha2", + "sha3", + "soroban-builtin-sdk-macros 21.2.1", + "soroban-env-common 21.2.1", + "soroban-wasmi", + "static_assertions", + "stellar-strkey 0.0.8", + "wasmparser", +] + [[package]] name = "soroban-env-host" version = "26.1.3" @@ -1418,29 +1590,58 @@ dependencies = [ "sec1", "sha2", "sha3", - "soroban-builtin-sdk-macros", - "soroban-env-common", + "soroban-builtin-sdk-macros 26.1.3", + "soroban-env-common 26.1.3", "soroban-wasmi", "static_assertions", "stellar-strkey 0.0.13", "wasmparser", ] +[[package]] +name = "soroban-env-macros" +version = "21.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "242926fe5e0d922f12d3796cd7cd02dd824e5ef1caa088f45fce20b618309f64" +dependencies = [ + "itertools 0.11.0", + "proc-macro2", + "quote", + "serde", + "serde_json", + "stellar-xdr 21.2.0", + "syn", +] + [[package]] name = "soroban-env-macros" version = "26.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2ada3449bb23c964a88a1bf633ac66ca3ebac2061693f53148b199ce816791d0" dependencies = [ - "itertools", + "itertools 0.13.0", "proc-macro2", "quote", "serde", "serde_json", - "stellar-xdr", + "stellar-xdr 26.0.1", "syn", ] +[[package]] +name = "soroban-ledger-snapshot" +version = "21.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6edf92749fd8399b417192d301c11f710b9cdce15789a3d157785ea971576fa" +dependencies = [ + "serde", + "serde_json", + "serde_with", + "soroban-env-common 21.2.1", + "soroban-env-host 21.2.1", + "thiserror", +] + [[package]] name = "soroban-ledger-snapshot" version = "26.1.0" @@ -1450,11 +1651,33 @@ dependencies = [ "serde", "serde_json", "serde_with", - "soroban-env-common", - "soroban-env-host", + "soroban-env-common 26.1.3", + "soroban-env-host 26.1.3", "thiserror", ] +[[package]] +name = "soroban-sdk" +version = "21.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7dcdf04484af7cc731a7a48ad1d9f5f940370edeea84734434ceaf398a6b862e" +dependencies = [ + "arbitrary", + "bytes-lit", + "ctor 0.2.9", + "derive_arbitrary", + "ed25519-dalek", + "rand", + "rustc_version", + "serde", + "serde_json", + "soroban-env-guest 21.2.1", + "soroban-env-host 21.2.1", + "soroban-ledger-snapshot 21.7.7", + "soroban-sdk-macros 21.7.7", + "stellar-strkey 0.0.8", +] + [[package]] name = "soroban-sdk" version = "26.1.0" @@ -1464,21 +1687,41 @@ dependencies = [ "arbitrary", "bytes-lit", "crate-git-revision", - "ctor", + "ctor 0.5.0", "derive_arbitrary", "ed25519-dalek", "rand", "rustc_version", "serde", "serde_json", - "soroban-env-guest", - "soroban-env-host", - "soroban-ledger-snapshot", - "soroban-sdk-macros", + "soroban-env-guest 26.1.3", + "soroban-env-host 26.1.3", + "soroban-ledger-snapshot 26.1.0", + "soroban-sdk-macros 26.1.0", "stellar-strkey 0.0.16", "visibility", ] +[[package]] +name = "soroban-sdk-macros" +version = "21.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0974e413731aeff2443f2305b344578b3f1ffd18335a7ba0f0b5d2eb4e94c9ce" +dependencies = [ + "crate-git-revision", + "darling 0.20.11", + "itertools 0.11.0", + "proc-macro2", + "quote", + "rustc_version", + "sha2", + "soroban-env-common 21.2.1", + "soroban-spec 21.7.7", + "soroban-spec-rust 21.7.7", + "stellar-xdr 21.2.0", + "syn", +] + [[package]] name = "soroban-sdk-macros" version = "26.1.0" @@ -1487,31 +1730,59 @@ checksum = "abbdc02e0d789df78c25b0d056eb01cc906feab690c74895febc96890a0e6aa0" dependencies = [ "darling 0.20.11", "heck", - "itertools", + "itertools 0.13.0", "macro-string", "proc-macro2", "quote", "sha2", - "soroban-env-common", - "soroban-spec", - "soroban-spec-rust", - "stellar-xdr", + "soroban-env-common 26.1.3", + "soroban-spec 26.1.0", + "soroban-spec-rust 26.1.0", + "stellar-xdr 26.0.1", "syn", ] +[[package]] +name = "soroban-spec" +version = "21.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2c70b20e68cae3ef700b8fa3ae29db1c6a294b311fba66918f90cb8f9fd0a1a" +dependencies = [ + "base64 0.13.1", + "stellar-xdr 21.2.0", + "thiserror", + "wasmparser", +] + [[package]] name = "soroban-spec" version = "26.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e3afb753b6ec3329af9744091ebe9f4c047c8fd1078d333b3f83a598a22bef33" dependencies = [ - "base64", + "base64 0.22.1", "sha2", - "stellar-xdr", + "stellar-xdr 26.0.1", "thiserror", "wasmparser", ] +[[package]] +name = "soroban-spec-rust" +version = "21.7.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2dafbde981b141b191c6c036abc86097070ddd6eaaa33b273701449501e43d3" +dependencies = [ + "prettyplease", + "proc-macro2", + "quote", + "sha2", + "soroban-spec 21.7.7", + "stellar-xdr 21.2.0", + "syn", + "thiserror", +] + [[package]] name = "soroban-spec-rust" version = "26.1.0" @@ -1522,8 +1793,8 @@ dependencies = [ "proc-macro2", "quote", "sha2", - "soroban-spec", - "stellar-xdr", + "soroban-spec 26.1.0", + "stellar-xdr 26.0.1", "syn", "thiserror", ] @@ -1569,6 +1840,17 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" +[[package]] +name = "stellar-strkey" +version = "0.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "12d2bf45e114117ea91d820a846fd1afbe3ba7d717988fee094ce8227a3bf8bd" +dependencies = [ + "base32", + "crate-git-revision", + "thiserror", +] + [[package]] name = "stellar-strkey" version = "0.0.13" @@ -1590,6 +1872,22 @@ dependencies = [ "heapless", ] +[[package]] +name = "stellar-xdr" +version = "21.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2675a71212ed39a806e415b0dbf4702879ff288ec7f5ee996dda42a135512b50" +dependencies = [ + "arbitrary", + "base64 0.13.1", + "crate-git-revision", + "escape-bytes", + "hex", + "serde", + "serde_with", + "stellar-strkey 0.0.8", +] + [[package]] name = "stellar-xdr" version = "26.0.1" @@ -1597,7 +1895,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea6e29c7e1f071c2767916460d006668197843d5d93f0ec8893a26f72a14f595" dependencies = [ "arbitrary", - "base64", + "base64 0.22.1", "cfg_eval", "crate-git-revision", "escape-bytes", diff --git a/contracts/Cargo.toml b/contracts/Cargo.toml index d6766c9..0fae30a 100644 --- a/contracts/Cargo.toml +++ b/contracts/Cargo.toml @@ -2,6 +2,7 @@ resolver = "2" members = [ "contracts/*", + "fee-primitives", ] [workspace.dependencies] diff --git a/contracts/fee-primitives/Cargo.toml b/contracts/fee-primitives/Cargo.toml index 74962f6..81dae27 100644 --- a/contracts/fee-primitives/Cargo.toml +++ b/contracts/fee-primitives/Cargo.toml @@ -6,7 +6,7 @@ edition = "2021" [dependencies] soroban-sdk = "21.0.0" -[dev_dependencies] +[dev-dependencies] soroban-sdk = { version = "21.0.0", features = ["testutils"] } [lib] diff --git a/contracts/fee-primitives/src/lib.rs b/contracts/fee-primitives/src/lib.rs index 5c93c52..d9db475 100644 --- a/contracts/fee-primitives/src/lib.rs +++ b/contracts/fee-primitives/src/lib.rs @@ -1,80 +1,178 @@ #![no_std] -use soroban_sdk::{contract, contracttype, Address, Env, String}; +use soroban_sdk::{contract, contractimpl, contracttype, Address, Env, String}; + +/// Denominator for basis-point (1/100th of a percent) calculations. +/// `10_000` basis points == 100%. +const BASIS_POINTS_DENOMINATOR: i128 = 10_000; /// Configuration for fee calculation #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct FeeConfig { - pub base_fee_rate: i128, // Basis points (e.g., 100 = 1%) - pub min_fee: i128, // Minimum fee in smallest unit - pub max_fee: i128, // Maximum fee in smallest unit - pub fee_recipient: Address, // Address receiving fees + pub base_fee_rate: i128, // Basis points (e.g., 100 = 1%) + pub min_fee: i128, // Minimum fee in smallest unit + pub max_fee: i128, // Maximum fee in smallest unit + pub fee_recipient: Address, // Address receiving fees } /// Currency conversion rate #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct ConversionRate { - pub asset_a: String, // Source asset identifier - pub asset_b: String, // Target asset identifier - pub rate: i128, // Conversion rate (basis points) - pub updated_at: u64, // Timestamp + pub asset_a: String, // Source asset identifier + pub asset_b: String, // Target asset identifier + pub rate: i128, // Conversion rate (basis points) + pub updated_at: u64, // Timestamp } /// Fee calculation result #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct FeeResult { - pub amount: i128, // Fee amount in source asset - pub rate_used: i128, // Fee rate applied + pub amount: i128, // Fee amount in source asset + pub rate_used: i128, // Fee rate applied pub converted_amount: Option, // Converted amount if conversion applied } +/// Computes the fee for `amount` under `config`. +/// +/// ## Rounding convention (favors the protocol, not the payer) +/// The raw basis-point fee (`amount * base_fee_rate / BASIS_POINTS_DENOMINATOR`) +/// is rounded **up** (ceiling division) whenever there is a non-zero +/// remainder. Truncating (floor) division would silently under-charge by up +/// to one unit on every call, which over a large volume of transactions is a +/// systematic loss of protocol revenue. Rounding up guarantees the protocol +/// never collects less than the exact basis-point rate implies. +/// +/// ## Overflow bound +/// `amount.checked_mul(config.base_fee_rate)` is `None` exactly when +/// `amount * base_fee_rate` would not fit in an `i128` +/// (i.e. `amount > i128::MAX / base_fee_rate` for `base_fee_rate > 0`). +/// Because `config.max_fee` already defines the maximum fee the protocol will +/// ever collect, an overflowing product can only mean the *true* fee is far +/// larger than `max_fee` — so on overflow we clamp straight to `max_fee` +/// instead of computing (or wrapping/panicking on) the unrepresentable exact +/// product. +/// +/// ## Invariants enforced +/// - Fee is never negative (`amount`, `min_fee`, `max_fee`, `base_fee_rate` +/// are all validated to be non-negative, and `min_fee <= max_fee`). +/// - Fee never exceeds `amount`: even if `min_fee` is configured above +/// `amount`, the payer is never charged more than the principal itself. +/// - `base_fee_rate` cannot exceed `BASIS_POINTS_DENOMINATOR` (100%). +fn compute_fee(amount: i128, config: &FeeConfig) -> FeeResult { + if amount < 0 { + panic!("amount must be non-negative"); + } + if config.base_fee_rate < 0 || config.base_fee_rate > BASIS_POINTS_DENOMINATOR { + panic!("base_fee_rate must be between 0 and 10_000 basis points (0%-100%)"); + } + if config.min_fee < 0 || config.max_fee < 0 { + panic!("min_fee and max_fee must be non-negative"); + } + if config.min_fee > config.max_fee { + panic!("min_fee must not exceed max_fee"); + } + + // Overflow-safe multiplication; see doc comment above for the fallback rationale. + let raw_fee = match amount.checked_mul(config.base_fee_rate) { + Some(product) => { + // Ceiling division: round the fee up in favor of the protocol. + // Safe: `product >= 0` (amount >= 0 and base_fee_rate >= 0) and + // `BASIS_POINTS_DENOMINATOR` is a non-zero constant, so this never + // panics on division-by-zero or overflows (the quotient is <= product). + let quotient = product / BASIS_POINTS_DENOMINATOR; + let remainder = product % BASIS_POINTS_DENOMINATOR; + if remainder > 0 { + quotient + 1 + } else { + quotient + } + } + None => config.max_fee, + }; + + // Apply min/max boundaries. + let clamped_fee = if raw_fee < config.min_fee { + config.min_fee + } else if raw_fee > config.max_fee { + config.max_fee + } else { + raw_fee + }; + + // Never charge a fee larger than the amount it is levied against, even + // if `min_fee` is configured above `amount` (protects the payer and + // keeps the "fee cannot exceed principal" invariant unconditional). + let final_fee = if clamped_fee > amount { + amount + } else { + clamped_fee + }; + + FeeResult { + amount: final_fee, + rate_used: config.base_fee_rate, + converted_amount: None, + } +} + +/// Converts `amount` from one asset to another using `rate`. +/// +/// ## Rounding convention +/// Unlike [`compute_fee`], this is a value transformation rather than a +/// protocol charge, so it uses standard truncating (floor) division: the +/// payer never receives fractional-unit value manufactured by rounding up an +/// asset conversion. Any fee subsequently charged on the converted amount +/// (see [`calculate_fee_with_conversion`]) still rounds up in favor of the +/// protocol, so protocol revenue is unaffected by this choice. +/// +/// ## Overflow bound +/// `amount.checked_mul(rate.rate)` is `None` exactly when +/// `amount * rate.rate` would not fit in an `i128`. Unlike fee calculation, +/// there is no configured ceiling to safely fall back to for a currency +/// conversion, so an overflow is a genuine error and panics rather than +/// silently wrapping or truncating. +fn apply_conversion(amount: i128, rate: &ConversionRate) -> i128 { + if amount < 0 { + panic!("amount must be non-negative"); + } + if rate.rate < 0 { + panic!("conversion rate must be non-negative"); + } + + let product = amount + .checked_mul(rate.rate) + .unwrap_or_else(|| panic!("currency conversion overflow: amount too large for given rate")); + + // Floor division: see rounding convention above. + product / BASIS_POINTS_DENOMINATOR +} + #[contract] pub struct FeePrimitives; #[contractimpl] impl FeePrimitives { /// Calculate fee based on amount and configuration - pub fn calculate_fee( - env: &Env, - amount: i128, - config: &FeeConfig, - ) -> FeeResult { - // Calculate fee using basis points - let fee = amount * config.base_fee_rate / 10000; - - // Apply min/max boundaries - let final_fee = if fee < config.min_fee { - config.min_fee - } else if fee > config.max_fee { - config.max_fee - } else { - fee - }; - - FeeResult { - amount: final_fee, - rate_used: config.base_fee_rate, - converted_amount: None, - } + pub fn calculate_fee(_env: &Env, amount: i128, config: FeeConfig) -> FeeResult { + compute_fee(amount, &config) } /// Convert amount from one asset to another pub fn convert_currency( - env: &Env, + _env: &Env, amount: i128, from_asset: String, to_asset: String, - rate: &ConversionRate, + rate: ConversionRate, ) -> i128 { // Ensure the rate matches the assets if rate.asset_a != from_asset || rate.asset_b != to_asset { panic!("Asset mismatch in conversion rate"); } - - // Apply conversion rate - amount * rate.rate / 10000 + + apply_conversion(amount, &rate) } /// Calculate fee with currency conversion @@ -83,59 +181,52 @@ impl FeePrimitives { amount: i128, from_asset: String, to_asset: String, - fee_config: &FeeConfig, - conversion_rate: &ConversionRate, + fee_config: FeeConfig, + conversion_rate: ConversionRate, ) -> FeeResult { // First convert the amount - let converted_amount = Self::convert_currency( - env, - amount, - from_asset, - to_asset, - conversion_rate, - ); - + let converted_amount = + Self::convert_currency(env, amount, from_asset, to_asset, conversion_rate); + // Calculate fee on the converted amount let mut fee_result = Self::calculate_fee(env, converted_amount, fee_config); - + // Add conversion info fee_result.converted_amount = Some(converted_amount); - + fee_result } /// Store a conversion rate - pub fn set_conversion_rate( - env: &Env, - asset_a: String, - asset_b: String, - rate: i128, - ) { + pub fn set_conversion_rate(env: &Env, asset_a: String, asset_b: String, rate: i128) { let conversion_rate = ConversionRate { asset_a: asset_a.clone(), asset_b: asset_b.clone(), rate, updated_at: env.ledger().timestamp(), }; - env.storage().set(&asset_a, &conversion_rate); + env.storage().persistent().set(&asset_a, &conversion_rate); } /// Get a conversion rate - pub fn get_conversion_rate( - env: &Env, - asset_a: String, - ) -> ConversionRate { - env.storage().get(&asset_a).unwrap_or_else(|| { - panic!("Conversion rate not found for asset") - }) + pub fn get_conversion_rate(env: &Env, asset_a: String) -> ConversionRate { + env.storage() + .persistent() + .get(&asset_a) + .unwrap_or_else(|| panic!("Conversion rate not found for asset")) } } #[cfg(test)] mod test { use super::*; + use soroban_sdk::testutils::Address as _; use soroban_sdk::{Env, String}; + fn addr(env: &Env) -> Address { + Address::generate(env) + } + #[test] fn test_calculate_fee() { let env = Env::default(); @@ -143,10 +234,10 @@ mod test { base_fee_rate: 100, // 1% min_fee: 0, max_fee: 1000, - fee_recipient: Address::from_string(&String::from_str(&env, "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA")), + fee_recipient: addr(&env), }; - let result = FeePrimitives::calculate_fee(&env, 1000, &fee_config); + let result = FeePrimitives::calculate_fee(&env, 1000, fee_config); assert_eq!(result.amount, 10); // 1000 * 1% = 10 assert_eq!(result.rate_used, 100); } @@ -157,7 +248,7 @@ mod test { let rate = ConversionRate { asset_a: String::from_str(&env, "USD"), asset_b: String::from_str(&env, "EUR"), - rate: 85, // 1 USD = 0.85 EUR + rate: 8_500, // 1 USD = 0.85 EUR, expressed in basis points updated_at: 0, }; @@ -166,8 +257,302 @@ mod test { 100, String::from_str(&env, "USD"), String::from_str(&env, "EUR"), - &rate, + rate, ); assert_eq!(result, 85); } + + #[test] + fn test_fee_rounds_up_in_favor_of_protocol() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 1, // 0.01% + min_fee: 0, + max_fee: 1_000_000, + fee_recipient: addr(&env), + }; + + // 999 * 1 / 10_000 = 0.0999 -> truncation would give 0, ceiling gives 1. + let result = FeePrimitives::calculate_fee(&env, 999, fee_config); + assert_eq!(result.amount, 1); + } + + #[test] + fn test_fee_of_zero_amount_is_zero() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 500, + min_fee: 50, // even with a non-zero minimum... + max_fee: 1000, + fee_recipient: addr(&env), + }; + + // ...a zero-amount charge must never yield a positive fee. + let result = FeePrimitives::calculate_fee(&env, 0, fee_config); + assert_eq!(result.amount, 0); + } + + #[test] + fn test_fee_of_max_i128_does_not_overflow() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 100, // 1% + min_fee: 0, + max_fee: 1_000_000, + fee_recipient: addr(&env), + }; + + // amount * base_fee_rate overflows i128; the fee must safely clamp + // to max_fee instead of panicking on overflow or wrapping. + let result = FeePrimitives::calculate_fee(&env, i128::MAX, fee_config); + assert_eq!(result.amount, 1_000_000); + } + + #[test] + fn test_fee_never_exceeds_principal() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 500, + min_fee: 100, // minimum fee exceeds the amount below + max_fee: 1000, + fee_recipient: addr(&env), + }; + + let result = FeePrimitives::calculate_fee(&env, 10, fee_config); + assert!(result.amount <= 10); + assert_eq!(result.amount, 10); + } + + #[test] + fn test_fee_plus_principal_matches_expected_total() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 250, // 2.5% + min_fee: 0, + max_fee: 10_000, + fee_recipient: addr(&env), + }; + + let amount = 40_000; + let result = FeePrimitives::calculate_fee(&env, amount, fee_config); + assert_eq!(result.amount, 1000); // 40_000 * 2.5% = 1000 exactly + assert_eq!(amount + result.amount, 41_000); + } + + #[test] + #[should_panic(expected = "base_fee_rate must be between 0 and 10_000 basis points")] + fn test_base_fee_rate_over_100_percent_panics() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 10_001, // > 100% + min_fee: 0, + max_fee: 1000, + fee_recipient: addr(&env), + }; + + FeePrimitives::calculate_fee(&env, 1000, fee_config); + } + + #[test] + #[should_panic(expected = "min_fee must not exceed max_fee")] + fn test_min_fee_over_max_fee_panics() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 100, + min_fee: 2000, + max_fee: 1000, + fee_recipient: addr(&env), + }; + + FeePrimitives::calculate_fee(&env, 1000, fee_config); + } + + #[test] + #[should_panic(expected = "currency conversion overflow")] + fn test_currency_conversion_overflow_panics() { + let env = Env::default(); + let rate = ConversionRate { + asset_a: String::from_str(&env, "USD"), + asset_b: String::from_str(&env, "EUR"), + rate: i128::MAX, + updated_at: 0, + }; + + FeePrimitives::convert_currency( + &env, + i128::MAX, + String::from_str(&env, "USD"), + String::from_str(&env, "EUR"), + rate, + ); + } + + #[test] + fn test_calculate_fee_with_conversion() { + let env = Env::default(); + let fee_config = FeeConfig { + base_fee_rate: 100, // 1% + min_fee: 0, + max_fee: 1_000_000, + fee_recipient: addr(&env), + }; + let rate = ConversionRate { + asset_a: String::from_str(&env, "USD"), + asset_b: String::from_str(&env, "EUR"), + rate: 8_500, // 1 USD = 0.85 EUR + updated_at: 0, + }; + + let result = FeePrimitives::calculate_fee_with_conversion( + &env, + 1000, + String::from_str(&env, "USD"), + String::from_str(&env, "EUR"), + fee_config, + rate, + ); + assert_eq!(result.converted_amount, Some(850)); // 1000 * 0.85 + assert_eq!(result.amount, 9); // ceil(850 * 1% ) = ceil(8.5) = 9 + } + + // -- Property-based tests ------------------------------------------------- + // + // These use simple deterministic pseudo-random sampling (a linear + // congruential generator) rather than pulling in the `proptest`/ + // `quickcheck` crates, since this workspace's Soroban contracts are + // `#![no_std]` and those crates require `std`. Each test still checks the + // required invariant across a broad, varied sample of inputs, generated + // from a fixed seed for reproducibility. + fn lcg_next(state: &mut u64) -> u64 { + // Numerical Recipes LCG constants. + *state = state + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + *state + } + + fn sample_amount(state: &mut u64) -> i128 { + (lcg_next(state) % 1_000_000_000_000u64) as i128 + } + + #[test] + fn prop_fee_of_zero_amount_is_always_zero() { + let env = Env::default(); + let mut state = 42u64; + for _ in 0..200 { + let base_fee_rate = (lcg_next(&mut state) % 10_001) as i128; + let min_fee = (lcg_next(&mut state) % 1_000_000) as i128; + let max_fee = min_fee + (lcg_next(&mut state) % 1_000_000) as i128; + let config = FeeConfig { + base_fee_rate, + min_fee, + max_fee, + fee_recipient: addr(&env), + }; + let result = FeePrimitives::calculate_fee(&env, 0, config); + assert_eq!(result.amount, 0); + } + } + + #[test] + fn prop_fee_of_max_i128_never_overflows_and_is_clamped() { + let env = Env::default(); + let mut state = 7u64; + for _ in 0..200 { + let base_fee_rate = 1 + (lcg_next(&mut state) % 10_000) as i128; + let max_fee = (lcg_next(&mut state) % 1_000_000_000) as i128; + let config = FeeConfig { + base_fee_rate, + min_fee: 0, + max_fee, + fee_recipient: addr(&env), + }; + // Must not panic (would fail the test), and must clamp to max_fee. + let result = FeePrimitives::calculate_fee(&env, i128::MAX, config); + assert_eq!(result.amount, max_fee); + } + } + + #[test] + fn prop_fee_plus_principal_never_overflows_for_bounded_amounts() { + let env = Env::default(); + let mut state = 99u64; + for _ in 0..200 { + let amount = sample_amount(&mut state); + let base_fee_rate = (lcg_next(&mut state) % 10_001) as i128; + let max_fee = (lcg_next(&mut state) % 1_000_000_000) as i128; + let config = FeeConfig { + base_fee_rate, + min_fee: 0, + max_fee, + fee_recipient: addr(&env), + }; + let result = FeePrimitives::calculate_fee(&env, amount, config); + // Fee never exceeds the principal... + assert!(result.amount <= amount); + // ...so amount + fee is always representable and well-formed. + let total = amount + result.amount; + assert!(total >= amount); + } + } + + #[test] + fn prop_rounding_is_always_non_negative_and_favors_protocol() { + let env = Env::default(); + let mut state = 1234u64; + for _ in 0..200 { + let amount = sample_amount(&mut state); + let base_fee_rate = (lcg_next(&mut state) % 10_001) as i128; + let max_fee = (lcg_next(&mut state) % 1_000_000_000) as i128; + let config = FeeConfig { + base_fee_rate, + min_fee: 0, + max_fee, + fee_recipient: addr(&env), + }; + let result = FeePrimitives::calculate_fee(&env, amount, config.clone()); + + assert!(result.amount >= 0); + + // The fee must never be less than the exact (unrounded) + // basis-point share, before min/max clamping is applied — i.e. + // rounding never favors the payer over the protocol. + if let Some(product) = amount.checked_mul(base_fee_rate) { + let floor_fee = product / BASIS_POINTS_DENOMINATOR; + let unclamped_ceiling_fee = if product % BASIS_POINTS_DENOMINATOR > 0 { + floor_fee + 1 + } else { + floor_fee + }; + let expected = unclamped_ceiling_fee + .clamp(config.min_fee, config.max_fee) + .min(amount); + assert_eq!(result.amount, expected); + assert!( + result.amount >= floor_fee.min(amount).min(config.max_fee) + || result.amount == config.min_fee.min(amount) + ); + } + } + } + + #[test] + fn prop_fee_never_exceeds_principal() { + let env = Env::default(); + let mut state = 555u64; + for _ in 0..200 { + let amount = sample_amount(&mut state); + let base_fee_rate = (lcg_next(&mut state) % 10_001) as i128; + let min_fee = (lcg_next(&mut state) % 1_000_000) as i128; + let max_fee = min_fee + (lcg_next(&mut state) % 1_000_000) as i128; + let config = FeeConfig { + base_fee_rate, + min_fee, + max_fee, + fee_recipient: addr(&env), + }; + let result = FeePrimitives::calculate_fee(&env, amount, config); + assert!(result.amount <= amount); + } + } } diff --git a/docs/fee-primitives.md b/docs/fee-primitives.md index 6037b5f..39e3ace 100644 --- a/docs/fee-primitives.md +++ b/docs/fee-primitives.md @@ -26,3 +26,50 @@ Converts currency then calculates fee on converted amount. ## Storage - Conversion rates are stored by asset identifier - Fee configuration is stored separately + +## Arithmetic Safety & Rounding Convention + +All `i128` arithmetic in `fee-primitives/src/lib.rs` was audited for overflow +and rounding correctness (see #193). + +### Fee rounding — always in favor of the protocol +`calculate_fee` computes `amount * base_fee_rate / BASIS_POINTS_DENOMINATOR` +(`BASIS_POINTS_DENOMINATOR = 10_000`) using **ceiling division**: any +non-zero remainder rounds the fee **up**. Truncating division would silently +under-charge by up to one unit on every call — a systematic revenue loss for +the protocol over a large volume of transactions. Rounding up guarantees the +protocol never collects less than the exact basis-point rate implies. + +`convert_currency`, by contrast, is a value transformation rather than a +protocol charge, and uses standard truncating (floor) division — the payer +never receives fractional-unit value manufactured by rounding up an asset +conversion. Because any fee subsequently charged on a converted amount (via +`calculate_fee_with_conversion`) still rounds up, protocol revenue is +unaffected by this choice. + +### Overflow handling +Every multiplication site (`amount * base_fee_rate`, `amount * rate.rate`) +uses `checked_mul` rather than a raw `*`: + +- **`calculate_fee`**: if `amount.checked_mul(base_fee_rate)` overflows + `i128`, the *true* fee is necessarily far larger than `config.max_fee` + (which already bounds the maximum fee the protocol will ever collect), so + the result is clamped directly to `max_fee` instead of computing an + unrepresentable product. +- **`convert_currency`**: there is no configured ceiling to fall back to for + a currency conversion, so an overflow here is a genuine error and panics + with a descriptive message rather than wrapping or truncating silently. + +### Invariants enforced +- `amount`, `min_fee`, `max_fee`, and `base_fee_rate` must all be + non-negative; `min_fee` must not exceed `max_fee`; `base_fee_rate` cannot + exceed `10_000` basis points (100%). Violations panic with a descriptive + message. +- The computed fee is never negative. +- The computed fee never exceeds the `amount` it is charged against, even if + `min_fee` is configured above `amount` — this protects the payer and holds + unconditionally. + +These invariants are covered by property-based tests in +`fee-primitives/src/lib.rs` (`prop_*` tests), run with `cargo test -p +fee-primitives` from the `contracts/` workspace.