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); +}