From 725f94e3fa6248444c899aebecd1b0bed1f35d19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CDove1010=E2=80=9D?= <“bookwiffidrips@gmail.com”> Date: Mon, 28 Sep 2026 04:13:36 +0100 Subject: [PATCH] fix(compute_split): reject self-payouts that strand funds in the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute_split previously validated only that recipients is non-empty and bps values sum to exactly 10_000 — it performed no validation on the addresses themselves. This adds a dedicated SplitError::SelfPayout variant (Error::SelfPayout in both escrow and milestones) that fires when any recipient equals env.current_contract_address(). Why this case specifically: a self-transfer leaves tokens in the contract, but the escrow/milestone status is marked Paid/Released *before* the transfer loop runs, so release/refund are permanently blocked by the terminal status and the funds are stranded with no recovery path. The other address-collision scenarios were audited and intentionally left unvalidated: - Duplicate addresses: compute_split pushes each (addr, share) pair into shares independently, and the payout loop transfers per entry, so two entries for the same address correctly produce two separate transfers summing to the full combined entitlement. - Treasury as recipient: the fee transfer and the recipient transfer are separate token::Client::transfer calls, so the treasury simply receives fee + share across two sequential transfers — no double-counting or loss. - Admin address (milestones): the admin is a plain address with no special token-receiving semantics, so it is a valid recipient like any other. Also fixes a pre-existing missing closing brace in escrow's set_treasury that prevented the crate from compiling at all. Tests added (7 new, all passing): - escrow: duplicate recipients, treasury-as-recipient, self-payout rejection - milestones: duplicate recipients, treasury-as-recipient, self-payout rejection, admin-as-recipient - common: updated fuzz tests to run inside a contract context (required by the new current_contract_address() call) --- contracts/common/src/split.rs | 14 ++++ contracts/common/src/test_fuzz.rs | 21 +++++- contracts/escrow/src/error.rs | 3 + contracts/escrow/src/lib.rs | 7 +- contracts/escrow/src/test.rs | 100 ++++++++++++++++++++++++++++ contracts/milestones/src/error.rs | 3 + contracts/milestones/src/lib.rs | 5 +- contracts/milestones/src/test.rs | 106 ++++++++++++++++++++++++++++++ 8 files changed, 254 insertions(+), 5 deletions(-) diff --git a/contracts/common/src/split.rs b/contracts/common/src/split.rs index e322b97..c5692b9 100644 --- a/contracts/common/src/split.rs +++ b/contracts/common/src/split.rs @@ -33,6 +33,9 @@ pub enum SplitError { /// `recipients` was empty, or its basis-point shares don't sum to /// exactly `BPS_DENOMINATOR` (10000 = 100%). InvalidSplit, + /// A recipient address equals the contract's own address — a self-payout + /// that would strand funds in the contract with no recovery path. + SelfPayout, } /// Validates that basis-point splits sum to exactly 10000 and computes the @@ -55,6 +58,17 @@ pub fn compute_split( return Err(SplitError::InvalidSplit); } + // Reject self-payouts: a recipient equal to the contract's own address + // would strand funds — the status is marked Paid/Released before the + // transfer loop runs, so a self-transfer leaves tokens in the contract + // with no future release or refund path to recover them. + let contract_address = env.current_contract_address(); + for (recipient, _) in recipients.iter() { + if recipient == contract_address { + return Err(SplitError::SelfPayout); + } + } + let fee = total * (fee_bps as i128) / BPS_DENOMINATOR; let distributable = total - fee; diff --git a/contracts/common/src/test_fuzz.rs b/contracts/common/src/test_fuzz.rs index 2678e1a..8e9a9af 100644 --- a/contracts/common/src/test_fuzz.rs +++ b/contracts/common/src/test_fuzz.rs @@ -11,6 +11,13 @@ mod tests { use crate::BPS_DENOMINATOR; use proptest::prelude::*; + use soroban_sdk::{contract, contractimpl}; + + #[contract] + pub struct DummyContract; + + #[contractimpl] + impl DummyContract {} /// Simplified compute_split for testing (mirrors the contract logic) fn compute_split_test(total: i128, recipients: &[u32], fee_bps: u32) -> Vec { @@ -254,16 +261,19 @@ mod tests { use soroban_sdk::{Address, Env, Vec as SorobanVec}; let env = Env::default(); + let contract_id = env.register(DummyContract, ()); let recipients: SorobanVec<(Address, u32)> = SorobanVec::new(&env); for fee_bps in [0u32, 250, 10_000] { - let result = compute_split(&env, 1_000_000, fee_bps, &recipients); + let result = env.as_contract(&contract_id, || { + compute_split(&env, 1_000_000, fee_bps, &recipients) + }); assert!(matches!(result, Err(SplitError::InvalidSplit))); } // Also rejected for a zero total — emptiness alone is invalid. assert!(matches!( - compute_split(&env, 0, 0, &recipients), + env.as_contract(&contract_id, || compute_split(&env, 0, 0, &recipients)), Err(SplitError::InvalidSplit) )); } @@ -277,10 +287,15 @@ mod tests { use soroban_sdk::{testutils::Address as _, Address, Env, Vec as SorobanVec}; let env = Env::default(); + let contract_id = env.register(DummyContract, ()); let recipient = Address::generate(&env); let recipients = SorobanVec::from_array(&env, [(recipient.clone(), 10_000u32)]); - let payouts = compute_split(&env, 1_000_000, 250, &recipients).unwrap(); + let payouts = env + .as_contract(&contract_id, || { + compute_split(&env, 1_000_000, 250, &recipients) + }) + .unwrap(); assert_eq!(payouts.fee, 25_000); assert_eq!(payouts.shares.len(), 1); assert_eq!(payouts.shares.get(0).unwrap(), (recipient, 975_000)); diff --git a/contracts/escrow/src/error.rs b/contracts/escrow/src/error.rs index 8c383a1..71ed7f1 100644 --- a/contracts/escrow/src/error.rs +++ b/contracts/escrow/src/error.rs @@ -26,4 +26,7 @@ pub enum Error { InvalidTreasury = 17, /// Contract is paused and this operation is not allowed (issue #14). ContractPaused = 19, + /// A recipient address equals the contract's own address — a self-payout + /// that would strand funds in the contract with no recovery path. + SelfPayout = 20, } diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 2f7ee89..d5c6b53 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -312,7 +312,10 @@ impl EscrowContract { let fee_bps: u32 = mergefi_common::get_fee_bps::(&env) .ok_or(Error::NotInitialized)?; let payouts = mergefi_common::compute_split(&env, escrow.amount, fee_bps, &recipients) - .map_err(|_| Error::InvalidSplit)?; + .map_err(|e| match e { + mergefi_common::SplitError::InvalidSplit => Error::InvalidSplit, + mergefi_common::SplitError::SelfPayout => Error::SelfPayout, + })?; let treasury: Address = mergefi_common::require_treasury::(&env).unwrap(); let token_client = token::Client::new(&env, &escrow.token); let contract_address = env.current_contract_address(); @@ -677,6 +680,8 @@ impl EscrowContract { env.storage().instance().set(&DataKey::Treasury, &new_treasury); extend_instance_ttl(&env); Ok(()) + } + pub fn get_version(env: Env) -> u32 { env.storage().instance().get(&DataKey::Version).unwrap_or(0) } diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index e04041c..59d1417 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -2195,3 +2195,103 @@ fn test_extend_deadline_returns_contribution_not_found_when_archived() { assert_eq!(err, Err(Ok(Error::ContributionNotFound))); } +#[test] +fn test_release_with_duplicate_recipient_addresses() { + let env = Env::default(); + env.mock_all_auths(); + let (_, _admin, treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + let alice = Address::generate(&env); + + client.fund( + &200u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + &None, + ); + + // Same address twice with 5000 bps each — must produce two separate + // transfers summing to the full 10000-bps entitlement. + let recipients = vec![&env, (alice.clone(), 5_000u32), (alice.clone(), 5_000u32)]; + client.release(&200u64, &recipients); + + let distributable = 950_0000000i128; // after 5% fee + assert_eq!(token_client.balance(&alice), distributable); + assert_eq!(token_client.balance(&treasury), 50_0000000i128); +} + +#[test] +fn test_release_with_treasury_as_recipient() { + let env = Env::default(); + env.mock_all_auths(); + let (_, _admin, treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + let alice = Address::generate(&env); + + client.fund( + &201u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + &None, + ); + + // Treasury as one of the recipients — fee transfer and recipient transfer + // are separate calls; treasury must receive fee + share without double-counting. + let recipients = vec![&env, (alice.clone(), 5_000u32), (treasury.clone(), 5_000u32)]; + client.release(&201u64, &recipients); + + let distributable = 950_0000000i128; // after 5% fee + let alice_expected = distributable * 5000 / 10000; + let treasury_expected = 50_0000000i128 + (distributable - alice_expected); + assert_eq!(token_client.balance(&alice), alice_expected); + assert_eq!(token_client.balance(&treasury), treasury_expected); +} + +#[test] +fn test_release_rejects_self_payout() { + let env = Env::default(); + env.mock_all_auths(); + let (_contract_id, _admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + client.fund( + &202u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + &None, + ); + + // Recipient equal to the contract's own address must be rejected. + let recipients = vec![ + &env, + (client.address.clone(), 5_000u32), + (Address::generate(&env), 5_000u32), + ]; + let err = client.try_release(&202u64, &recipients); + assert_eq!(err, Err(Ok(Error::SelfPayout))); + + // Escrow must remain Funded (not Paid) so the funds are not stranded. + let escrow = client.get_escrow(&202u64); + assert_eq!(escrow.status, crate::types::EscrowStatus::Funded); +} + diff --git a/contracts/milestones/src/error.rs b/contracts/milestones/src/error.rs index cfcdb75..3f7a8be 100644 --- a/contracts/milestones/src/error.rs +++ b/contracts/milestones/src/error.rs @@ -30,4 +30,7 @@ pub enum Error { ContractPaused = 18, /// The contribution index is out of range for an existing milestone (issue #256). ContributionNotFound = 19, + /// A recipient address equals the contract's own address — a self-payout + /// that would strand funds in the contract with no recovery path. + SelfPayout = 20, } diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 673f4cf..d20ae23 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -384,7 +384,10 @@ impl MilestonesContract { let fee_bps: u32 = mergefi_common::get_fee_bps::(&env) .ok_or(Error::NotInitialized)?; let payouts = mergefi_common::compute_split(&env, amount, fee_bps, &recipients) - .map_err(|_| Error::InvalidSplit)?; + .map_err(|e| match e { + mergefi_common::SplitError::InvalidSplit => Error::InvalidSplit, + mergefi_common::SplitError::SelfPayout => Error::SelfPayout, + })?; let treasury: Address = mergefi_common::require_treasury::(&env).unwrap(); let token_client = token::Client::new(&env, &milestone.token); let contract_address = env.current_contract_address(); diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 88a530c..981ea64 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -1806,3 +1806,109 @@ fn test_cancel_milestone_after_deadline_with_partial_allocation() { // Sponsor should receive full refund of remaining 40% (no fee on refunds) assert_eq!(token_client.balance(&sponsor), 4_000_000_000i128); } + +#[test] +fn test_release_issue_with_duplicate_recipient_addresses() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + let alice = Address::generate(&env); + + client.create_milestone(&2u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.allocate(&2u64, &201u64, &10_000_000_000i128); + + // Same address twice with 5000 bps each — must produce two separate + // transfers summing to the full 10000-bps entitlement. + let recipients = vec![&env, (alice.clone(), 5_000u32), (alice.clone(), 5_000u32)]; + client.release_issue(&2u64, &201u64, &recipients); + + let distributable = 950_0000000i128; // after 5% fee + assert_eq!(token_client.balance(&alice), distributable); + assert_eq!(token_client.balance(&treasury), 50_0000000i128); +} + +#[test] +fn test_release_issue_with_treasury_as_recipient() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + let alice = Address::generate(&env); + + client.create_milestone(&2u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.allocate(&2u64, &202u64, &10_000_000_000i128); + + // Treasury as one of the recipients — fee transfer and recipient transfer + // are separate calls; treasury must receive fee + share without double-counting. + let recipients = vec![&env, (alice.clone(), 5_000u32), (treasury.clone(), 5_000u32)]; + client.release_issue(&2u64, &202u64, &recipients); + + let distributable = 950_0000000i128; // after 5% fee + let alice_expected = distributable * 5000 / 10000; + let treasury_expected = 50_0000000i128 + (distributable - alice_expected); + assert_eq!(token_client.balance(&alice), alice_expected); + assert_eq!(token_client.balance(&treasury), treasury_expected); +} + +#[test] +fn test_release_issue_rejects_self_payout() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + client.create_milestone(&2u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.allocate(&2u64, &203u64, &10_000_000_000i128); + + // Recipient equal to the contract's own address must be rejected. + let recipients = vec![ + &env, + (client.address.clone(), 5_000u32), + (Address::generate(&env), 5_000u32), + ]; + let err = client.try_release_issue(&2u64, &203u64, &recipients); + assert_eq!(err, Err(Ok(Error::SelfPayout))); + + // Issue must remain Allocated (not Released) so the funds are not stranded. + assert_eq!( + client.get_issue_status(&2u64, &203u64), + crate::types::IssueStatus::Allocated + ); +} + +#[test] +fn test_release_issue_with_admin_as_recipient() { + let env = Env::default(); + env.mock_all_auths(); + let (admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + client.create_milestone(&2u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.allocate(&2u64, &204u64, &10_000_000_000i128); + + // Admin is a regular address — must be a valid recipient. + let recipients = vec![&env, (admin.clone(), 10_000u32)]; + client.release_issue(&2u64, &204u64, &recipients); + + let distributable = 950_0000000i128; // after 5% fee + assert_eq!(token_client.balance(&admin), distributable); +}