From 8c8a9e9eb79b8e0eaa6d974bc7369e5f8df6c350 Mon Sep 17 00:00:00 2001 From: ZuLu0890 Date: Tue, 18 Aug 2026 19:52:53 +0000 Subject: [PATCH] fix(contracts): cap fee_bps with a MAX_FEE_BPS ceiling; repair main CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add MAX_FEE_BPS = 1000 (10%) to all three contracts and reject any fee_bps above it at initialize with the existing InvalidFee error, closing the full-fee (100%) configuration that silently zeroed every payout (#40). Add boundary tests locking in the inclusive ceiling and update the README. Rebasing onto current main also surfaced pre-existing CI breakages that this commit repairs so the workspace is green again: - restore the missing closing brace on milestones::sort_remainders_desc, which left the workspace unable to compile; - advance the multi-sponsor refund test past the new grace-period window so the permissionless path is actually exercised; - normalize trailing whitespace so cargo fmt --check passes. 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- README.md | 6 ++-- contracts/escrow/src/lib.rs | 16 ++++++++- contracts/escrow/src/test.rs | 46 +++++++++++++++++++++++--- contracts/maintenance-pool/src/lib.rs | 16 ++++++++- contracts/maintenance-pool/src/test.rs | 36 ++++++++++++++++++++ contracts/milestones/src/lib.rs | 17 +++++++++- contracts/milestones/src/test.rs | 36 ++++++++++++++++++++ 7 files changed, 162 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ea0627a..9d64fcb 100644 --- a/README.md +++ b/README.md @@ -364,9 +364,9 @@ archival, tuned for a multi-month bounty/release lifecycle). that race requires a structural change (an atomic deploy+init constructor) rather than an in-contract check. - **Fee mechanics.** `fee_bps` is basis points (1/100 of a percent) out of - 10000, validated `<= 10000` at `initialize`. It's deducted from the top - of every payout (`release`, `release_issue`, `withdraw`) before the - remainder is split among recipients — the treasury is paid in the same + 10000, validated `<= MAX_FEE_BPS` (1000 = 10%) at `initialize`. It's + deducted from the top of every payout (`release`, `release_issue`, + `withdraw`) before the remainder is split among recipients — the treasury is paid in the same transaction as the recipients, so there's no separate "sweep fees" step that could be skipped. - **Replay / double-spend protection.** Every escrow/milestone-issue diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 56af404..56c5fc1 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -20,6 +20,20 @@ use types::{Contribution, DataKey, Escrow, EscrowStatus}; /// Basis points denominator (100.00%). pub const BPS_DENOMINATOR: i128 = 10_000; +/// Maximum protocol fee accepted at `initialize`, in basis points +/// (1000 = 10%). +/// +/// This is a sanity ceiling, not a target. Bounty-payout platforms charge +/// single-digit-percent treasury fees in practice, so 10% is already an +/// order of magnitude below the mathematical maximum (`BPS_DENOMINATOR` = +/// 10000 = 100%) and any fee near the ceiling is itself a red flag. Capping +/// here also means `fee_bps` can never be set to 100%: at 10000 bps +/// `compute_split` computes `distributable = total - total = 0` and every +/// recipient's share silently becomes zero while the whole escrow goes to +/// the treasury. Values above this ceiling are rejected with the existing +/// `Error::InvalidFee` (no new error variant). See issue #40. +pub const MAX_FEE_BPS: u32 = 1_000; + /// Maximum number of distinct contributions (sponsors) a single escrow can /// accumulate. Bounds the per-contributor loops in `refund` and /// `extend_deadline` to a small, predictable constant regardless of how @@ -58,7 +72,7 @@ impl EscrowContract { if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); } - if fee_bps as i128 > BPS_DENOMINATOR { + if fee_bps > MAX_FEE_BPS { return Err(Error::InvalidFee); } diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index ea57571..00745cb 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -37,6 +37,42 @@ fn test_initialize_rejects_double_init() { assert_eq!(err, Err(Ok(Error::AlreadyInitialized))); } +#[test] +fn test_initialize_rejects_fee_bps_above_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(EscrowContract, ()); + let client = EscrowContractClient::new(&env, &contract_id); + + // One basis point above the sanity ceiling, plus the old mathematical + // maximum (100%): both must now be rejected by `MAX_FEE_BPS`, not just + // the previous `> BPS_DENOMINATOR` guard (which silently accepted 100% + // and let every payout compute to zero). + for fee_bps in [crate::MAX_FEE_BPS + 1, 10_000u32] { + let err = client.try_initialize(&admin, &treasury, &fee_bps); + assert_eq!(err, Err(Ok(Error::InvalidFee))); + } +} + +#[test] +fn test_initialize_accepts_fee_bps_at_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(EscrowContract, ()); + let client = EscrowContractClient::new(&env, &contract_id); + + // Boundary-exact: `MAX_FEE_BPS` is inclusive, so the ceiling itself is + // accepted and stored verbatim. + client.initialize(&admin, &treasury, &crate::MAX_FEE_BPS); + assert_eq!(client.get_fee_bps(), crate::MAX_FEE_BPS); +} + #[test] fn test_fund_and_release_single_recipient() { let env = Env::default(); @@ -600,8 +636,8 @@ fn test_multi_sponsor_refund_returns_exact_contributions_to_each_sponsor() { assert_eq!(escrow.amount, 11_500i128); assert_eq!(escrow.contributor_count, 3); - // Past the deadline: permissionless refund. - env.ledger().set_timestamp(300); + // Past the deadline + grace period: permissionless refund. + env.ledger().set_timestamp(200 + crate::GRACE_PERIOD); env.set_auths(&[]); client.refund(&100u64); @@ -834,7 +870,7 @@ fn test_release_succeeds_in_grace_period() { // Pass the nominal deadline but stay within the grace period. env.ledger().set_timestamp(200 + crate::GRACE_PERIOD - 1); - + // Permissionless refund is still rejected. env.set_auths(&[]); let result = client.try_refund(&200u64); @@ -870,14 +906,14 @@ fn test_release_loses_race_to_refund_at_grace_period_boundary() { env.set_auths(&[]); client.refund(&201u64); assert_eq!(token_client.balance(&sponsor), 10_000_000_000i128); - + // The backend's subsequently-landing release call fails. env.mock_all_auths(); let contributor = Address::generate(&env); let recipients = vec![&env, (contributor.clone(), 10_000u32)]; let err = client.try_release(&201u64, &recipients); assert_eq!(err, Err(Ok(Error::AlreadyRefunded))); - + // The would-be recipient gets nothing. assert_eq!(token_client.balance(&contributor), 0); } diff --git a/contracts/maintenance-pool/src/lib.rs b/contracts/maintenance-pool/src/lib.rs index a23f759..2e56915 100644 --- a/contracts/maintenance-pool/src/lib.rs +++ b/contracts/maintenance-pool/src/lib.rs @@ -20,6 +20,20 @@ use types::{DataKey, Deposit, MaintenancePool}; pub const BPS_DENOMINATOR: i128 = 10_000; +/// Maximum protocol fee accepted at `initialize`, in basis points +/// (1000 = 10%). +/// +/// This is a sanity ceiling, not a target. Recurring-maintenance payout +/// fees are single-digit-percent in practice, so 10% is already an order of +/// magnitude below the mathematical maximum (`BPS_DENOMINATOR` = 10000 = +/// 100%) and any fee near the ceiling is itself a red flag. Capping here +/// also means `fee_bps` can never be set to 100%: at 10000 bps `withdraw` +/// computes `payout = amount - fee = 0` and silently pays the maintainer +/// nothing while the full amount goes to the treasury. Values above this +/// ceiling are rejected with the existing `Error::InvalidFee` (no new error +/// variant). See issue #40. +pub const MAX_FEE_BPS: u32 = 1_000; + #[contract] pub struct MaintenancePoolContract; @@ -40,7 +54,7 @@ impl MaintenancePoolContract { if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); } - if fee_bps as i128 > BPS_DENOMINATOR { + if fee_bps > MAX_FEE_BPS { return Err(Error::InvalidFee); } env.storage().instance().set(&DataKey::Admin, &admin); diff --git a/contracts/maintenance-pool/src/test.rs b/contracts/maintenance-pool/src/test.rs index 6330bb9..5f7c15a 100644 --- a/contracts/maintenance-pool/src/test.rs +++ b/contracts/maintenance-pool/src/test.rs @@ -25,6 +25,42 @@ fn setup(env: &Env) -> (Address, Address, MaintenancePoolContractClient<'_>) { (admin, treasury, client) } +#[test] +fn test_initialize_rejects_fee_bps_above_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(MaintenancePoolContract, ()); + let client = MaintenancePoolContractClient::new(&env, &contract_id); + + // One basis point above the sanity ceiling, plus the old mathematical + // maximum (100%): both must now be rejected by `MAX_FEE_BPS`, not just + // the previous `> BPS_DENOMINATOR` guard (which silently accepted 100% + // and let every withdraw compute to zero). + for fee_bps in [crate::MAX_FEE_BPS + 1, 10_000u32] { + let err = client.try_initialize(&admin, &treasury, &fee_bps); + assert_eq!(err, Err(Ok(Error::InvalidFee))); + } +} + +#[test] +fn test_initialize_accepts_fee_bps_at_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(MaintenancePoolContract, ()); + let client = MaintenancePoolContractClient::new(&env, &contract_id); + + // Boundary-exact: `MAX_FEE_BPS` is inclusive, so the ceiling itself is + // accepted (this contract has no fee getter; a non-panicking initialize + // is the assertion). + client.initialize(&admin, &treasury, &crate::MAX_FEE_BPS); +} + #[test] fn test_deposit_accumulates_balance_and_history() { let env = Env::default(); diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 55bb2d9..421decc 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -23,6 +23,20 @@ use types::{Contribution, DataKey, IssueStatus, Milestone}; pub const BPS_DENOMINATOR: i128 = 10_000; +/// Maximum protocol fee accepted at `initialize`, in basis points +/// (1000 = 10%). +/// +/// This is a sanity ceiling, not a target. Bounty-payout platforms charge +/// single-digit-percent treasury fees in practice, so 10% is already an +/// order of magnitude below the mathematical maximum (`BPS_DENOMINATOR` = +/// 10000 = 100%) and any fee near the ceiling is itself a red flag. Capping +/// here also means `fee_bps` can never be set to 100%: at 10000 bps +/// `compute_split` computes `distributable = total - total = 0` and every +/// recipient's share silently becomes zero while the whole allocation goes +/// to the treasury. Values above this ceiling are rejected with the existing +/// `Error::InvalidFee` (no new error variant). See issue #40. +pub const MAX_FEE_BPS: u32 = 1_000; + /// Maximum number of distinct contributions (sponsors) a single milestone /// can accumulate. Bounds the per-contributor loop in `cancel_milestone` /// (and any future timeout-triggered wind-down that reuses @@ -50,7 +64,7 @@ impl MilestonesContract { if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); } - if fee_bps as i128 > BPS_DENOMINATOR { + if fee_bps > MAX_FEE_BPS { return Err(Error::InvalidFee); } env.storage().instance().set(&DataKey::Admin, &admin); @@ -468,6 +482,7 @@ fn sort_remainders_desc(order: &mut Vec<(u32, i128, Address)>) { order.set(end, a); sift_down_remainder_order(order, 0, end); } +} /// Pays each contributor their share of `milestone.remaining_budget` (the /// unallocated remainder of the pool), computed as /// `remaining_budget * contribution.amount / total_budget` — i.e. in diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index d7c4b7c..0062c94 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -25,6 +25,42 @@ fn setup(env: &Env) -> (Address, Address, MilestonesContractClient<'_>) { (admin, treasury, client) } +#[test] +fn test_initialize_rejects_fee_bps_above_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(MilestonesContract, ()); + let client = MilestonesContractClient::new(&env, &contract_id); + + // One basis point above the sanity ceiling, plus the old mathematical + // maximum (100%): both must now be rejected by `MAX_FEE_BPS`, not just + // the previous `> BPS_DENOMINATOR` guard (which silently accepted 100% + // and let every allocation compute to zero). + for fee_bps in [crate::MAX_FEE_BPS + 1, 10_000u32] { + let err = client.try_initialize(&admin, &treasury, &fee_bps); + assert_eq!(err, Err(Ok(Error::InvalidFee))); + } +} + +#[test] +fn test_initialize_accepts_fee_bps_at_ceiling() { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(MilestonesContract, ()); + let client = MilestonesContractClient::new(&env, &contract_id); + + // Boundary-exact: `MAX_FEE_BPS` is inclusive, so the ceiling itself is + // accepted (this contract has no fee getter; a non-panicking initialize + // is the assertion). + client.initialize(&admin, &treasury, &crate::MAX_FEE_BPS); +} + #[test] fn test_create_milestone_allocate_and_release_per_issue() { let env = Env::default();