fix(settlement): enforce MAX_FEE_BPS ceiling independent of governance config (#521)

bettapay_common/src/constants.rs

@@ -21,9 +21,12 @@ pub const MIN_FEE_BPS: u32 = 5;
 
 /// Maximum allowed protocol-wide fee, in basis points (50 %).
 ///
-/// Currently enforced only by the governance `FeeConfig`, but defined here so
-/// the settlement contract can adopt the same upper bound without having to
-/// redeclare the constant.
+/// Enforced by both the governance `FeeConfig` and the settlement
+/// `SettlementRule`/default-rule setters, independent of whether a
+/// governance `FeeConfig` has been set yet — settlement's separate
+/// `validate_fee_against_governance` check only tightens the ceiling
+/// further once governance configures one, it never has to be the thing
+/// that first caps a per-fee value.
 pub const MAX_FEE_BPS: u32 = 5_000;
 
 /// Approximate number of Soroban ledgers closed per day, given the 5-second

settlement_contract/src/admin.rs

@@ -2,7 +2,7 @@ use soroban_sdk::xdr::ToXdr;
 use soroban_sdk::{contractimpl, panic_with_error, Address, BytesN, Env, Symbol, Vec};
 
 use bettapay_common::{
-    constants::{BPS_DENOMINATOR, MIN_FEE_BPS, RECOVERY_DELAY_SECONDS},
+    constants::{BPS_DENOMINATOR, MAX_FEE_BPS, MIN_FEE_BPS, RECOVERY_DELAY_SECONDS},
     events::{self, AdminTransferred, PendingRecovery},
     storage::{self, CommonDataKey},
 };
@@ -448,6 +448,9 @@ impl SettlementContract {
             if rule.platform_fee_bps < MIN_FEE_BPS || rule.network_fee_bps < MIN_FEE_BPS {
                 panic_with_error!(env, SettlementError::InvalidFeeBps);
             }
+            if rule.platform_fee_bps > MAX_FEE_BPS || rule.network_fee_bps > MAX_FEE_BPS {
+                panic_with_error!(env, SettlementError::InvalidFeeBps);
+            }
             if rule.platform_fee_bps + rule.network_fee_bps > BPS_DENOMINATOR {
                 panic_with_error!(env, SettlementError::InvalidFeeBps);
             }
@@ -505,6 +508,9 @@ impl SettlementContract {
             if new_rule.platform_fee_bps < MIN_FEE_BPS || new_rule.network_fee_bps < MIN_FEE_BPS {
                 panic_with_error!(env, SettlementError::InvalidFeeBps);
             }
+            if new_rule.platform_fee_bps > MAX_FEE_BPS || new_rule.network_fee_bps > MAX_FEE_BPS {
+                panic_with_error!(env, SettlementError::InvalidFeeBps);
+            }
             if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
                 panic_with_error!(env, SettlementError::InvalidSettlementDelay);
             }

settlement_contract/src/errors.rs

@@ -17,8 +17,8 @@ pub enum SettlementError {
     NotInitialized = 2,
     /// The caller does not match the stored admin address.
     Unauthorized = 3,
-    /// The fee BPS values exceed 10 000 (`BPS_DENOMINATOR`) or their sum
-    /// exceeds 10 000, or either value is below `MIN_FEE_BPS` (5).
+    /// Either fee BPS value is below `MIN_FEE_BPS` (5) or above `MAX_FEE_BPS`
+    /// (5 000), or their sum exceeds `BPS_DENOMINATOR` (10 000).
     /// Raised by `set_settlement_rule` and `set_default_rule`.
     InvalidFeeBps = 4,
     /// The contract is paused. Most state‑mutating operations are blocked.
@@ -44,8 +44,7 @@ pub enum SettlementError {
     /// The target merchant address is not registered. Raised by
     /// `set_settlement_rule`, `store_payment_reference`, `calculate_fee_split`,
     /// and `unregister_merchant` when the merchant is missing.
-    MerchantMissing = 301,
-    // Code 302 is intentionally reserved (formerly `InvalidAmount`).
+    MerchantMissing = 302,
     /// `store_payment_reference` was called with a 32‑byte reference that
     /// already exists in storage.
     DuplicatePaymentReference = 303,

settlement_contract/src/settlement.rs

@@ -1,6 +1,6 @@
 use soroban_sdk::{contractimpl, panic_with_error, Address, Env, Symbol, Vec};
 
-use bettapay_common::constants::{BPS_DENOMINATOR, MIN_FEE_BPS};
+use bettapay_common::constants::{BPS_DENOMINATOR, MAX_FEE_BPS, MIN_FEE_BPS};
 
 use crate::errors::SettlementError;
 use crate::storage::{
@@ -36,6 +36,9 @@ impl SettlementContract {
             if rule.platform_fee_bps < MIN_FEE_BPS || rule.network_fee_bps < MIN_FEE_BPS {
                 panic_with_error!(&env, SettlementError::InvalidFeeBps);
             }
+            if rule.platform_fee_bps > MAX_FEE_BPS || rule.network_fee_bps > MAX_FEE_BPS {
+                panic_with_error!(&env, SettlementError::InvalidFeeBps);
+            }
             if rule.platform_fee_bps + rule.network_fee_bps > BPS_DENOMINATOR {
                 panic_with_error!(&env, SettlementError::InvalidFeeBps);
             }
@@ -98,6 +101,9 @@ impl SettlementContract {
             if new_rule.platform_fee_bps < MIN_FEE_BPS || new_rule.network_fee_bps < MIN_FEE_BPS {
                 panic_with_error!(&env, SettlementError::InvalidFeeBps);
             }
+            if new_rule.platform_fee_bps > MAX_FEE_BPS || new_rule.network_fee_bps > MAX_FEE_BPS {
+                panic_with_error!(&env, SettlementError::InvalidFeeBps);
+            }
             if new_rule.settlement_delay_ledger > MAX_SETTLEMENT_DELAY_LEDGER {
                 panic_with_error!(&env, SettlementError::InvalidSettlementDelay);
             }

settlement_contract/src/tests/admin_tests.rs

@@ -250,6 +250,72 @@ fn set_default_rule_rejected_when_paused() {
     client.set_default_rule(&admins, &rule);
 }
 
+// ---------------------------------------------------------------------------
+// fee ceiling (issue #521)
+// ---------------------------------------------------------------------------
+
+// Both fees are independently capped at MAX_FEE_BPS (5000, i.e. 50%), even
+// before governance has configured a FeeConfig - settlement no longer relies
+// solely on `validate_fee_against_governance` (which is a no-op with no
+// governance config set) to keep per-fee values below 100%.
+#[test]
+#[should_panic(expected = "Error(Contract, #4)")]
+fn set_settlement_rule_rejects_platform_fee_above_max_fee_bps() {
+    let (_env, client, admins, merchant) = setup();
+    client.register_merchant(&admins, &merchant);
+
+    let rule = SettlementRule {
+        platform_fee_bps: bettapay_common::constants::MAX_FEE_BPS + 1,
+        network_fee_bps: 50,
+        settlement_delay_ledger: 7,
+        auto_settle: true,
+    };
+    client.set_settlement_rule(&admins, &merchant, &rule);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #4)")]
+fn set_settlement_rule_rejects_network_fee_above_max_fee_bps() {
+    let (_env, client, admins, merchant) = setup();
+    client.register_merchant(&admins, &merchant);
+
+    let rule = SettlementRule {
+        platform_fee_bps: 50,
+        network_fee_bps: bettapay_common::constants::MAX_FEE_BPS + 1,
+        settlement_delay_ledger: 7,
+        auto_settle: true,
+    };
+    client.set_settlement_rule(&admins, &merchant, &rule);
+}
+
+#[test]
+fn set_settlement_rule_accepts_fee_at_max_fee_bps_ceiling() {
+    let (_env, client, admins, merchant) = setup();
+    client.register_merchant(&admins, &merchant);
+
+    let rule = SettlementRule {
+        platform_fee_bps: bettapay_common::constants::MAX_FEE_BPS,
+        network_fee_bps: bettapay_common::constants::MIN_FEE_BPS,
+        settlement_delay_ledger: 7,
+        auto_settle: true,
+    };
+    client.set_settlement_rule(&admins, &merchant, &rule);
+}
+
+#[test]
+#[should_panic(expected = "Error(Contract, #4)")]
+fn set_default_rule_rejects_fee_above_max_fee_bps() {
+    let (_env, client, admins, _merchant) = setup();
+
+    let rule = SettlementRule {
+        platform_fee_bps: bettapay_common::constants::MAX_FEE_BPS + 1,
+        network_fee_bps: 50,
+        settlement_delay_ledger: 7,
+        auto_settle: true,
+    };
+    client.set_default_rule(&admins, &rule);
+}
+
 // ---------------------------------------------------------------------------
 // upgrade
 // ---------------------------------------------------------------------------