fix: two-step fee recipient, checked fee arithmetic, slash floor, tok…

intent_settlement/src/lib.rs

@@ -41,6 +41,7 @@ const INSTANCE_TTL_EXTEND_TO: u32 = DAY_IN_LEDGERS * 60;
 pub enum DataKey {
     Admin,
     FeeRecipient,
+    PendingFeeRecipient, // proposed-but-not-yet-accepted new fee recipient (issue #30)
     BondToken,          // USDC address for bonds
     Intent(BytesN<32>), // intent_id -> IntentRecord
     Solver(Address),    // address -> SolverRecord
@@ -133,6 +134,12 @@ pub enum Error {
     DeadlineNotReached = 19,
     InsufficientBond = 20,
     DstTokenNotAllowed = 21,
+    /// #30: no pending fee-recipient proposal to accept
+    NoPendingFeeRecipient = 22,
+    /// #31: fee arithmetic overflowed (fill_amount is astronomically large)
+    FeeOverflow = 23,
+    /// #33: the address passed to add_allowed_dst_token doesn't implement SEP-41
+    InvalidTokenInterface = 24,
 }
 
 // ─── Contract ─────────────────────────────────────────────────────────────────
@@ -164,19 +171,51 @@ impl IntentSettlement {
 
     // ── Admin ──────────────────────────────────────────────────────────────────
 
-    /// Admin-only: rotate the address that receives protocol fees and slashed
-    /// bonds. There's no other way to change this once `initialize` runs.
-    pub fn set_fee_recipient(env: Env, new_fee_recipient: Address) {
+    /// Admin-only: propose a new fee recipient address. The proposal is stored
+    /// but not yet active. The new address must call `accept_fee_recipient` to
+    /// confirm, mirroring `transfer_admin`'s two-step pattern so a typo'd or
+    /// unreachable address can never silently misroute protocol fees.
+    ///
+    /// A new proposal overwrites any prior pending proposal, so the admin can
+    /// correct a mistake before the recipient has accepted.
+    pub fn propose_fee_recipient(env: Env, new_fee_recipient: Address) {
         let admin: Address = env
             .storage()
             .instance()
             .get(&DataKey::Admin)
             .unwrap_or_else(|| panic_with_error!(&env, Error::NotInitialized));
         admin.require_auth();
 
+        env.storage()
+            .instance()
+            .set(&DataKey::PendingFeeRecipient, &new_fee_recipient);
+
+        env.events().publish(
+            (Symbol::new(&env, "fee_recipient_proposed"),),
+            new_fee_recipient,
+        );
+    }
+
+    /// The pending fee recipient confirms the handover. Until this is called
+    /// the current fee recipient remains unchanged.
+    pub fn accept_fee_recipient(env: Env, new_fee_recipient: Address) {
+        let pending: Address = env
+            .storage()
+            .instance()
+            .get(&DataKey::PendingFeeRecipient)
+            .unwrap_or_else(|| panic_with_error!(&env, Error::NoPendingFeeRecipient));
+
+        if pending != new_fee_recipient {
+            panic_with_error!(&env, Error::Unauthorized);
+        }
+        new_fee_recipient.require_auth();
+
         env.storage()
             .instance()
             .set(&DataKey::FeeRecipient, &new_fee_recipient);
+        env.storage()
+            .instance()
+            .remove(&DataKey::PendingFeeRecipient);
 
         env.events().publish(
             (Symbol::new(&env, "fee_recipient_updated"),),
@@ -208,8 +247,25 @@ impl IntentSettlement {
     /// submit_intent had no validation on dst_token at all -- any address,
     /// including a bogus or malicious "token" contract, could be named as
     /// the destination.
+    ///
+    /// Before storing the allowance we call `decimals()` on the candidate
+    /// address as a lightweight SEP-41 interface probe (issue #33). If the
+    /// address doesn't implement the token interface the call traps and the
+    /// transaction reverts, surfacing the error at admin time rather than
+    /// silently allowing a non-token that would only fail later inside
+    /// fill_intent's transfer call.
+    ///
+    /// Note: `decimals()` is a read-only view, so this probe has no side
+    /// effects on the token's state.
     pub fn add_allowed_dst_token(env: Env, token: Address) {
         Self::require_admin(&env);
+
+        // Probe the SEP-41 interface: if `token` isn't a real token contract
+        // this will trap and revert the transaction before we store anything.
+        let token_client = token::Client::new(&env, &token);
+        // decimals() is a pure view with no side-effects; we discard the value.
+        let _decimals = token_client.decimals();
+
         env.storage()
             .instance()
             .set(&DataKey::AllowedDstToken(token.clone()), &true);
@@ -604,7 +660,15 @@ impl IntentSettlement {
         // fee from the solver — rather than clawing it back from the user — keeps
         // the user's received amount at or above `min_dst_amount`, and keeps every
         // token transfer authorized by the solver who signed this call.
-        let fee = fill_amount * PROTOCOL_FEE_BPS / 10_000;
+        //
+        // Explicit checked_mul/checked_div makes the overflow-safety property
+        // visible in code, rather than relying solely on the Cargo.toml
+        // overflow-checks = true release-profile setting (issue #31).
+        let fee = fill_amount
+            .checked_mul(PROTOCOL_FEE_BPS)
+            .unwrap_or_else(|| panic_with_error!(&env, Error::FeeOverflow))
+            .checked_div(10_000)
+            .unwrap_or_else(|| panic_with_error!(&env, Error::FeeOverflow));
         if fee > 0 {
             let fee_recipient: Address = env
                 .storage()
@@ -713,8 +777,10 @@ impl IntentSettlement {
             .get(&DataKey::Solver(solver_addr.clone()))
             .unwrap();
 
-        // Slash 10% of bond
-        let slash_amount = solver_record.bond_amount / 10;
+        // Slash 10% of bond, with a floor of 1 so that a non-zero bond is never
+        // economically unpunished due to integer division rounding to zero
+        // (issue #32: tiny bonds below 10 would otherwise yield slash_amount = 0).
+        let slash_amount = (solver_record.bond_amount / 10).max(1);
         solver_record.bond_amount -= slash_amount;
         solver_record.fills_failed += 1;
         solver_record.active_intents = solver_record.active_intents.saturating_sub(1);
@@ -824,6 +890,10 @@ impl IntentSettlement {
         env.storage().instance().get(&DataKey::FeeRecipient)
     }
 
+    pub fn get_pending_fee_recipient(env: Env) -> Option<Address> {
+        env.storage().instance().get(&DataKey::PendingFeeRecipient)
+    }
+
     pub fn get_bond_token(env: Env) -> Option<Address> {
         env.storage().instance().get(&DataKey::BondToken)
     }

intent_settlement/src/test.rs

@@ -6,8 +6,8 @@
 //! expiry, solver bonding/slashing, and the guard conditions on each step.
 
 use crate::{
-    Error, IntentSettlement, IntentSettlementClient, IntentState, FILL_WINDOW, INTENT_EXPIRY,
-    MIN_BOND,
+    DataKey, Error, IntentSettlement, IntentSettlementClient, IntentState, SolverRecord,
+    FILL_WINDOW, INTENT_EXPIRY, MIN_BOND,
 };
 use soroban_sdk::{
     testutils::{Address as _, Ledger},
@@ -136,15 +136,29 @@ fn cannot_initialize_twice() {
 // ─── Admin ──────────────────────────────────────────────────────────────────────
 
 #[test]
-fn admin_can_set_fee_recipient() {
+fn admin_can_propose_and_accept_fee_recipient() {
     let ctx = setup();
     let new_recipient = Address::generate(&ctx.env);
 
-    ctx.client().set_fee_recipient(&new_recipient);
+    // Step 1: admin proposes
+    ctx.client().propose_fee_recipient(&new_recipient);
+    assert_eq!(
+        ctx.client().get_pending_fee_recipient(),
+        Some(new_recipient.clone())
+    );
+    // Active recipient unchanged until accepted
+    assert_eq!(
+        ctx.client().get_fee_recipient(),
+        Some(ctx.fee_recipient.clone())
+    );
+
+    // Step 2: new recipient accepts
+    ctx.client().accept_fee_recipient(&new_recipient);
     assert_eq!(
         ctx.client().get_fee_recipient(),
         Some(new_recipient.clone())
     );
+    assert_eq!(ctx.client().get_pending_fee_recipient(), None);
 
     // The new recipient actually receives fees going forward.
     let c = ctx.client();
@@ -157,6 +171,34 @@ fn admin_can_set_fee_recipient() {
     assert_eq!(ctx.dst().balance(&new_recipient), fee);
 }
 
+/// #30: A non-pending address cannot hijack the accept step.
+#[test]
+fn accept_fee_recipient_wrong_address_fails() {
+    let ctx = setup();
+    let new_recipient = Address::generate(&ctx.env);
+    let imposter = Address::generate(&ctx.env);
+
+    ctx.client().propose_fee_recipient(&new_recipient);
+
+    let res = ctx.client().try_accept_fee_recipient(&imposter);
+    assert_eq!(res, Err(Ok(Error::Unauthorized.into())));
+
+    // Original fee recipient unchanged.
+    assert_eq!(
+        ctx.client().get_fee_recipient(),
+        Some(ctx.fee_recipient.clone())
+    );
+}
+
+/// #30: Calling accept before propose fails cleanly.
+#[test]
+fn accept_fee_recipient_without_proposal_fails() {
+    let ctx = setup();
+    let addr = Address::generate(&ctx.env);
+    let res = ctx.client().try_accept_fee_recipient(&addr);
+    assert_eq!(res, Err(Ok(Error::NoPendingFeeRecipient.into())));
+}
+
 #[test]
 fn admin_can_transfer_admin() {
     let ctx = setup();
@@ -166,9 +208,15 @@ fn admin_can_transfer_admin() {
     ctx.client().transfer_admin(&new_admin);
     assert_eq!(ctx.client().get_admin(), Some(new_admin.clone()));
 
-    // The new admin can now exercise admin-only functions.
+    // The new admin can now exercise admin-only functions — use the two-step
+    // propose/accept flow that replaced set_fee_recipient (issue #30).
     let another_recipient = Address::generate(&ctx.env);
-    ctx.client().set_fee_recipient(&another_recipient);
+    ctx.client().propose_fee_recipient(&another_recipient);
+    assert_eq!(
+        ctx.client().get_pending_fee_recipient(),
+        Some(another_recipient.clone())
+    );
+    ctx.client().accept_fee_recipient(&another_recipient);
     assert_eq!(ctx.client().get_fee_recipient(), Some(another_recipient));
 }
 
@@ -994,3 +1042,143 @@ fn get_bond_token_returns_configured_token() {
     let ctx = setup();
     assert_eq!(ctx.client().get_bond_token(), Some(ctx.bond_token.clone()));
 }
+
+// ─── Issue #31: fee overflow boundary ────────────────────────────────────────────
+
+/// #31: fill_amount just above i128::MAX / PROTOCOL_FEE_BPS (5) overflows the
+/// checked_mul and returns FeeOverflow rather than silently wrapping.
+///
+/// Boundary: i128::MAX / 5 = 34_028_236_692_093_846_346_337_460_743_176_821_145.
+/// Any value above that will cause `fill_amount * 5` to overflow i128.
+#[test]
+fn fill_intent_fee_overflow_returns_error() {
+    let ctx = setup();
+    let c = ctx.client();
+    ctx.register_solver();
+    let id = ctx.submit();
+    c.accept_intent(&ctx.solver, &id);
+
+    // Smallest fill_amount that overflows: (i128::MAX / 5) + 1.
+    // We satisfy min_dst_amount by keeping fill_amount >> MIN_DST.
+    let overflow_fill: i128 = i128::MAX / 5 + 1;
+
+    // Fund the solver so the dst transfer can proceed; the overflow is caught
+    // in the fee calculation that follows the transfer (the full transaction
+    // rolls back on panic_with_error, so the user's balance stays zero).
+    ctx.dst_admin().mint(&ctx.solver, &overflow_fill);
+
+    let res = c.try_fill_intent(&ctx.solver, &id, &overflow_fill);
+    assert_eq!(res, Err(Ok(Error::FeeOverflow.into())));
+}
+
+/// Sanity: a fill_amount just *at* the boundary (i128::MAX / 5) does not overflow.
+#[test]
+fn fill_intent_fee_at_boundary_does_not_overflow() {
+    let ctx = setup();
+    let c = ctx.client();
+    ctx.register_solver();
+    let id = ctx.submit();
+    c.accept_intent(&ctx.solver, &id);
+
+    // i128::MAX / 5 — fee = (i128::MAX / 5) * 5 / 10_000, which fits in i128.
+    let boundary_fill: i128 = i128::MAX / 5;
+    let fee = boundary_fill * 5 / 10_000;
+    ctx.dst_admin().mint(&ctx.solver, &(boundary_fill + fee));
+
+    // Should succeed (no overflow).
+    c.fill_intent(&ctx.solver, &id, &boundary_fill);
+    assert!(c.get_intent(&id).unwrap().state == IntentState::Filled);
+}
+
+// ─── Issue #32: tiny bond slash floor ────────────────────────────────────────────
+
+/// #32: When a solver's bond has been whittled to a very small value (< 10 in
+/// the token's smallest unit), integer division `bond / 10` rounds to 0.  The
+/// `.max(1)` floor ensures the slash is never economically free — a non-zero
+/// bond always produces a non-zero slash.
+///
+/// We plant a SolverRecord with bond_amount = 5 directly into storage (bypassing
+/// the MIN_BOND registration guard) to test the math boundary in isolation.
+#[test]
+fn slash_tiny_bond_always_yields_nonzero_slash() {
+    let ctx = setup();
+    let c = ctx.client();
+
+    // Register normally first so the contract recognises ctx.solver.
+    ctx.register_solver();
+
+    // Plant a SolverRecord with an artificially tiny bond directly into
+    // contract storage, simulating a bond that has been slashed many times.
+    let tiny_bond: i128 = 5; // 5 / 10 = 0 without the .max(1) floor
+    ctx.env.as_contract(&ctx.contract_id, || {
+        let mut record: SolverRecord = ctx
+            .env
+            .storage()
+            .persistent()
+            .get(&DataKey::Solver(ctx.solver.clone()))
+            .unwrap();
+        record.bond_amount = tiny_bond;
+        record.active_intents = 0;
+        ctx.env
+            .storage()
+            .persistent()
+            .set(&DataKey::Solver(ctx.solver.clone()), &record);
+    });
+
+    // Submit and accept an intent so slash_solver has something to slash.
+    let id = ctx.submit();
+    c.accept_intent(&ctx.solver, &id);
+
+    ctx.pass_time(FILL_WINDOW + 1);
+    c.slash_solver(&id);
+
+    // The slash must be >= 1 even though 5 / 10 == 0.
+    let solver = c.get_solver(&ctx.solver).unwrap();
+    assert!(
+        solver.bond_amount < tiny_bond,
+        "bond should have decreased after slash"
+    );
+    let slashed = tiny_bond - solver.bond_amount;
+    assert!(slashed >= 1, "slash_amount must be at least 1, got {slashed}");
+}
+
+// ─── Issue #33: add_allowed_dst_token validates SEP-41 interface ─────────────────
+
+/// #33: Passing the settlement contract's own address (which is not a token)
+/// to add_allowed_dst_token must fail.  The `decimals()` probe inside
+/// add_allowed_dst_token will trap on a contract that doesn't implement SEP-41,
+/// reverting the transaction before any storage entry is written.
+#[test]
+fn add_allowed_dst_token_rejects_non_token_contract() {
+    let ctx = setup();
+
+    // ctx.contract_id is a real deployed contract (IntentSettlement) but it
+    // does not implement the SEP-41 token interface, so decimals() will trap.
+    let res = ctx
+        .client()
+        .try_add_allowed_dst_token(&ctx.contract_id);
+
+    // The call must fail — either with InvalidTokenInterface or a generic
+    // contract-trap error (the host converts a trapped cross-contract call
+    // into an Err result in the test environment).
+    assert!(
+        res.is_err(),
+        "allowlisting a non-token address should fail"
+    );
+
+    // No storage entry must have been written for the bogus address.
+    assert!(
+        !ctx.client().is_dst_token_allowed(&ctx.contract_id),
+        "non-token address must not be stored in the allowlist"
+    );
+}
+
+/// #33 (positive case): a real SEP-41 token passes the probe and is stored.
+#[test]
+fn add_allowed_dst_token_accepts_real_token() {
+    let ctx = setup();
+
+    // dst_token was registered as a StellarAssetContract — it implements SEP-41.
+    ctx.client().add_allowed_dst_token(&ctx.dst_token);
+    assert!(ctx.client().is_dst_token_allowed(&ctx.dst_token));
+}