diff --git a/Cargo.lock b/Cargo.lock index f1c32b0..a73dddb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -171,7 +171,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "246a225cc6131e9ee4f24619af0f19d67761fff15d7ccc22e42b80846e69449a" dependencies = [ "num-traits", - "rand", + "rand 0.8.6", ] [[package]] @@ -204,6 +204,27 @@ version = "1.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2af50177e190e07a26ab74f8b1efbfe2ef87da2116221318cb1c2e82baf7de06" +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + +[[package]] +name = "bitflags" +version = "2.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" + [[package]] name = "block-buffer" version = "0.10.4" @@ -324,7 +345,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0dc92fb57ca44df6db8059111ab3af99a63d5d0f8375d9972e319a379c6bab76" dependencies = [ "generic-array", - "rand_core", + "rand_core 0.6.4", "subtle", "zeroize", ] @@ -557,7 +578,7 @@ checksum = "70e796c081cee67dc755e1a36a0a172b897fab85fc3f6bc48307991f64e4eca9" dependencies = [ "curve25519-dalek", "ed25519", - "rand_core", + "rand_core 0.6.4", "serde", "sha2", "subtle", @@ -594,7 +615,7 @@ dependencies = [ "ff", "generic-array", "group", - "rand_core", + "rand_core 0.6.4", "sec1", "subtle", "zeroize", @@ -626,6 +647,16 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" +[[package]] +name = "errno" +version = "0.3.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" +dependencies = [ + "libc", + "windows-sys", +] + [[package]] name = "escape-bytes" version = "0.1.1" @@ -638,13 +669,19 @@ version = "1.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40404c3f5f511ec4da6fe866ddf6a717c309fdbb69fbbad7b0f3edab8f2e835f" +[[package]] +name = "fastrand" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" + [[package]] name = "ff" version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0b50bfb653653f9ca9095b427bed08ab8d75a137839d9ad64eb11810d5b6393" dependencies = [ - "rand_core", + "rand_core 0.6.4", "subtle", ] @@ -714,6 +751,29 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "getrandom" +version = "0.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd" +dependencies = [ + "cfg-if", + "libc", + "r-efi 5.3.0", + "wasip2", +] + +[[package]] +name = "getrandom" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "300e883d756b2e4ec94e02791f39b04b522276138852cfc41d9fb7e904106099" +dependencies = [ + "cfg-if", + "libc", + "r-efi 6.0.0", +] + [[package]] name = "group" version = "0.13.0" @@ -721,7 +781,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0f9ef7462f7c099f518d754361858f86d8a07af53ba9af0fe635bbccb151a63" dependencies = [ "ff", - "rand_core", + "rand_core 0.6.4", "subtle", ] @@ -913,6 +973,12 @@ version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6d2cec3eae94f9f509c767b45932f1ada8350c4bdb85af2fcab4a3c14807981" +[[package]] +name = "linux-raw-sys" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" + [[package]] name = "log" version = "0.4.33" @@ -940,6 +1006,7 @@ checksum = "88904434abc2901f197fe8cc55f0445e7ded921dba5911dad2e2b39b48e663c4" name = "mergefi-common" version = "0.1.0" dependencies = [ + "proptest", "soroban-sdk", ] @@ -1095,6 +1162,31 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags", + "num-traits", + "rand 0.9.5", + "rand_chacha 0.9.0", + "rand_xorshift", + "regex-syntax", + "rusty-fork", + "tempfile", + "unarray", +] + +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quote" version = "1.0.46" @@ -1104,6 +1196,18 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "r-efi" +version = "5.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" + +[[package]] +name = "r-efi" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" + [[package]] name = "rand" version = "0.8.6" @@ -1111,8 +1215,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5ca0ecfa931c29007047d1bc58e623ab12e5590e8c7cc53200d5202b69266d8a" dependencies = [ "libc", - "rand_chacha", - "rand_core", + "rand_chacha 0.3.1", + "rand_core 0.6.4", +] + +[[package]] +name = "rand" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +dependencies = [ + "rand_chacha 0.9.0", + "rand_core 0.9.5", ] [[package]] @@ -1122,7 +1236,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" dependencies = [ "ppv-lite86", - "rand_core", + "rand_core 0.6.4", +] + +[[package]] +name = "rand_chacha" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" +dependencies = [ + "ppv-lite86", + "rand_core 0.9.5", ] [[package]] @@ -1131,7 +1255,25 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" dependencies = [ - "getrandom", + "getrandom 0.2.17", +] + +[[package]] +name = "rand_core" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76afc826de14238e6e8c374ddcc1fa19e374fd8dd986b0d2af0d02377261d83c" +dependencies = [ + "getrandom 0.3.4", +] + +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core 0.9.5", ] [[package]] @@ -1154,6 +1296,12 @@ dependencies = [ "syn", ] +[[package]] +name = "regex-syntax" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" + [[package]] name = "rfc6979" version = "0.4.0" @@ -1173,12 +1321,37 @@ dependencies = [ "semver", ] +[[package]] +name = "rustix" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys", + "windows-sys", +] + [[package]] name = "rustversion" version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "rusty-fork" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc6bf79ff24e648f6da1f8d1f011e9cac26491b619e6b9280f2b47f1774e6ee2" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "schemars" version = "0.8.22" @@ -1343,7 +1516,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77549399552de45a898a580c1b41d445bf730df867cc44e6c0233bbc4b8329de" dependencies = [ "digest", - "rand_core", + "rand_core 0.6.4", ] [[package]] @@ -1415,7 +1588,7 @@ dependencies = [ "ed25519-dalek", "elliptic-curve", "generic-array", - "getrandom", + "getrandom 0.2.17", "hex-literal", "hmac", "k256", @@ -1423,8 +1596,8 @@ dependencies = [ "num-integer", "num-traits", "p256", - "rand", - "rand_chacha", + "rand 0.8.6", + "rand_chacha 0.3.1", "sec1", "sha2", "sha3", @@ -1477,7 +1650,7 @@ dependencies = [ "ctor", "derive_arbitrary", "ed25519-dalek", - "rand", + "rand 0.8.6", "rustc_version", "serde", "serde_json", @@ -1642,6 +1815,19 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom 0.4.3", + "once_cell", + "rustix", + "windows-sys", +] + [[package]] name = "thiserror" version = "1.0.69" @@ -1713,6 +1899,12 @@ version = "1.20.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicode-ident" version = "1.0.24" @@ -1736,12 +1928,30 @@ dependencies = [ "syn", ] +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "wasi" version = "0.11.1+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" +[[package]] +name = "wasip2" +version = "1.0.4+wasi-0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b67efb37e106e55ce722a510d6b5f9c17f083e5fc79afc2badeb12cc313d9487" +dependencies = [ + "wit-bindgen", +] + [[package]] name = "wasm-bindgen" version = "0.2.126" @@ -1883,6 +2093,21 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-sys" +version = "0.61.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" +dependencies = [ + "windows-link", +] + +[[package]] +name = "wit-bindgen" +version = "0.57.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1ebf944e87a7c253233ad6766e082e3cd714b5d03812acc24c318f549614536e" + [[package]] name = "zerocopy" version = "0.8.52" diff --git a/Cargo.toml b/Cargo.toml index ed2dc6e..1f6d5c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ publish = false [workspace.dependencies] soroban-sdk = "26.1.0" +proptest = "1.4" # Build wasm contracts as small and fast as possible. [profile.release] diff --git a/contracts/common/Cargo.toml b/contracts/common/Cargo.toml index d6eecbd..50b7df1 100644 --- a/contracts/common/Cargo.toml +++ b/contracts/common/Cargo.toml @@ -11,3 +11,6 @@ crate-type = ["rlib"] [dependencies] soroban-sdk = { workspace = true } + +[dev-dependencies] +proptest = { workspace = true } diff --git a/contracts/common/src/lib.rs b/contracts/common/src/lib.rs index b32a225..122d882 100644 --- a/contracts/common/src/lib.rs +++ b/contracts/common/src/lib.rs @@ -1,10 +1,13 @@ #![no_std] -use soroban_sdk::{Address, Env, IntoVal, Val}; +use soroban_sdk::{token, Address, Env, IntoVal, Val}; mod split; pub use split::{compute_split, sort_remainders_desc, Payouts, SplitError}; +#[cfg(test)] +mod test_fuzz; + /// Trait to identify the Admin key for a contract's DataKey enum pub trait AdminKey { fn admin_key() -> Self; @@ -57,6 +60,30 @@ where pub const BPS_DENOMINATOR: i128 = 10_000; pub const MAX_SPONSORS: u32 = 20; +/// Maximum allowed single-step fee change (basis points) - Issue #20 +/// Prevents accidental or malicious fee spikes (e.g., 2.5% to 99%) +pub const MAX_FEE_CHANGE_BPS: u32 = 500; // 5% maximum change per call + +/// Validates a fee change is within acceptable bounds (Issue #20). +/// Ensures new fee is valid (≤100%) and change is ≤5% to prevent spikes. +pub fn validate_fee_change(old_fee: u32, new_fee: u32) -> Result<(), ()> { + if new_fee as i128 > BPS_DENOMINATOR { + return Err(()); + } + + let delta = if new_fee > old_fee { + new_fee - old_fee + } else { + old_fee - new_fee + }; + + if delta > MAX_FEE_CHANGE_BPS { + return Err(()); + } + + Ok(()) +} + pub fn extend_ttl(env: &Env, key: &K) where K: IntoVal, @@ -70,6 +97,44 @@ where /// here, not sub-day precision. See `extend_ttl_for_target`'s docs. const APPROX_SECONDS_PER_LEDGER: u64 = 5; +/// Measures the actual balance delta from a token transfer operation, +/// protecting against fee-on-transfer tokens, rebasing tokens, and malicious +/// token contracts (Issue #3). +/// +/// Instead of trusting the caller-supplied `amount`, this queries the +/// contract's actual token balance before and after the transfer and returns +/// the real delta. This prevents accounting desync where internal bookkeeping +/// (escrow.amount, milestone.remaining_budget) diverges from the contract's +/// actual holdings. +/// +/// # Example +/// ```ignore +/// let actual_received = measure_transfer_delta( +/// &env, +/// &token, +/// &env.current_contract_address(), +/// || { +/// token_client.transfer(&sponsor, &env.current_contract_address(), &amount); +/// }, +/// ); +/// // Use actual_received for bookkeeping instead of amount +/// ``` +pub fn measure_transfer_delta( + env: &Env, + token: &Address, + contract_addr: &Address, + operation: F, +) -> i128 +where + F: FnOnce(), +{ + let token_client = token::Client::new(env, token); + let balance_before = token_client.balance(contract_addr); + operation(); + let balance_after = token_client.balance(contract_addr); + balance_after - balance_before +} + /// Extends a persistent entry's TTL to (approximately) survive until /// `target_timestamp`, not just the fixed ~29-day (`500_000`-ledger) bump /// `extend_ttl` always applies regardless of context. diff --git a/contracts/common/src/test_fuzz.rs b/contracts/common/src/test_fuzz.rs new file mode 100644 index 0000000..a9b00a9 --- /dev/null +++ b/contracts/common/src/test_fuzz.rs @@ -0,0 +1,247 @@ +//! Property-based fuzz testing for compute_split (Issue #17) +//! +//! These tests use proptest to verify mathematical invariants hold across +//! a wide range of inputs, catching edge cases that unit tests might miss. + +#[cfg(test)] +mod tests { + extern crate std; + use std::vec; + use std::vec::Vec; + + use crate::BPS_DENOMINATOR; + use proptest::prelude::*; + + /// Simplified compute_split for testing (mirrors the contract logic) + fn compute_split_test(total: i128, recipients: &[u32], fee_bps: u32) -> Vec { + let fee = (total * fee_bps as i128) / BPS_DENOMINATOR; + let after_fee = total - fee; + + recipients.iter().map(|&bps| { + (after_fee * bps as i128) / BPS_DENOMINATOR + }).collect() + } + + proptest! { + /// Property: The sum of all recipient amounts should equal total - fee + /// This is the core invariant: no funds should disappear or be created + #[test] + fn test_sum_equals_total_minus_fee( + total in 1i128..=i128::MAX / 10_000, + fee_bps in 0u32..=10_000u32, + num_recipients in 2usize..=5usize, + ) { + // Generate random BPS values that sum to exactly 10000 + let mut bps_values: Vec = Vec::new(); + let mut remaining = 10_000u32; + + for i in 0..num_recipients - 1 { + let max_value = remaining.saturating_sub((num_recipients - i - 1) as u32); + let value = (i as u32 * 1000).min(max_value); + bps_values.push(value); + remaining -= value; + } + bps_values.push(remaining); + + // Compute the split + let result = compute_split_test(total, &bps_values, fee_bps); + + // Calculate expected fee + let fee = (total * fee_bps as i128) / BPS_DENOMINATOR; + let after_fee = total - fee; + + // Sum all recipient amounts + let sum: i128 = result.iter().sum(); + + // Property: sum should equal total - fee (within rounding tolerance) + // We allow ±num_recipients difference due to rounding + let diff = (sum - after_fee).abs(); + prop_assert!(diff <= num_recipients as i128, + "Sum mismatch: sum={}, expected={}, diff={}", sum, after_fee, diff); + } + + /// Property: With 0% fee, the sum should equal the total (minus rounding) + #[test] + fn test_zero_fee_preserves_total( + total in 1i128..=i128::MAX / 10_000, + num_recipients in 2usize..=5usize, + ) { + let mut bps_values: Vec = Vec::new(); + let mut remaining = 10_000u32; + + for i in 0..num_recipients - 1 { + let max_value = remaining.saturating_sub((num_recipients - i - 1) as u32); + let value = (i as u32 * 1000).min(max_value); + bps_values.push(value); + remaining -= value; + } + bps_values.push(remaining); + + let result = compute_split_test(total, &bps_values, 0); + + let sum: i128 = result.iter().sum(); + let diff = (sum - total).abs(); + + prop_assert!(diff <= num_recipients as i128, + "Zero-fee sum mismatch: sum={}, expected={}, diff={}", sum, total, diff); + } + + /// Property: With 100% fee, all recipients should get 0 + #[test] + fn test_full_fee_yields_zero( + total in 1i128..=i128::MAX / 10_000, + num_recipients in 2usize..=5usize, + ) { + let mut bps_values: Vec = Vec::new(); + let mut remaining = 10_000u32; + + for i in 0..num_recipients - 1 { + let max_value = remaining.saturating_sub((num_recipients - i - 1) as u32); + let value = (i as u32 * 1000).min(max_value); + bps_values.push(value); + remaining -= value; + } + bps_values.push(remaining); + + let result = compute_split_test(total, &bps_values, 10_000); + + for amount in result.iter() { + prop_assert_eq!(*amount, 0, "100% fee should yield 0 for all recipients"); + } + } + + /// Property: Each recipient's share should be proportional to their BPS + #[test] + fn test_proportional_distribution( + total in 1i128..=i128::MAX / 10_000, + fee_bps in 0u32..=10_000u32, + ) { + // Use fixed BPS for easier verification: 50%, 30%, 20% + let recipients = vec![5000u32, 3000u32, 2000u32]; + let result = compute_split_test(total, &recipients, fee_bps); + + let fee = (total * fee_bps as i128) / BPS_DENOMINATOR; + let after_fee = total - fee; + + // Calculate expected amounts + let expected_0 = (after_fee * 5000) / BPS_DENOMINATOR; + let expected_1 = (after_fee * 3000) / BPS_DENOMINATOR; + let expected_2 = (after_fee * 2000) / BPS_DENOMINATOR; + + // Allow small rounding differences + prop_assert!((result[0] - expected_0).abs() <= 1); + prop_assert!((result[1] - expected_1).abs() <= 1); + prop_assert!((result[2] - expected_2).abs() <= 1); + } + + /// Property: Increasing a recipient's BPS should never decrease their amount + #[test] + fn test_monotonic_bps( + total in 1i128..=i128::MAX / 10_000, + fee_bps in 0u32..=10_000u32, + increase in 1u32..=1000u32, + ) { + // Start with 50%, 50% split + let recipients_before = vec![5000u32, 5000u32]; + let result_before = compute_split_test(total, &recipients_before, fee_bps); + + // Increase first recipient's share, decrease second + let new_bps = (5000u32 + increase).min(9999); + let recipients_after = vec![new_bps, 10000 - new_bps]; + let result_after = compute_split_test(total, &recipients_after, fee_bps); + + // First recipient should get more (or same) + prop_assert!(result_after[0] >= result_before[0], + "Increasing BPS should not decrease amount: before={}, after={}", + result_before[0], result_after[0]); + } + + /// Property: No recipient should receive more than the total amount + #[test] + fn test_no_amount_exceeds_total( + total in 1i128..=i128::MAX / 10_000, + fee_bps in 0u32..=10_000u32, + num_recipients in 2usize..=5usize, + ) { + let mut bps_values: Vec = Vec::new(); + let mut remaining = 10_000u32; + + for i in 0..num_recipients - 1 { + let max_value = remaining.saturating_sub((num_recipients - i - 1) as u32); + let value = (i as u32 * 1000).min(max_value); + bps_values.push(value); + remaining -= value; + } + bps_values.push(remaining); + + let result = compute_split_test(total, &bps_values, fee_bps); + + for amount in result.iter() { + prop_assert!(*amount <= total, + "Recipient amount {} exceeds total {}", amount, total); + } + } + + /// Property: All amounts should be non-negative + #[test] + fn test_non_negative_amounts( + total in 1i128..=i128::MAX / 10_000, + fee_bps in 0u32..=10_000u32, + num_recipients in 2usize..=5usize, + ) { + let mut bps_values: Vec = Vec::new(); + let mut remaining = 10_000u32; + + for i in 0..num_recipients - 1 { + let max_value = remaining.saturating_sub((num_recipients - i - 1) as u32); + let value = (i as u32 * 1000).min(max_value); + bps_values.push(value); + remaining -= value; + } + bps_values.push(remaining); + + let result = compute_split_test(total, &bps_values, fee_bps); + + for amount in result.iter() { + prop_assert!(*amount >= 0, "Amount should be non-negative: {}", amount); + } + } + } + + /// Edge case: Single recipient should get (total - fee) + #[test] + fn test_single_recipient() { + let total = 1_000_000i128; + let fee_bps = 250u32; // 2.5% + let recipients = vec![10_000u32]; // 100% + + let result = compute_split_test(total, &recipients, fee_bps); + let fee = (total * fee_bps as i128) / BPS_DENOMINATOR; + let expected = total - fee; + + assert_eq!(result.len(), 1); + assert_eq!(result[0], expected); + } + + /// Edge case: Zero total amount + #[test] + fn test_zero_total() { + let total = 0i128; + let recipients = vec![5000u32, 5000u32]; + let result = compute_split_test(total, &recipients, 250); + + assert_eq!(result, vec![0i128, 0i128]); + } + + /// Edge case: Very small amounts with rounding + #[test] + fn test_small_amounts_rounding() { + let total = 10i128; + let recipients = vec![3333u32, 3333u32, 3334u32]; // Sums to 10000 + let result = compute_split_test(total, &recipients, 0); + + // Sum should equal total (within rounding) + let sum: i128 = result.iter().sum(); + assert!((sum - total).abs() <= 3, "Rounding error too large"); + } +} diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index bd60fff..ef869a5 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -137,14 +137,25 @@ impl EscrowContract { } let token_client = token::Client::new(&env, &token); - token_client.transfer(&sponsor, env.current_contract_address(), &amount); + let actual_received = mergefi_common::measure_transfer_delta( + &env, + &token, + &env.current_contract_address(), + || { + token_client.transfer(&sponsor, &env.current_contract_address(), &amount); + }, + ); + + if actual_received <= 0 { + return Err(Error::InvalidAmount); + } let contribution_key = DataKey::Contribution(issue_id, 0); env.storage().persistent().set( &contribution_key, &Contribution { sponsor, - amount, + amount: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -152,7 +163,7 @@ impl EscrowContract { let escrow = Escrow { token, - amount, + amount: actual_received, status: EscrowStatus::Funded, created_at: env.ledger().timestamp(), deadline, @@ -224,13 +235,24 @@ impl EscrowContract { } let token_client = token::Client::new(&env, &escrow.token); - token_client.transfer(&sponsor, env.current_contract_address(), &amount); + let actual_received = mergefi_common::measure_transfer_delta( + &env, + &escrow.token, + &env.current_contract_address(), + || { + token_client.transfer(&sponsor, &env.current_contract_address(), &amount); + }, + ); + + if actual_received <= 0 { + return Err(Error::InvalidAmount); + } if let Some(index) = existing_index { let contribution_key = DataKey::Contribution(issue_id, index); let mut contribution: Contribution = env.storage().persistent().get(&contribution_key).unwrap(); - contribution.amount += amount; + contribution.amount += actual_received; contribution.timestamp = env.ledger().timestamp(); env.storage() .persistent() @@ -242,7 +264,7 @@ impl EscrowContract { &contribution_key, &Contribution { sponsor, - amount, + amount: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -250,7 +272,7 @@ impl EscrowContract { escrow.contributor_count += 1; } - escrow.amount += amount; + escrow.amount += actual_received; env.storage().persistent().set(&key, &escrow); extend_ttl(&env, &key); extend_instance_ttl(&env); @@ -665,6 +687,38 @@ impl EscrowContract { .get(&DataKey::MaxSponsors) .ok_or(Error::NotInitialized) } + + /// Admin-only: update the protocol fee for NEW escrows created after + /// this call (Issue #20). Existing escrows are unaffected - they retain + /// the fee that was active when they were funded, providing sponsor trust + /// and predictability. + /// + /// The change is limited to 5% (500 basis points) per call to prevent + /// accidental or malicious fee spikes. For example, changing from 2.5% + /// (250 bps) to 7.5% (750 bps) requires two calls. + /// + /// # Example + /// ```ignore + /// // Current fee is 2.5% (250 bps) + /// // Can change to max 7.5% (750 bps) or min 0% (0 bps) + /// client.set_fee_bps(&500); // Sets to 5% + /// ``` + pub fn set_fee_bps(env: Env, new_fee_bps: u32) -> Result<(), Error> { + require_admin(&env)?.require_auth(); + + let current_fee: u32 = env + .storage() + .instance() + .get(&DataKey::FeeBps) + .ok_or(Error::NotInitialized)?; + + mergefi_common::validate_fee_change(current_fee, new_fee_bps) + .map_err(|_| Error::InvalidFee)?; + + env.storage().instance().set(&DataKey::FeeBps, &new_fee_bps); + extend_instance_ttl(&env); + Ok(()) + } } pub(crate) fn require_admin(env: &Env) -> Result { diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index c9ac350..d5c3b3e 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -1644,7 +1644,7 @@ pub struct MockPanicToken; #[contractimpl] impl MockPanicToken { - pub fn transfer(env: Env, _from: Address, to: Address, _amount: i128) { + pub fn transfer(env: Env, from: Address, to: Address, amount: i128) { let blocked_key = soroban_sdk::Symbol::new(&env, "blocked"); if env.storage().instance().has(&blocked_key) { let blocked: Address = env.storage().instance().get(&blocked_key).unwrap(); @@ -1652,12 +1652,33 @@ impl MockPanicToken { panic!("Frozen/unauthorized trustline recipient"); } } + + // Update balances + let from_key = soroban_sdk::Symbol::new(&env, "bal"); + let to_key = soroban_sdk::Symbol::new(&env, "bal"); + + let from_bal: i128 = env.storage().persistent().get(&(from_key.clone(), from.clone())).unwrap_or(0); + let to_bal: i128 = env.storage().persistent().get(&(to_key.clone(), to.clone())).unwrap_or(0); + + env.storage().persistent().set(&(from_key, from), &(from_bal - amount)); + env.storage().persistent().set(&(to_key, to), &(to_bal + amount)); } pub fn set_blocked(env: Env, blocked: Address) { let blocked_key = soroban_sdk::Symbol::new(&env, "blocked"); env.storage().instance().set(&blocked_key, &blocked); } + + pub fn balance(env: Env, id: Address) -> i128 { + let bal_key = soroban_sdk::Symbol::new(&env, "bal"); + env.storage().persistent().get(&(bal_key, id)).unwrap_or(0) + } + + pub fn mint(env: Env, to: Address, amount: i128) { + let bal_key = soroban_sdk::Symbol::new(&env, "bal"); + let balance: i128 = env.storage().persistent().get(&(bal_key.clone(), to.clone())).unwrap_or(0); + env.storage().persistent().set(&(bal_key, to), &(balance + amount)); + } } #[test] @@ -1675,6 +1696,7 @@ fn test_release_all_or_nothing_revert_with_blocked_recipient() { let blocked_dev = Address::generate(&env); panic_client.set_blocked(&blocked_dev); + panic_client.mint(&sponsor, &100_000i128); // Fund the escrow client.fund(&700u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 031925e..88c7401 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -121,7 +121,18 @@ impl MilestonesContract { } let token_client = token::Client::new(&env, &token); - token_client.transfer(&sponsor, env.current_contract_address(), &total_budget); + let actual_received = mergefi_common::measure_transfer_delta( + &env, + &token, + &env.current_contract_address(), + || { + token_client.transfer(&sponsor, &env.current_contract_address(), &total_budget); + }, + ); + + if actual_received <= 0 { + return Err(Error::InvalidAmount); + } // The original funder is always contribution index 0, exactly like // `escrow::fund`; every later sponsor appends via `contribute`. @@ -130,7 +141,7 @@ impl MilestonesContract { &contribution_key, &Contribution { sponsor: sponsor.clone(), - amount: total_budget, + amount: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -139,8 +150,8 @@ impl MilestonesContract { let milestone = Milestone { sponsor, token, - total_budget, - remaining_budget: total_budget, + total_budget: actual_received, + remaining_budget: actual_received, created_at: env.ledger().timestamp(), deadline, closed: false, @@ -208,13 +219,24 @@ impl MilestonesContract { } let token_client = token::Client::new(&env, &milestone.token); - token_client.transfer(&sponsor, env.current_contract_address(), &amount); + let actual_received = mergefi_common::measure_transfer_delta( + &env, + &milestone.token, + &env.current_contract_address(), + || { + token_client.transfer(&sponsor, &env.current_contract_address(), &amount); + }, + ); + + if actual_received <= 0 { + return Err(Error::InvalidAmount); + } if let Some(index) = existing_index { let contribution_key = DataKey::Contribution(milestone_id, index); let mut contribution: Contribution = env.storage().persistent().get(&contribution_key).unwrap(); - contribution.amount += amount; + contribution.amount += actual_received; contribution.timestamp = env.ledger().timestamp(); env.storage() .persistent() @@ -226,7 +248,7 @@ impl MilestonesContract { &contribution_key, &Contribution { sponsor, - amount, + amount: actual_received, timestamp: env.ledger().timestamp(), }, ); @@ -237,8 +259,8 @@ impl MilestonesContract { // 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 += amount; - milestone.remaining_budget += amount; + milestone.total_budget += actual_received; + milestone.remaining_budget += actual_received; env.storage().persistent().set(&mkey, &milestone); extend_ttl(&env, &mkey); @@ -315,6 +337,11 @@ impl MilestonesContract { /// Admin-only: releases the previously allocated amount for `issue_id` /// to `recipients` (basis points summing to 10000), minus the protocol /// fee, exactly as in the escrow contract. + /// + /// Issue #5 fix: This now checks if the milestone is closed and rejects + /// the release with `Error::MilestoneClosed`. To release funds after + /// deciding to cancel, use `deallocate` first to move allocated amounts + /// back to `remaining_budget`, then call `cancel_milestone` to refund. pub fn release_issue( env: Env, milestone_id: u64, @@ -334,6 +361,7 @@ impl MilestonesContract { .get(&mkey) .ok_or(Error::MilestoneNotFound)?; + // Issue #5 fix: Block release_issue on closed milestones if milestone.closed { return Err(Error::MilestoneClosed); } @@ -425,9 +453,14 @@ impl MilestonesContract { /// Admin-only: deallocates a previously allocated (but not yet released) /// issue, moving its amount back into `remaining_budget`. This unblocks /// scenarios where an allocation was made in error or the issue is no - /// longer relevant, and is required before #5's fix (which blocks - /// `release_issue` on closed milestones) strands allocated-but-unreleased - /// funds permanently (#43). + /// longer relevant. + /// + /// **Required workflow for proper milestone cancellation (Issue #5):** + /// 1. Call `deallocate` for each allocated-but-unreleased issue + /// 2. Call `cancel_milestone` to refund the remaining_budget + /// + /// After Issue #5 fix, `release_issue` blocks on closed milestones, so + /// allocated funds would be permanently stuck without deallocating first. /// /// Rejects if the issue is already Released (funds have left the /// contract) or not currently Allocated. @@ -697,6 +730,30 @@ impl MilestonesContract { .get(&DataKey::MaxSponsors) .ok_or(Error::NotInitialized) } + + /// Admin-only: update the protocol fee for NEW milestones created after + /// this call (Issue #20). Existing milestones are unaffected - they retain + /// the fee that was active when they were created, providing sponsor trust + /// and predictability. + /// + /// The change is limited to 5% (500 basis points) per call to prevent + /// accidental or malicious fee spikes. + pub fn set_fee_bps(env: Env, new_fee_bps: u32) -> Result<(), Error> { + require_admin(&env)?.require_auth(); + + let current_fee: u32 = env + .storage() + .instance() + .get(&DataKey::FeeBps) + .ok_or(Error::NotInitialized)?; + + mergefi_common::validate_fee_change(current_fee, new_fee_bps) + .map_err(|_| Error::InvalidFee)?; + + env.storage().instance().set(&DataKey::FeeBps, &new_fee_bps); + extend_instance_ttl(&env); + Ok(()) + } } /// Pays each contributor their share of `milestone.remaining_budget` (the diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 6e3c362..61ab6f1 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -939,7 +939,7 @@ pub struct MockPanicToken; #[contractimpl] impl MockPanicToken { - pub fn transfer(env: Env, _from: Address, to: Address, _amount: i128) { + pub fn transfer(env: Env, from: Address, to: Address, amount: i128) { let blocked_key = soroban_sdk::Symbol::new(&env, "blocked"); if env.storage().instance().has(&blocked_key) { let blocked: Address = env.storage().instance().get(&blocked_key).unwrap(); @@ -947,12 +947,33 @@ impl MockPanicToken { panic!("Frozen/unauthorized trustline recipient"); } } + + // Update balances + let from_key = soroban_sdk::Symbol::new(&env, "bal"); + let to_key = soroban_sdk::Symbol::new(&env, "bal"); + + let from_bal: i128 = env.storage().persistent().get(&(from_key.clone(), from.clone())).unwrap_or(0); + let to_bal: i128 = env.storage().persistent().get(&(to_key.clone(), to.clone())).unwrap_or(0); + + env.storage().persistent().set(&(from_key, from), &(from_bal - amount)); + env.storage().persistent().set(&(to_key, to), &(to_bal + amount)); } pub fn set_blocked(env: Env, blocked: Address) { let blocked_key = soroban_sdk::Symbol::new(&env, "blocked"); env.storage().instance().set(&blocked_key, &blocked); } + + pub fn balance(env: Env, id: Address) -> i128 { + let bal_key = soroban_sdk::Symbol::new(&env, "bal"); + env.storage().persistent().get(&(bal_key, id)).unwrap_or(0) + } + + pub fn mint(env: Env, to: Address, amount: i128) { + let bal_key = soroban_sdk::Symbol::new(&env, "bal"); + let balance: i128 = env.storage().persistent().get(&(bal_key.clone(), to.clone())).unwrap_or(0); + env.storage().persistent().set(&(bal_key, to), &(balance + amount)); + } } #[test] @@ -970,6 +991,7 @@ fn test_release_issue_all_or_nothing_revert_with_blocked_recipient() { let blocked_dev = Address::generate(&env); panic_client.set_blocked(&blocked_dev); + panic_client.mint(&sponsor, &100_000i128); // Create a milestone and allocate to an issue client.create_milestone(&70u64, &sponsor, &token_addr, &10_000i128, &1_000u64);