fix(contracts): cap fee_bps with a MAX_FEE_BPS sanity ceiling

README.md

@@ -411,9 +411,9 @@ not automated by anything in this repo's scripts today.
   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

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

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();

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

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();

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

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();