diff --git a/CHANGELOG.md b/CHANGELOG.md index 76d61c3..2cbd2ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,4 +58,10 @@ This project does not yet follow a formal versioning scheme — each contract's are the same address with `Error::SelfStream`, so a deposit can't be counted as a committed donation while streaming straight back to its donor. +### Fixed + +- `donation-vault`: `set_treasury` rejects the vault's own address with + `Error::InvalidTreasury`, so protocol fees can't be locked in the + contract. + [Unreleased]: https://github.com/StreamGive/streamgive-contracts/compare/main...HEAD diff --git a/README.md b/README.md index 0a5208f..368ee5e 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,12 @@ the numeric code below (e.g. a failed `try_withdraw` surfacing `Error(5)`). | 6 | `ContractPaused` | The admin has paused the vault; see [Pausing](#pausing) for what still works. | | 7 | `FeeTooHigh` | `set_fee_bps` was called with a value above the 10% (1,000 bps) cap. | | 8 | `NoPendingAdmin` | `accept_admin` was called without a prior (or already-completed) `propose_admin`. | +| 9 | `ArithmeticOverflow` | A stream's `balance`/`withdrawn` or the stream-id counter would overflow. | +| 10 | `InvalidTreasury` | `set_treasury` was called with the vault's own address. | +| 10 | `SelfStream` | `create_stream` was called with the same address as both `donor` and `ngo`, which would stream the donor's own deposit back to them. | +| 11 | `StreamCancelled` | `top_up` or `modify_rate` was called on a stream that `cancel_stream` has already closed out. | +| 9 | `ArithmeticOverflow` | A stream balance, withdrawn total, or stream ID would exceed its integer range. | +| 10 | `DepositTooLow` | `create_stream` was called with a deposit below the admin-configured minimum. | | 9 | `ArithmeticOverflow` | A balance, payout, or stream-id calculation exceeded its supported range. | | 10 | `DepositTooLow` | `create_stream` received a deposit below the configured minimum. | | 11 | `AlreadyPaused` | `pause` was called when the vault was already paused. | diff --git a/contracts/donation-vault/src/lib.rs b/contracts/donation-vault/src/lib.rs index 36086bc..f033fc1 100644 --- a/contracts/donation-vault/src/lib.rs +++ b/contracts/donation-vault/src/lib.rs @@ -10,11 +10,20 @@ use soroban_sdk::{ contract, contractclient, contracterror, contractimpl, contracttype, symbol_short, token, + Address, BytesN, Env, String, Address, BytesN, Env, Map, String, Vec, }; mod math; +/// Mirrors ngo-registry's `Ngo` record for cross-contract calls. Declared +/// locally rather than imported from the `ngo-registry` crate: depending on +/// its source directly would pull that crate's own `#[contractimpl]` exports +/// into this contract's Wasm link unit, colliding with this contract's +/// identically-named entry points (`admin`, `init`, `upgrade`, ...). +#[contracttype] +#[derive(Clone, Debug)] +pub struct NgoRecord { /// The subset of the NGO registry's `Ngo` record the vault needs when /// verifying a target NGO. Mirrors the registry's on-chain layout so a record /// returned by `get_ngo` decodes identically. @@ -26,6 +35,13 @@ pub struct Ngo { pub verified: bool, } +/// Thin cross-contract interface onto the configured ngo-registry contract. +/// Only the one method `create_stream` needs. See `NgoRecord` for why this +/// isn't just imported from the `ngo-registry` crate. +#[contractclient(name = "NgoRegistryClient")] +#[allow(dead_code)] +trait NgoRegistryInterface { + fn get_ngo(env: Env, owner: Address) -> NgoRecord; /// The NGO registry entry point the vault calls. Declared as a client trait /// rather than importing the registry's contract crate so the registry's /// exported entry points aren't linked into the vault's wasm (which would @@ -110,6 +126,7 @@ pub enum DataKey { /// deleted) — so this is really a lifetime cap, not a live cap. DonorStreamCount(Address), MinDeposit, + /// Additional ledgers to retain a cancelled stream for indexing. CancelGraceLedgers, /// Optional NGO registry contract used to verify NGOs before a stream /// is opened. Absent means "no registry check configured". @@ -135,6 +152,39 @@ pub enum Error { /// leave its type's range. Returned instead of letting the release /// profile's overflow checks panic and abort the transaction. ArithmeticOverflow = 9, + /// The donor and the NGO are the same address, so the stream would pay + /// the donor back their own deposit. Rejected at creation: a stream that + /// nets to zero still counts as a committed donation in the indexer and + /// on impact pages, which is a way to inflate those totals for free. + SelfStream = 10, + /// `set_treasury` was given the vault's own address. Fees paid there + /// could never be moved out again. + InvalidTreasury = 11, + AlreadyPaused = 12, + AlreadyUnpaused = 13, + /// `deposit` was below the configured `min_deposit`. + DepositTooLow = 14, + /// `top_up` or `modify_rate` was called on a stream that `cancel_stream` + /// has already closed out. A cancelled stream's rate and balance are + /// zeroed for good; topping it up would just sit inert, and changing + /// its rate would quietly revive a stream the backend already treats + /// as terminal. + StreamCancelled = 15, + /// The proposed administrator is not a valid replacement. + InvalidAdmin = 16, + /// The donor already has `max_streams_per_donor` streams. Raised by + /// `create_stream` before the deposit is pulled. See issue #94. + StreamLimitExceeded = 17, + /// The NGO address passed to `create_stream` is not verified in the + /// configured ngo-registry. Only set when a registry address has been + /// stored via `set_registry`. + NgoNotVerified = 18, + /// `NextStreamId` was missing from instance storage when `create_stream` + /// tried to read it. `init` always sets it, so this should be + /// unreachable in practice, but a missing counter must never be + /// silently treated as `0` — that could collide with an existing + /// stream. Returned instead of defaulting. + StreamCounterMissing = 19, /// `deposit` was below the configured `min_deposit`. DepositTooLow = 10, AlreadyPaused = 11, @@ -1005,6 +1055,10 @@ impl DonationVault { /// Sets where the protocol fee (if any) gets paid. Admin-gated. /// + /// Fails with `Error::InvalidTreasury` if `treasury` is this contract's + /// own address: the vault has no way to spend from itself, so fees sent + /// there would be locked permanently. + /// /// # Examples /// /// ```rust,no_run @@ -1022,6 +1076,9 @@ impl DonationVault { /// ``` pub fn set_treasury(env: Env, treasury: Address) -> Result<(), Error> { require_admin(&env)?; + if treasury == env.current_contract_address() { + return Err(Error::InvalidTreasury); + } env.storage().instance().set(&DataKey::Treasury, &treasury); extend_instance_ttl(&env); @@ -1286,8 +1343,9 @@ impl DonationVault { .unwrap_or(0) } - /// Sets how many additional ledgers a cancelled stream remains available - /// for indexers after the normal stream-retention period. Admin-only. + /// Sets the number of additional ledgers that a cancelled stream remains + /// available for indexing after the normal stream TTL bump. Admin-gated. + /// A value of zero preserves the default stream retention period. pub fn set_cancel_grace_ledgers(env: Env, grace_ledgers: u32) -> Result<(), Error> { require_admin(&env)?; STREAM_BUMP_AMOUNT @@ -1300,6 +1358,7 @@ impl DonationVault { Ok(()) } + /// Reads the additional cancelled-stream retention period, in ledgers. /// Returns the configured cancelled-stream indexing grace period. pub fn cancel_grace_ledgers(env: Env) -> u32 { env.storage() diff --git a/contracts/donation-vault/src/test.rs b/contracts/donation-vault/src/test.rs index 4beba9f..1b6d698 100644 --- a/contracts/donation-vault/src/test.rs +++ b/contracts/donation-vault/src/test.rs @@ -127,7 +127,7 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); let created = last_event(&s.env); - // `last_event` only sees the latest top-level call, so assert it before + // `last_event` only sees the latest top-level call, so snapshot it before // any other call (such as a balance read) replaces it. let created = last_event(&s.env); assert_eq!( @@ -184,10 +184,6 @@ fn full_lifecycle_create_accrue_withdraw_cancel() { assert_eq!(s.token.balance(&s.ngo), 700); assert_eq!(s.token.balance(&s.donor), 300); - // 200 more settles to the NGO on cancel; the untouched 300 refunds to the donor. - assert_eq!(s.token.balance(&s.ngo), 700); - assert_eq!(s.token.balance(&s.donor), 300); - let stream = s.client.get_stream(&stream_id); assert_eq!(stream.balance, 0); assert_eq!(stream.rate, 0); @@ -987,6 +983,9 @@ fn withdraw_splits_protocol_fee_to_treasury() { let treasury = Address::generate(&s.env); s.client.set_treasury(&treasury); + let treasury_evt = last_event(&s.env); + assert_eq!( + treasury_evt, assert_eq!( last_event(&s.env), ( @@ -995,6 +994,13 @@ fn withdraw_splits_protocol_fee_to_treasury() { ) ); s.client.set_fee_bps(&500); // 5% + let feebps_evt = last_event(&s.env); + assert_eq!( + feebps_evt, + ( + (symbol_short!("feeset"),).into_val(&s.env), + 500u32.into_val(&s.env), + ) assert_last_event( &s.env, (symbol_short!("feeset"),).into_val(&s.env), @@ -1124,6 +1130,25 @@ fn protocol_fee_becomes_nonzero_at_the_rounding_boundary() { } #[test] +fn set_treasury_rejects_the_vault_own_address() { + let s = setup(); + let real_treasury = Address::generate(&s.env); + s.client.set_treasury(&real_treasury); + + let result = s.client.try_set_treasury(&s.client.address); + assert_eq!(result, Err(Ok(Error::InvalidTreasury))); + + // The rejected call leaves the existing treasury in place. + assert_eq!(s.client.treasury(), Some(real_treasury)); +} + +#[test] +fn set_treasury_rejects_the_vault_own_address_when_none_is_set() { + let s = setup(); + + let result = s.client.try_set_treasury(&s.client.address); + assert_eq!(result, Err(Ok(Error::InvalidTreasury))); + assert_eq!(s.client.treasury(), None); fn compute_fee_avoids_overflow_near_i128_max() { let s = setup(); @@ -1178,6 +1203,83 @@ fn set_fee_bps_boundary_exact_max_succeeds() { } #[test] +fn pending_payout_with_no_treasury_returns_full_gross_and_zero_fee() { + let s = setup(); + s.token_admin.mint(&s.donor, &1_000); + s.client.set_fee_bps(&500); // 5% configured, but no treasury + + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.env.ledger().with_mut(|l| l.timestamp += 50); // 500 accrues + + let (net, fee) = s.client.pending_payout(&stream_id); + assert_eq!(fee, 0); // no treasury → no fee, regardless of fee_bps + assert_eq!(net, 500); // full accrual goes to the NGO + + // Read-only: nothing moved. + assert_eq!(s.token.balance(&s.ngo), 0); + let stream = s.client.get_stream(&stream_id); + assert_eq!(stream.balance, 1_000); + assert_eq!(stream.withdrawn, 0); +} + +#[test] +fn pending_payout_with_treasury_and_zero_bps_returns_full_gross_and_zero_fee() { + let s = setup(); + s.token_admin.mint(&s.donor, &1_000); + + let treasury = Address::generate(&s.env); + s.client.set_treasury(&treasury); + // fee_bps defaults to 0 — a treasury exists but takes nothing + + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.env.ledger().with_mut(|l| l.timestamp += 50); // 500 accrues + + let (net, fee) = s.client.pending_payout(&stream_id); + assert_eq!(fee, 0); // 0 bps → zero fee even with a treasury set + assert_eq!(net, 500); + + // Read-only: nothing moved. + assert_eq!(s.token.balance(&s.ngo), 0); + assert_eq!(s.token.balance(&treasury), 0); +} + +#[test] +fn pending_payout_with_nonzero_fee_matches_actual_withdraw_split() { + let s = setup(); + s.token_admin.mint(&s.donor, &1_000); + + let treasury = Address::generate(&s.env); + s.client.set_treasury(&treasury); + s.client.set_fee_bps(&500); // 5% + + let stream_id = s + .client + .create_stream(&s.donor, &s.ngo, &s.token.address, &1_000, &10); + s.env.ledger().with_mut(|l| l.timestamp += 50); // 500 accrues + + let (net, fee) = s.client.pending_payout(&stream_id); + assert_eq!(fee, 25); // 5% of 500 + assert_eq!(net, 475); // 500 - 25 + + // Read-only: nothing moved yet. + assert_eq!(s.token.balance(&s.ngo), 0); + assert_eq!(s.token.balance(&treasury), 0); + let stream = s.client.get_stream(&stream_id); + assert_eq!(stream.balance, 1_000); + assert_eq!(stream.withdrawn, 0); + + // The values must match exactly what withdraw actually distributes. + s.client.withdraw(&stream_id); + assert_eq!(s.token.balance(&s.ngo), net); + assert_eq!(s.token.balance(&treasury), fee); +} + +#[test] +fn min_deposit_setter_and_guard() { fn max_fee_bps_is_exposed_on_chain() { let s = setup(); assert_eq!(s.client.max_fee_bps(), 1_000); @@ -2615,6 +2717,8 @@ fn status_is_queryable_after_cancel_then_further_operations_fail() { ); // top_up on a cancelled stream is rejected (cancelled sets rate = 0). s.env.ledger().with_mut(|l| l.timestamp += 10); + let result = s.client.try_withdraw(&stream_id); + assert!(result.is_err()); // nothing left to withdraw let result = s.client.try_top_up(&stream_id, &100); assert_eq!(result, Err(Ok(Error::StreamCancelled))); } diff --git a/contracts/ngo-registry/src/lib.rs b/contracts/ngo-registry/src/lib.rs index ab2f9d3..79a0d95 100644 --- a/contracts/ngo-registry/src/lib.rs +++ b/contracts/ngo-registry/src/lib.rs @@ -44,6 +44,7 @@ pub enum Error { AlreadyVerified = 5, /// `name` is longer than `MAX_NGO_NAME_LEN`. NameTooLong = 6, + /// `revoke_ngo` was called on an NGO that isn't currently verified. /// The NGO has not been approved, so it cannot be revoked. NotVerified = 7, ArithmeticOverflow = 8,