diff --git a/CHANGELOG.md b/CHANGELOG.md index e28ea290..c19dcbaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,19 @@ change, not backfilled later. contract's `Unauthorized` variant remains (used in `extend_deadline`). Update client-side error decoders (`scripts/error-codes.mjs`) accordingly. (#319) +- `compute_split` now rejects zero-bps recipients and duplicate addresses with + `InvalidSplit`, and it rejects self-payouts before any transfer executes + (#425). +- Added explicit overflow error handling for arithmetic-boundary failures and + surfaced the dedicated `ArithmeticOverflow` family of variants instead of + relying on a host abort (#429). +- Enforced the per-contract `MAX_DEPOSITS`/`TooManyDeposits` cap and added the + dedicated error code used by sponsor contribution flows (#427). +- Typed contribution lookup failures (`ContributionNotFound` / related typed + errors) are now surfaced consistently instead of falling back to generic + logic (#421, #422). +- Reclaim-window logic now aligns with the actual post-deadline grace-period + behavior used by the refund/recovery path (#423). ### Added - `CODEOWNERS`, pinning `contracts/`, `docs/` and `scripts/` review to the @@ -39,7 +52,7 @@ change, not backfilled later. Escrows gained multi-sponsor crowdfunding: several sponsors can contribute toward one escrow's funding target instead of a single sponsor funding it outright, with a permissionless refund path once the deadline plus -`GRACE_PERIOD` has passed and the target wasn't met. +`GRACE_PERIOD` has passed, regardless of whether the target was met. ### Pause / circuit-breaker mechanism (#14) `escrow`, `maintenance-pool`, and `milestones` each gained `pause`/`unpause` diff --git a/SECURITY.md b/SECURITY.md index dd011247..082a2810 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,7 +12,7 @@ Older tagged releases are not actively patched. Please upgrade to the latest com **Do not open a public GitHub issue for security vulnerabilities.** -Send a report by email to the maintainers listed in [CODEOWNERS](.github/CODEOWNERS) or use GitHub's private [Security Advisory](../../security/advisories/new) feature. +Use GitHub's private [Security Advisory](../../security/advisories/new) feature for private disclosure. The repository does not publish an email address for security reports, and public issues or pull requests are not an acceptable disclosure channel. Include: @@ -27,4 +27,4 @@ We will acknowledge receipt within **48 hours** and aim to provide a resolution This policy covers all Soroban smart contracts under the `contracts/` directory of this repository. -Off-chain tooling (`scripts/`, `bindings/`) is in scope for disclosure but carries lower severity by default. +Off-chain tooling under `scripts/` is in scope for disclosure but carries lower severity by default. The repository does not maintain a `bindings/` directory, so it is intentionally out of scope until a concrete implementation exists. diff --git a/contracts/common/src/split.rs b/contracts/common/src/split.rs index 9220f728..35e7154e 100644 --- a/contracts/common/src/split.rs +++ b/contracts/common/src/split.rs @@ -32,8 +32,9 @@ pub struct Payouts { /// already has, e.g. `compute_split(..).map_err(|_| Error::InvalidSplit)?`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SplitError { - /// `recipients` was empty, or its basis-point shares don't sum to - /// exactly `BPS_DENOMINATOR` (10000 = 100%). + /// `recipients` was empty; any share is zero; a recipient address appears + /// more than once; or the 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. @@ -56,6 +57,19 @@ pub fn compute_split( return Err(SplitError::InvalidSplit); } + for i in 0..recipients.len() { + let (recipient, bps) = recipients.get(i).unwrap(); + if bps == 0 { + return Err(SplitError::InvalidSplit); + } + for j in 0..i { + let (other_recipient, _) = recipients.get(j).unwrap(); + if recipient == other_recipient { + return Err(SplitError::InvalidSplit); + } + } + } + let mut bps_sum: i128 = 0; for (_, bps) in recipients.iter() { bps_sum = bps_sum diff --git a/contracts/common/src/test_fuzz.rs b/contracts/common/src/test_fuzz.rs index 8e9a9aff..099f814b 100644 --- a/contracts/common/src/test_fuzz.rs +++ b/contracts/common/src/test_fuzz.rs @@ -278,6 +278,43 @@ mod tests { )); } + /// `compute_split` must reject a 0-bps recipient even when the remaining + /// percentages still sum to 100%. + #[test] + fn test_zero_bps_recipient_rejected() { + use crate::{compute_split, SplitError}; + use soroban_sdk::{testutils::Address as _, Address, Env, Vec as SorobanVec}; + + let env = Env::default(); + let contract_id = env.register(DummyContract, ()); + let a = Address::generate(&env); + let b = Address::generate(&env); + let recipients = SorobanVec::from_array(&env, [(a.clone(), 5_000u32), (b, 0u32), (a.clone(), 5_000u32)]); + + let result = env.as_contract(&contract_id, || { + compute_split(&env, 1_000_000, 0, &recipients) + }); + assert!(matches!(result, Err(SplitError::InvalidSplit))); + } + + /// Duplicate recipient addresses are invalid because they create multiple + /// transfers to the same address and distort indexer accounting. + #[test] + fn test_duplicate_recipients_rejected() { + use crate::{compute_split, SplitError}; + 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(), 5_000u32), (recipient, 5_000u32)]); + + let result = env.as_contract(&contract_id, || { + compute_split(&env, 1_000_000, 0, &recipients) + }); + assert!(matches!(result, Err(SplitError::InvalidSplit))); + } + /// Sanity counterpart: the real `compute_split` accepts a single /// recipient at 100%, so the empty-list rejection above is specific to /// emptiness and not a blanket failure of the harness. diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 64510d74..9d97b71e 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -989,6 +989,11 @@ mod contract { mergefi_common::get_fee_bps::(&env).ok_or(Error::NotInitialized) } + /// Admin-only: rotate the treasury address. + /// + /// Requires the current admin to authorize the change. The call rejects + /// an invalid treasury candidate (`InvalidTreasury`) and, once accepted, + /// routes all subsequent protocol-fee payouts to the updated treasury. pub fn set_treasury(env: Env, new_treasury: Address) -> Result<(), Error> { let admin = require_admin(&env)?; admin.require_auth();