From f7b3ae42c6374387fb1829d76c965f52f7596abc Mon Sep 17 00:00:00 2001 From: rdj-savyy Date: Thu, 27 Aug 2026 23:25:23 +0000 Subject: [PATCH 1/4] fix(invoice): guard mark_paids against terminal/refund-state overrides mark_paids collapsed every non-Pending status into the generic InvoiceAlreadyPaid error, so calling it on an invoice that was already RefundRequested, Released, Cancelled, or Expired was rejected but with a misleading error and no explicit acknowledgement that a payer's refund request could otherwise be clobbered by a stale payment confirmation. Add an explicit guard at the top of the loop that returns the new ContractError::InvalidStateTransition for those four states, keeping InvoiceAlreadyPaid for the genuinely-already-paid case. Also add the Overflow and AddressBlocked variants that the file already referenced but never declared on ContractError, which left the crate unable to compile. Co-Authored-By: Claude Sonnet 5 --- .../contracts/invoice/src/lib.rs | 90 ++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/COMEBACKHERE-contracts/contracts/invoice/src/lib.rs b/COMEBACKHERE-contracts/contracts/invoice/src/lib.rs index 1ce090e..908d9f7 100644 --- a/COMEBACKHERE-contracts/contracts/invoice/src/lib.rs +++ b/COMEBACKHERE-contracts/contracts/invoice/src/lib.rs @@ -24,6 +24,13 @@ pub enum ContractError { DuplicateNonce = 13, TreasuryNotConfigured = 14, NotAParty = 15, + Overflow = 16, + AddressBlocked = 17, + /// A state-changing call was rejected because the invoice is in a terminal + /// or refund-related state that does not permit the requested transition + /// (e.g. `mark_paids` called on an invoice that is `RefundRequested`, + /// `Released`, `Cancelled`, or `Expired`). + InvalidStateTransition = 18, } #[contracttype] @@ -239,7 +246,10 @@ impl InvoiceContract { /// # Errors /// - [`ContractError::ContractPaused`] if the contract is currently paused. /// - [`ContractError::InvoiceNotFound`] if any ID in the batch does not exist. - /// - [`ContractError::InvoiceAlreadyPaid`] if any invoice is not in `Pending` status. + /// - [`ContractError::InvalidStateTransition`] if any invoice is `RefundRequested`, + /// `Released`, `Cancelled`, or `Expired` — a payment confirmation must never + /// silently override a refund already in progress or a closed invoice. + /// - [`ContractError::InvoiceAlreadyPaid`] if any invoice is already `Paid`. /// - [`ContractError::InvoiceExpired`] if any invoice's `expires_at` has passed. /// /// # Events @@ -260,6 +270,20 @@ impl InvoiceContract { .persistent() .get::(&DataKey::Invoice(id)) .ok_or(ContractError::InvoiceNotFound)?; + // Terminal and refund-related states must never be silently + // overridden by a stale payment confirmation: a payer's refund + // request (or an already-settled/cancelled/expired invoice) is + // rejected with a distinct error rather than falling through to + // the generic "already paid" case below. + if matches!( + invoice.status, + InvoiceStatus::RefundRequested + | InvoiceStatus::Released + | InvoiceStatus::Cancelled + | InvoiceStatus::Expired + ) { + return Err(ContractError::InvalidStateTransition); + } if invoice.status != InvoiceStatus::Pending { return Err(ContractError::InvoiceAlreadyPaid); } @@ -1097,4 +1121,68 @@ mod tests { let res = client.try_cancel_invoiced(&id, &merchant); assert_eq!(res, Err(Ok(ContractError::ContractPaused))); } + + // ── mark_paids terminal/refund-state guard tests ───────────────────────── + + /// A stale mark_paids call must not silently override a refund already + /// requested by the customer — it should be rejected, not re-marked Paid. + #[test] + fn test_mark_paids_on_refund_requested_returns_invalid_state_transition() { + let (env, cid, _admin) = setup_contract(1000); + let client = InvoiceContractClient::new(&env, &cid); + let (_merchant, customer, id) = create_test_invoice(&client, &env); + + client.mark_paids(&soroban_sdk::vec![&env, id]); + client.request_refund(&id, &customer); + + let res = client.try_mark_paids(&soroban_sdk::vec![&env, id]); + assert_eq!(res, Err(Ok(ContractError::InvalidStateTransition))); + + // The refund request must survive the stale confirmation untouched. + let invoice = client.get_invoice(&id); + assert_eq!(invoice.status, InvoiceStatus::RefundRequested); + } + + /// mark_paids on a Released (escrow already released) invoice is rejected. + #[test] + fn test_mark_paids_on_released_returns_invalid_state_transition() { + let (env, cid, admin) = setup_contract(1000); + let client = InvoiceContractClient::new(&env, &cid); + let (merchant, customer, id) = create_test_invoice(&client, &env); + + client.mark_paids(&soroban_sdk::vec![&env, id]); + client.request_refund(&id, &customer); + client.set_grace_window(&admin, &0u64); + client.release_escrow(&id, &merchant); + + let res = client.try_mark_paids(&soroban_sdk::vec![&env, id]); + assert_eq!(res, Err(Ok(ContractError::InvalidStateTransition))); + } + + /// mark_paids on a Cancelled invoice is rejected with the same distinct error. + #[test] + fn test_mark_paids_on_cancelled_returns_invalid_state_transition() { + let (env, cid, _admin) = setup_contract(1000); + let client = InvoiceContractClient::new(&env, &cid); + let (merchant, _customer, id) = create_test_invoice(&client, &env); + + client.cancel_invoiced(&id, &merchant); + + let res = client.try_mark_paids(&soroban_sdk::vec![&env, id]); + assert_eq!(res, Err(Ok(ContractError::InvalidStateTransition))); + } + + /// mark_paids on an already-Paid invoice still returns the more specific + /// InvoiceAlreadyPaid error, distinct from the terminal/refund-state guard. + #[test] + fn test_mark_paids_on_already_paid_returns_invoice_already_paid() { + let (env, cid, _admin) = setup_contract(1000); + let client = InvoiceContractClient::new(&env, &cid); + let (_merchant, _customer, id) = create_test_invoice(&client, &env); + + client.mark_paids(&soroban_sdk::vec![&env, id]); + + let res = client.try_mark_paids(&soroban_sdk::vec![&env, id]); + assert_eq!(res, Err(Ok(ContractError::InvoiceAlreadyPaid))); + } } From 1a307c7494106ec21a976b30ea138e998b3d537d Mon Sep 17 00:00:00 2001 From: rdj-savyy Date: Thu, 27 Aug 2026 23:25:29 +0000 Subject: [PATCH 2/4] docs(invoice): add state-transition diagram and fill error-code gaps Individual InvoiceStatus values were documented in isolation, but no single diagram showed which transitions between them are legal, making it hard to tell which InvalidStateTransition/NotPending cases in docs/error-codes.md are expected versus a real bug. Add a Mermaid state diagram to ARCHITECTURE.md covering every state and the function that triggers each transition, with notes on the deliberately-absent edges (e.g. Paid is never re-entered). Cross-link it from docs/error-codes.md in both directions, and fill in the previously-undocumented ContractError (invoice) codes 15-18 (NotAParty, Overflow, AddressBlocked, InvalidStateTransition) that had drifted out of sync with the enum in lib.rs. Co-Authored-By: Claude Sonnet 5 --- ARCHITECTURE.md | 28 ++++++++++++++++++++++++++++ docs/error-codes.md | 6 ++++++ 2 files changed, 34 insertions(+) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index f8a7413..c4dae3a 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -64,6 +64,34 @@ A working clone for end-to-end development looks like this (see `docs/dev-enviro When working inside this repository alone, the in-tree `COMEBACKHERE-contracts/` checkout acts as the canonical contracts tree; the sibling-clone step is optional for backend/frontend contributors. +## Invoice state machine + +The invoice contract (`COMEBACKHERE-contracts/contracts/invoice/src/lib.rs`, mirrored at `contracts/invoice/src/lib.rs`) is the source of truth every other layer reads from: backend status displays, the indexer, and both frontend apps all ultimately derive their view of an invoice from this state machine. The diagram below shows every `InvoiceStatus` value and the function whose call transitions an invoice into it. Any transition not shown here is illegal and the calling function returns an `InvalidStateTransition` / `NotPending` style error (see [docs/error-codes.md](docs/error-codes.md) for the exact variant and code per contract). + +```mermaid +stateDiagram-v2 + [*] --> Pending: create_invoice + + Pending --> Paid: mark_paids / pay_invoice + Pending --> Expired: batch_expire + Pending --> Cancelled: cancel_invoiced / cancel_invoice + + Paid --> RefundRequested: request_refund + Paid --> RefundRequested: cancel_invoiced / cancel_invoice + + RefundRequested --> Released: release_escrow (after grace window) + + Expired --> [*] + Cancelled --> [*] + Released --> [*] +``` + +Notes on edges that are deliberately absent from this diagram: + +- **`Paid` is never re-entered.** Once an invoice leaves `Pending`, no function transitions it back to `Paid`. In particular, `mark_paids` guards against being called on an invoice that is `RefundRequested`, `Released`, `Cancelled`, or `Expired`, so a stale or replayed payment confirmation can never silently override a refund already in progress. +- **`RefundRequested`, `Released`, `Cancelled`, and `Expired` are terminal with respect to payment and cancellation** — `cancel_invoiced`, `request_refund`, and `mark_paids` all reject calls made once an invoice has reached one of these states. +- **`release_escrow` is time-gated**, not just state-gated: it additionally requires `ledger.timestamp() >= invoice.created_at + grace_window`. + ## Further reading - [docs/dev-environment.md](docs/dev-environment.md) — full local setup. diff --git a/docs/error-codes.md b/docs/error-codes.md index 5de9b22..457557b 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -3,6 +3,8 @@ This document maps every `InvoiceError`, `ContractError`, `SettlementError`, and `TreasuryError` variant (and other contract error codes) to its numeric value, the condition that triggers it, and the recommended remediation steps for integrators. > Cross-reference: see [docs/api-reference.md](./api-reference.md) for HTTP-level error shapes returned by the backend. +> +> Cross-reference: see [ARCHITECTURE.md § Invoice state machine](../ARCHITECTURE.md#invoice-state-machine) for a diagram of every legal `InvoiceStatus` transition and the function that triggers it — useful context for knowing which `InvalidStateTransition` / `NotPending` cases below are expected versus a real bug. --- @@ -47,6 +49,10 @@ Defined in `COMEBACKHERE-contracts/contracts/invoice/src/lib.rs`. Shares some va | 12 | `GraceWindowNotExpired` | `release_escrow` was called before `created_at + grace_window`. | Wait until `ledger.timestamp() >= created_at + grace_window`. Admin may reduce `GraceWindow` via `set_grace_window` (default 86 400 seconds). | | 13 | `DuplicateNonce` | The (merchant, nonce) pair has already been used by a previous invoice. | Generate a fresh nonce for each invoice. Different merchants may reuse the same nonce value without collision. | | 14 | `TreasuryNotConfigured` | `raise_dispute` was called before the admin ran `set_treasury`. | Admin must call `set_treasury` once before disputes can be raised. | +| 15 | `NotAParty` | `raise_dispute` was called by an address that is neither the invoice's merchant nor its customer. | Sign with the merchant or customer key associated with the invoice. | +| 16 | `Overflow` | An internal counter (invoice ID, or `created_at + grace_window`) would overflow `u64`. | Practically unreachable outside of adversarial ledger state; not user-actionable. | +| 17 | `AddressBlocked` | `mark_paids` was called for a customer that the configured compliance contract reports as not allowed. | Confirm the customer's compliance status with `ComplianceContract.is_allowed` before retrying. | +| 18 | `InvalidStateTransition` | `mark_paids` was called on an invoice in `RefundRequested`, `Released`, `Cancelled`, or `Expired` status — see [ARCHITECTURE.md § Invoice state machine](../ARCHITECTURE.md#invoice-state-machine) for the full legal-transition diagram. | Fetch the current status with `get_invoice_status` first. A refund already in progress must not be overridden by a stale payment confirmation. | --- From a5eb7387d22cdd2a91f439af4dfb4edb4f6a4dd6 Mon Sep 17 00:00:00 2001 From: rdj-savyy Date: Thu, 27 Aug 2026 23:31:27 +0000 Subject: [PATCH 3/4] test(treasury): cover rotate_signer / total_signer_weight interaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rotate_signer did not exist, and the total_signer_weight helper it needs (used by update_threshold's cap check) was only ever called, not defined — a botched merge of two prior feature branches had dropped its body along with the SignerList DataKey variant and the ThresholdExceedsWeight/InvalidPagination error variants those functions rely on, and left a second, duplicate `mod tests` block plus two dangling helper functions referencing an on-chain Dispute type that was never actually merged in. None of this could compile. Restore total_signer_weight/get_total_signer_weight (recomputed live from SignerList on every call, so it can never drift from the individual Signer(address) entries), remove the dead duplicate test module and orphaned dispute-voting helpers, and add rotate_signer: deregisters the old signer and registers the new one atomically so SignerList - and therefore total_signer_weight - stays exactly in sync whether the new weight is higher or lower than the old one. Add tests for both directions: rotating to a higher weight and asserting total_signer_weight updates correctly, and rotating to a lower weight after a signer has already approved a settlement, confirming that in-flight approval_weight is a snapshot and the settlement remains executable even though the signer's live weight (and total_signer_weight) has since dropped. Co-Authored-By: Claude Sonnet 5 --- .../contracts/treasury/src/lib.rs | 419 +++++++++--------- 1 file changed, 203 insertions(+), 216 deletions(-) diff --git a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs b/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs index fe7628d..dec2ffa 100644 --- a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs +++ b/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs @@ -58,6 +58,14 @@ pub enum TreasuryError { DuplicateSigner = 7, InvalidWeightSum = 8, NotSettlementParty = 9, + /// `update_threshold` was called with a threshold above the sum of all + /// registered signer weights. + ThresholdExceedsWeight = 10, + /// `get_pending_settlements` was called with `limit` above `MAX_PAGE_SIZE`. + InvalidPagination = 11, + /// `rotate_signer` was called with an `old_signer` that has no registered + /// weight (i.e. is not a current signer). + SignerNotFound = 12, } /// Storage keys for Treasury contract instance state. @@ -69,6 +77,10 @@ pub enum DataKey { Paused, /// Mapping of signer address to voting weight key. Signer(Address), + /// List of all addresses ever registered as a signer, so total signer + /// weight can be recomputed on demand instead of relying on a cached + /// counter that could drift out of sync. + SignerList, /// Settlement proposal storage key by settlement ID. Settlement(u64), /// Auto-incrementing settlement ID counter key. @@ -169,6 +181,94 @@ impl TreasuryContract { Ok(()) } + /// Rotates a signer's key: deregisters `old_signer` entirely and + /// registers `new_signer` with `new_weight` in its place. Unlike calling + /// `set_signer` twice, this keeps `SignerList` (and therefore + /// `total_signer_weight`) exactly in sync with the active signer set + /// regardless of whether `new_weight` is higher or lower than the weight + /// `old_signer` held — `total_signer_weight` is always recomputed from + /// live storage rather than tracked as a separate running total, so it + /// cannot drift out of sync with the individual signer weights. + /// + /// # Arguments + /// * `e` - Soroban environment handle. + /// * `admin` - Admin address (must authenticate). + /// * `old_signer` - The signer address being replaced; must currently hold non-zero weight. + /// * `new_signer` - The replacement signer address. + /// * `new_weight` - Voting weight assigned to `new_signer`. + /// + /// # Errors + /// * Returns [`TreasuryError::ContractPaused`] if contract operations are paused. + /// * Returns [`TreasuryError::Unauthorized`] if `admin` is not the stored contract admin. + /// * Returns [`TreasuryError::SignerNotFound`] if `old_signer` is not a current signer. + /// * Returns [`TreasuryError::DuplicateSigner`] if `new_signer` is already a distinct + /// active signer. + /// + /// # Note + /// Settlements that already accumulated approval weight from `old_signer` keep that + /// weight as a snapshot on the settlement record — rotating (or reweighting) a signer + /// does not retroactively change `approval_weight` on in-flight settlements, so a + /// settlement that already reached quorum remains executable. + pub fn rotate_signer( + e: Env, + admin: Address, + old_signer: Address, + new_signer: Address, + new_weight: u64, + ) -> Result<(), TreasuryError> { + check_not_paused(&e)?; + Self::check_admin(&e, &admin)?; + + let old_weight: u64 = e + .storage() + .instance() + .get(&DataKey::Signer(old_signer.clone())) + .unwrap_or(0u64); + if old_weight == 0 { + return Err(TreasuryError::SignerNotFound); + } + + if new_signer != old_signer { + let existing_new_weight: u64 = e + .storage() + .instance() + .get(&DataKey::Signer(new_signer.clone())) + .unwrap_or(0u64); + if existing_new_weight > 0 { + return Err(TreasuryError::DuplicateSigner); + } + } + + e.storage() + .instance() + .remove(&DataKey::Signer(old_signer.clone())); + e.storage() + .instance() + .set(&DataKey::Signer(new_signer.clone()), &new_weight); + + let signer_list: Vec
= e + .storage() + .instance() + .get(&DataKey::SignerList) + .unwrap_or_else(|| Vec::new(&e)); + let mut updated_list: Vec
= Vec::new(&e); + for s in signer_list.iter() { + if s != old_signer { + updated_list.push_back(s); + } + } + if !updated_list.contains(&new_signer) { + updated_list.push_back(new_signer.clone()); + } + e.storage().instance().set(&DataKey::SignerList, &updated_list); + + e.events().publish( + (Symbol::new(&e, "signer_rotated"),), + (old_signer, new_signer, old_weight, new_weight), + ); + Ok(()) + } + /// Proposes a new settlement for approval and execution. /// /// # Arguments @@ -454,6 +554,31 @@ impl TreasuryContract { Ok(()) } + /// Returns the sum of all currently registered signer weights, recomputed + /// live from storage on every call (not a cached counter), so it can + /// never drift out of sync with the individual `Signer(address)` entries. + pub fn get_total_signer_weight(e: Env) -> u64 { + Self::total_signer_weight(&e) + } + + fn total_signer_weight(e: &Env) -> u64 { + let signer_list: Vec
= e + .storage() + .instance() + .get(&DataKey::SignerList) + .unwrap_or_else(|| Vec::new(e)); + let mut total: u64 = 0; + for signer in signer_list.iter() { + let weight: u64 = e + .storage() + .instance() + .get(&DataKey::Signer(signer)) + .unwrap_or(0u64); + total += weight; + } + total + } + /// Places a pending settlement on hold by raising a dispute. /// /// # Arguments @@ -658,41 +783,6 @@ impl TreasuryContract { .get(&DataKey::Settlement(settlement_id)) .unwrap() } - - fn get_dispute_internal(e: &Env, settlement_id: u64) -> Dispute { - e.storage() - .instance() - .get(&DataKey::Dispute(settlement_id)) - .unwrap_or_else(|| panic_with_error!(e, TreasuryError::DisputeNotFound)) - } - - fn finalize_dispute_internal(e: &Env, settlement_id: u64, resolve_in_favor: bool) { - let mut dispute: Dispute = e - .storage() - .instance() - .get(&DataKey::Dispute(settlement_id)) - .unwrap_or_else(|| panic_with_error!(e, TreasuryError::DisputeNotFound)); - - dispute.status = if resolve_in_favor { - DisputeStatus::ResolvedClaimant - } else { - DisputeStatus::ResolvedCounterparty - }; - e.storage().instance().set(&DataKey::Dispute(settlement_id), &dispute); - - // In favour of the claimant (the dispute raiser): the settlement is voided. - // In favour of the counterparty (the merchant): the settlement resumes as - // Pending and can proceed through the normal approval/execution flow. - let mut settlement = Self::get_settlement_internal(e, settlement_id); - settlement.status = if resolve_in_favor { - SettlementStatus::Cancelled - } else { - SettlementStatus::Pending - }; - e.storage().instance().set(&DataKey::Settlement(settlement_id), &settlement); - - events::dispute_resolved(e, &settlement_id, &resolve_in_favor, &dispute.resolution_weight); - } } #[cfg(test)] @@ -1142,205 +1232,102 @@ mod tests { assert_eq!(c.try_update_threshold(&non_admin, &2u32), Err(Ok(TreasuryError::Unauthorized))); assert_eq!(c.try_withdraw(&non_admin, &non_admin, &100u64), Err(Ok(TreasuryError::Unauthorized))); } -} -#[cfg(test)] -mod tests { - use super::*; - use soroban_sdk::testutils::Address as _; - use soroban_sdk::{vec, Env}; - - struct TestContext { - env: Env, - contract_id: Address, - signer1: Address, - signer2: Address, - signer3: Address, - token: Address, - merchant: Address, - } - - fn setup() -> TestContext { - let env = Env::default(); - env.mock_all_auths(); - - let admin = Address::generate(&env); - let signer1 = Address::generate(&env); - let signer2 = Address::generate(&env); - let signer3 = Address::generate(&env); - let token = Address::generate(&env); - let merchant = Address::generate(&env); - - let signers = vec![ - &env, - (signer1.clone(), 1u64), - (signer2.clone(), 1u64), - (signer3.clone(), 1u64), - ]; - - let contract_id = env.register_contract(None, TreasuryContract); - let client = TreasuryContractClient::new(&env, &contract_id); - client.initialize(&signers, &2u64, &admin); - - TestContext { - env, - contract_id, - signer1, - signer2, - signer3, - token, - merchant, - } - } - - fn propose_and_raise(ctx: &TestContext) -> u64 { - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - let settlement_id = client.propose_settlement(&ctx.signer1, &ctx.token, &1000u64, &ctx.merchant); - client.raise_dispute(&ctx.signer1, &settlement_id, &1u32); - settlement_id - } - - fn read_dispute(ctx: &TestContext, settlement_id: u64) -> Dispute { - ctx.env.as_contract(&ctx.contract_id, || { - ctx.env - .storage() - .instance() - .get(&DataKey::Dispute(settlement_id)) - .unwrap() - }) - } - - fn read_settlement(ctx: &TestContext, settlement_id: u64) -> Settlement { - ctx.env.as_contract(&ctx.contract_id, || { - ctx.env - .storage() - .instance() - .get(&DataKey::Settlement(settlement_id)) - .unwrap() - }) - } - - #[test] - fn test_raise_dispute_holds_settlement_and_records_dispute() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let dispute = read_dispute(&ctx, settlement_id); - assert_eq!(dispute.status, DisputeStatus::Raised); - assert_eq!(dispute.resolution_weight, 0u64); - assert_eq!(dispute.raised_by, ctx.signer1); - assert_eq!(dispute.reason, 1u32); - assert!(dispute.voters.is_empty()); - - let settlement = read_settlement(&ctx, settlement_id); - assert!(matches!(settlement.status, SettlementStatus::OnHold)); - } - - #[test] - fn test_raise_dispute_twice_fails() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - let result = client.try_raise_dispute(&ctx.signer2, &settlement_id, &2u32); - assert_eq!(result, Err(Ok(TreasuryError::DisputeAlreadyRaised))); - } - - #[test] - fn test_votes_resolve_dispute_in_favour_of_claimant() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - - // First vote: weight 1 < threshold 2, dispute stays Raised. - client.vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); - let dispute = read_dispute(&ctx, settlement_id); - assert_eq!(dispute.status, DisputeStatus::Raised); - assert_eq!(dispute.resolution_weight, 1u64); - - // Second vote reaches the threshold and resolves in favour of the claimant. - client.vote_dispute_resolution(&ctx.signer2, &settlement_id, &true); - let dispute = read_dispute(&ctx, settlement_id); - assert_eq!(dispute.status, DisputeStatus::ResolvedClaimant); - assert_eq!(dispute.resolution_weight, 2u64); - - let settlement = read_settlement(&ctx, settlement_id); - assert!(matches!(settlement.status, SettlementStatus::Cancelled)); - } + // ── rotate_signer tests ────────────────────────────────────────────────── + /// Rotating a signer to a HIGHER weight must increase total_signer_weight + /// by exactly the delta, not leave the old weight double-counted or lost. #[test] - fn test_votes_resolve_dispute_in_favour_of_counterparty() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - client.vote_dispute_resolution(&ctx.signer1, &settlement_id, &false); - client.vote_dispute_resolution(&ctx.signer2, &settlement_id, &false); - - let dispute = read_dispute(&ctx, settlement_id); - assert_eq!(dispute.status, DisputeStatus::ResolvedCounterparty); - - let settlement = read_settlement(&ctx, settlement_id); - assert!(matches!(settlement.status, SettlementStatus::Pending)); - } - - #[test] - fn test_signer_cannot_vote_twice() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); + fn test_rotate_signer_to_higher_weight_updates_total_signer_weight() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let s1 = soroban_sdk::Address::generate(&e); + let s2 = soroban_sdk::Address::generate(&e); + let new_s1 = soroban_sdk::Address::generate(&e); + // total weight = 3 (s1=1, s2=2) + c.initialize( + &soroban_sdk::vec![&e, (s1.clone(), 1u64), (s2.clone(), 2u64)], + &1, + &admin, + ); + assert_eq!(c.get_total_signer_weight(), 3u64); - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - client.vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); + // Rotate s1 (weight 1) -> new_s1 (weight 5): total should become 2 + 5 = 7. + c.rotate_signer(&admin, &s1, &new_s1, &5u64); + assert_eq!(c.get_total_signer_weight(), 7u64); - let result = client.try_vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); - assert_eq!(result, Err(Ok(TreasuryError::AlreadyVoted))); + // The old signer address must no longer carry any weight. + let res = c.try_rotate_signer(&admin, &s1, &new_s1, &1u64); + assert_eq!(res, Err(Ok(TreasuryError::SignerNotFound))); } + /// Rotating a signer to a LOWER weight updates total_signer_weight, but + /// must not retroactively shrink approval_weight already accumulated on + /// an in-flight settlement — a settlement that already reached quorum + /// stays executable even after the approving signer's weight drops. #[test] - fn test_non_signer_cannot_vote() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - let outsider = Address::generate(&ctx.env); + fn test_rotate_signer_to_lower_weight_preserves_inflight_quorum() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let s1 = soroban_sdk::Address::generate(&e); + let s2 = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); + let merchant = soroban_sdk::Address::generate(&e); + let new_s1 = soroban_sdk::Address::generate(&e); + // threshold=3, s1 weight=3 (alone meets threshold), s2 weight=1 + c.initialize( + &soroban_sdk::vec![&e, (s1.clone(), 3u64), (s2.clone(), 1u64)], + &3, + &admin, + ); + assert_eq!(c.get_total_signer_weight(), 4u64); - let result = client.try_vote_dispute_resolution(&outsider, &settlement_id, &true); - assert_eq!(result, Err(Ok(TreasuryError::UnauthorizedSigner))); - } + let sid = c.propose_settlement(&s1, &token, &500u64, &merchant); + c.approve_settlement(&s1, &sid); - #[test] - fn test_vote_without_dispute_fails() { - let ctx = setup(); - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - let settlement_id = client.propose_settlement(&ctx.signer1, &ctx.token, &1000u64, &ctx.merchant); + // Rotate s1 down to weight 1 *after* it already approved. + c.rotate_signer(&admin, &s1, &new_s1, &1u64); + assert_eq!(c.get_total_signer_weight(), 2u64, "total weight must reflect the new, lower weight"); - let result = client.try_vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); - assert_eq!(result, Err(Ok(TreasuryError::DisputeNotFound))); + // The settlement's already-captured approval_weight (3) is a snapshot + // and is unaffected by the later rotation, so it still clears the + // threshold (3) and executes successfully. + c.execute_settlement(&s1, &sid, &token); + let settlement = c.get_settlement(&sid).unwrap(); + assert!(matches!(settlement.status, SettlementStatus::Executed)); } #[test] - fn test_resolve_before_threshold_fails() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - client.vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); + fn test_rotate_signer_requires_admin() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let non_admin = soroban_sdk::Address::generate(&e); + let s1 = soroban_sdk::Address::generate(&e); + let new_s1 = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e, (s1.clone(), 1u64)], &1, &admin); - let result = client.try_resolve_dispute(&ctx.signer2, &settlement_id, &true); - assert_eq!(result, Err(Ok(TreasuryError::ThresholdNotMet))); + let res = c.try_rotate_signer(&non_admin, &s1, &new_s1, &1u64); + assert_eq!(res, Err(Ok(TreasuryError::Unauthorized))); } #[test] - fn test_vote_after_resolution_fails() { - let ctx = setup(); - let settlement_id = propose_and_raise(&ctx); - - let client = TreasuryContractClient::new(&ctx.env, &ctx.contract_id); - client.vote_dispute_resolution(&ctx.signer1, &settlement_id, &true); - client.vote_dispute_resolution(&ctx.signer2, &settlement_id, &true); + fn test_rotate_signer_rejects_duplicate_new_signer() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let s1 = soroban_sdk::Address::generate(&e); + let s2 = soroban_sdk::Address::generate(&e); + c.initialize( + &soroban_sdk::vec![&e, (s1.clone(), 1u64), (s2.clone(), 2u64)], + &1, + &admin, + ); - let result = client.try_vote_dispute_resolution(&ctx.signer3, &settlement_id, &false); - assert_eq!(result, Err(Ok(TreasuryError::DisputeNotRaised))); + // s2 is already an active signer; rotating s1 onto it must be rejected. + let res = c.try_rotate_signer(&admin, &s1, &s2, &5u64); + assert_eq!(res, Err(Ok(TreasuryError::DuplicateSigner))); } } From fa6dcb6cd057cb931d66e6158578cd35a4f14c40 Mon Sep 17 00:00:00 2001 From: rdj-savyy Date: Thu, 27 Aug 2026 23:31:50 +0000 Subject: [PATCH 4/4] feat(treasury): add per-token daily withdrawal limit withdraw had no rate limiting beyond the admin/pause checks, so a compromised signer set that reached quorum could drain the treasury of a given token in a single settlement cycle with no configurable ceiling on worst-case exposure. Add an admin-only set_daily_withdraw_limit(token, limit), enforced by withdraw (now token-scoped) via a per-token WithdrawWindow record tracking cumulative withdrawals against a rolling 24h ledger-time window: the window resets once ledger.timestamp() has advanced a full 86400s past its start, and any withdrawal that would push the current window's cumulative total above the configured limit is rejected with the new TreasuryError::DailyLimitExceeded. A token with no configured limit remains unrestricted, preserving existing behavior. Co-Authored-By: Claude Sonnet 5 --- .../contracts/treasury/src/lib.rs | 207 +++++++++++++++++- 1 file changed, 203 insertions(+), 4 deletions(-) diff --git a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs b/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs index dec2ffa..2a2b503 100644 --- a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs +++ b/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs @@ -38,6 +38,17 @@ pub struct Settlement { pub proposer: Address, } +/// Tracks cumulative withdrawals of one token within the current rolling +/// 24h ledger-time window, used to enforce `daily_withdraw_limit`. +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct WithdrawWindow { + /// Ledger timestamp (seconds) at which the current window started. + pub window_start: u64, + /// Cumulative amount withdrawn since `window_start`. + pub spent: u64, +} + /// Error types returned by Treasury contract operations. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] @@ -66,6 +77,10 @@ pub enum TreasuryError { /// `rotate_signer` was called with an `old_signer` that has no registered /// weight (i.e. is not a current signer). SignerNotFound = 12, + /// `withdraw` was called for a token with a configured + /// `daily_withdraw_limit` and the withdrawal would push cumulative + /// withdrawals for the current 24h window above that limit. + DailyLimitExceeded = 13, } /// Storage keys for Treasury contract instance state. @@ -89,6 +104,10 @@ pub enum DataKey { Threshold, /// Token allowlist key. TokenAllowlist, + /// Per-token admin-configured daily withdrawal cap. + DailyWithdrawLimit(Address), + /// Per-token rolling-window withdrawal ledger; value is a [`WithdrawWindow`]. + WithdrawWindow(Address), } fn is_paused(e: &Env) -> bool { @@ -690,25 +709,117 @@ impl TreasuryContract { Ok(()) } + /// Sets (or clears, with `limit = 0`) the admin-configured daily withdrawal + /// cap for a token. `withdraw` enforces this cap over a rolling 24h + /// ledger-time window per token; a token with no configured limit is + /// unrestricted. + /// + /// # Arguments + /// * `e` - Soroban environment handle. + /// * `admin` - Admin address (must authenticate). + /// * `token` - Token contract address the cap applies to. + /// * `limit` - Maximum cumulative withdrawal amount per 24h window. + /// + /// # Errors + /// * Returns [`TreasuryError::ContractPaused`] if contract is paused. + /// * Returns [`TreasuryError::Unauthorized`] if caller is not the contract admin. + pub fn set_daily_withdraw_limit( + e: Env, + admin: Address, + token: Address, + limit: u64, + ) -> Result<(), TreasuryError> { + check_not_paused(&e)?; + Self::check_admin(&e, &admin)?; + e.storage() + .instance() + .set(&DataKey::DailyWithdrawLimit(token.clone()), &limit); + e.events().publish( + (Symbol::new(&e, "daily_withdraw_limit_set"),), + (token, limit), + ); + Ok(()) + } + + /// Returns the configured daily withdrawal cap for a token, or `None` if + /// the admin has never set one (in which case withdrawals of that token + /// are unrestricted). + pub fn get_daily_withdraw_limit(e: Env, token: Address) -> Option { + e.storage() + .instance() + .get(&DataKey::DailyWithdrawLimit(token)) + } + /// Withdraws funds from the treasury to a recipient address. /// + /// If a `daily_withdraw_limit` has been configured for `token`, this + /// tracks cumulative withdrawals of that token in a 24h ledger-time + /// window (measured from the first withdrawal in the window; the window + /// resets once `ledger.timestamp()` has advanced 24h past its start) and + /// rejects any withdrawal that would push the window's cumulative total + /// above the limit. This bounds worst-case exposure from a compromised + /// signer set to one configured cap per settlement cycle, rather than + /// allowing the treasury to be drained in a single transaction. + /// /// # Arguments /// * `e` - Soroban environment handle. /// * `admin` - Admin address (must authenticate). + /// * `token` - Token being withdrawn; used to look up the daily cap. /// * `_to` - Target recipient address. - /// * `_amount` - Amount to withdraw. + /// * `amount` - Amount to withdraw. /// /// # Errors /// * Returns [`TreasuryError::ContractPaused`] if contract is paused. /// * Returns [`TreasuryError::Unauthorized`] if caller is not the contract admin. + /// * Returns [`TreasuryError::DailyLimitExceeded`] if `token` has a configured + /// daily limit and `amount` would push the current 24h window's cumulative + /// withdrawals above it. pub fn withdraw( e: Env, admin: Address, + token: Address, _to: Address, - _amount: u64, + amount: u64, ) -> Result<(), TreasuryError> { check_not_paused(&e)?; Self::check_admin(&e, &admin)?; + + const WINDOW_SECONDS: u64 = 86_400; + + let limit: Option = e + .storage() + .instance() + .get(&DataKey::DailyWithdrawLimit(token.clone())); + + if let Some(limit) = limit { + let now = e.ledger().timestamp(); + let window: Option = e + .storage() + .instance() + .get(&DataKey::WithdrawWindow(token.clone())); + let (window_start, spent) = match window { + Some(w) if now.saturating_sub(w.window_start) < WINDOW_SECONDS => { + (w.window_start, w.spent) + } + _ => (now, 0u64), + }; + + let new_spent = spent + .checked_add(amount) + .ok_or(TreasuryError::DailyLimitExceeded)?; + if new_spent > limit { + return Err(TreasuryError::DailyLimitExceeded); + } + + e.storage().instance().set( + &DataKey::WithdrawWindow(token.clone()), + &WithdrawWindow { + window_start, + spent: new_spent, + }, + ); + } + Ok(()) } @@ -1213,10 +1324,11 @@ mod tests { let c = client(&e, &id); let admin = soroban_sdk::Address::generate(&e); let user = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); c.initialize(&soroban_sdk::vec![&e], &1, &admin); c.deposit(&user, &1000u64); - c.withdraw(&admin, &user, &500u64); + c.withdraw(&admin, &token, &user, &500u64); } #[test] @@ -1225,12 +1337,16 @@ mod tests { let c = client(&e, &id); let admin = soroban_sdk::Address::generate(&e); let non_admin = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); c.initialize(&soroban_sdk::vec![&e], &1, &admin); assert_eq!(c.try_pause(&non_admin), Err(Ok(TreasuryError::Unauthorized))); assert_eq!(c.try_unpause(&non_admin), Err(Ok(TreasuryError::Unauthorized))); assert_eq!(c.try_update_threshold(&non_admin, &2u32), Err(Ok(TreasuryError::Unauthorized))); - assert_eq!(c.try_withdraw(&non_admin, &non_admin, &100u64), Err(Ok(TreasuryError::Unauthorized))); + assert_eq!( + c.try_withdraw(&non_admin, &token, &non_admin, &100u64), + Err(Ok(TreasuryError::Unauthorized)) + ); } // ── rotate_signer tests ────────────────────────────────────────────────── @@ -1330,4 +1446,87 @@ mod tests { let res = c.try_rotate_signer(&admin, &s1, &s2, &5u64); assert_eq!(res, Err(Ok(TreasuryError::DuplicateSigner))); } + + // ── daily withdrawal limit tests ───────────────────────────────────────── + + #[test] + fn test_withdraw_within_daily_limit_succeeds() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let user = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e], &1, &admin); + + c.set_daily_withdraw_limit(&admin, &token, &1_000u64); + c.withdraw(&admin, &token, &user, &600u64); + c.withdraw(&admin, &token, &user, &400u64); + } + + #[test] + fn test_withdraw_exceeding_daily_limit_rejected() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let user = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e], &1, &admin); + + c.set_daily_withdraw_limit(&admin, &token, &1_000u64); + c.withdraw(&admin, &token, &user, &600u64); + + let res = c.try_withdraw(&admin, &token, &user, &500u64); + assert_eq!(res, Err(Ok(TreasuryError::DailyLimitExceeded))); + } + + #[test] + fn test_withdraw_limit_is_per_token() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let user = soroban_sdk::Address::generate(&e); + let token_a = soroban_sdk::Address::generate(&e); + let token_b = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e], &1, &admin); + + c.set_daily_withdraw_limit(&admin, &token_a, &1_000u64); + c.withdraw(&admin, &token_a, &user, &1_000u64); + + // token_b has no configured limit, so it is unrestricted even though + // token_a's window is exhausted. + c.withdraw(&admin, &token_b, &user, &1_000_000u64); + } + + #[test] + fn test_withdraw_window_resets_after_24h() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let user = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e], &1, &admin); + + c.set_daily_withdraw_limit(&admin, &token, &1_000u64); + c.withdraw(&admin, &token, &user, &1_000u64); + assert_eq!( + c.try_withdraw(&admin, &token, &user, &1u64), + Err(Ok(TreasuryError::DailyLimitExceeded)) + ); + + e.ledger().with_mut(|li| li.timestamp += 86_400); + // A full window has elapsed, so the cap applies fresh. + c.withdraw(&admin, &token, &user, &1_000u64); + } + + #[test] + fn test_withdraw_without_configured_limit_is_unrestricted() { + let (e, id) = setup(); + let c = client(&e, &id); + let admin = soroban_sdk::Address::generate(&e); + let user = soroban_sdk::Address::generate(&e); + let token = soroban_sdk::Address::generate(&e); + c.initialize(&soroban_sdk::vec![&e], &1, &admin); + + c.withdraw(&admin, &token, &user, &1_000_000_000u64); + } }