From e8a81c0c81bd0b98838fa14228b8ad2e59392c75 Mon Sep 17 00:00:00 2001 From: quartune <123062848+quartune@users.noreply.github.com> Date: Sun, 30 Aug 2026 14:23:11 +0000 Subject: [PATCH] fix: honor live protocol fee, validate EVM chains and chain_id, scope solver/user auth - fill_intent now charges the live set_config protocol_fee_bps instead of the compile-time PROTOCOL_FEE_BPS constant. - validate_src_token recognizes "avalanche" and "bsc" as EVM chains, closing a format-validation gap. - proof_registry's set_authorized_emitter rejects chain_id > u16::MAX. - Audited all require_auth() call sites (docs/auth-audit.md); upgraded submit_intent, accept_intent, and fill_intent to require_auth_for_args scoped to their load-bearing arguments. Closes: #260 Closes: #261 Closes: #262 Closes: #263 --- CHANGELOG.md | 6 ++ docs/auth-audit.md | 44 +++++++++++ docs/pre-deploy-security-checklist.md | 4 +- docs/solver-integration-guide.md | 10 +++ intent_settlement/src/lib.rs | 65 +++++++++------- intent_settlement/src/test.rs | 102 +++++++++++++++++++++++++- proof_registry/src/lib.rs | 7 ++ proof_registry/src/test.rs | 22 ++++++ 8 files changed, 231 insertions(+), 29 deletions(-) create mode 100644 docs/auth-audit.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 935afb1..835c410 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ first deploys to mainnet. ### Fixed +- **`fill_intent` now charges the live, admin-configured `protocol_fee_bps`** + (via `set_config`) instead of the compile-time `PROTOCOL_FEE_BPS` + constant, which previously made fee-rate changes silently have no effect. +- `validate_src_token` now recognizes `"avalanche"` and `"bsc"` as EVM + chains, closing a gap where malformed source-token addresses on those two + chains passed with no format validation. - `deregister_solver` now refuses to return a solver's bond while they hold an `Accepted` intent, closing a path to dodge `slash_solver` by withdrawing before the fill window expired. diff --git a/docs/auth-audit.md b/docs/auth-audit.md new file mode 100644 index 0000000..cff0fb8 --- /dev/null +++ b/docs/auth-audit.md @@ -0,0 +1,44 @@ +# `require_auth()` Call Site Audit + +Closes the "Authorization hardening" item in `docs/pre-deploy-security-checklist.md` +(#45, tracked here as #263). Every `require_auth()` call site in +`intent_settlement/src/lib.rs` was reviewed for whether upgrading to +`require_auth_for_args` would meaningfully reduce delegated-execution risk — +i.e. the risk that a third-party invoker contract calling on a signer's behalf +could redirect their signature toward unintended arguments. + +## Upgraded + +| Function | Old | New scope | Rationale | +|---|---|---|---| +| `submit_intent` | `user.require_auth()` | `(user, dst_token, min_dst_amount)` | If a composable invoker ever submits on a user's behalf, this prevents it from redirecting the user's signed submission to a different destination token or minimum output. | +| `accept_intent` | `solver.require_auth()` | `(intent_id,)` | Prevents a delegating invoker contract from having a solver accept a different intent than the one the solver actually signed for. | +| `fill_intent` | `solver.require_auth()` | `(solver, intent_id, fill_amount)` | Highest-value call site — the auth gates an outgoing token transfer. Prevents a delegating invoker from filling a different intent, or a different amount, than the solver signed for. | + +`accept_intent`'s batch wrapper (`accept_intent_batch`) delegates to +`accept_intent` per element and needed no separate change. + +## Kept as `require_auth()` + +| Function | Signer | Rationale | +|---|---|---| +| `initialize` | `admin` | One-time setup; the signer *is* the value being recorded as admin — no sub-scope to narrow. | +| `propose_fee_recipient` | stored `admin` | Single global admin capability; no meaningful sub-scope within "being admin". | +| `accept_fee_recipient` | `new_fee_recipient` | Recipient proves ownership of their own address; the timelock and pending-proposal match (`pending != new_fee_recipient` check) already constrain which proposal can be accepted. | +| `propose_admin_transfer` | stored `admin` | Same as `propose_fee_recipient`. | +| `accept_admin_transfer` | `new_admin` | Same as `accept_fee_recipient`. | +| `register_solver` | `solver` | Solver consents to locking their own bond funds; simple self-action with no delegated-execution surface. | +| `deregister_solver` | `solver` | Solver-only self-action. | +| `withdraw_bond` | `solver` | Solver-only self-action on their own bond. | +| `cancel_intent` | `user` | Simple "cancel my own intent" self-action; an explicit `intent.user != user` ownership check runs immediately after, providing defence-in-depth. | +| `request_extension` | `solver` | Grants at most one grace-period extension per intent; no funds move and no cross-intent redirection is possible (the intent is loaded and ownership-checked before use). | +| `require_admin` (helper; gates `unpause`, `set_pauser`, dst-token allowlist admin functions) | `admin` | Single admin address with uniform authority across these functions — no per-argument capability to scope. | +| `require_admin_or_pauser` (helper; gates `pause`) | `admin` or `pauser` | Same reasoning as `require_admin`; the admin/pauser check already precedes the auth call. | + +## Integration impact + +`require_auth_for_args` changes the exact signed-payload shape a client must +build. `submit_intent`, `accept_intent`, and `fill_intent` are called +respectively by user-facing clients and solver bots — see +`docs/solver-integration-guide.md` for the updated payload shapes solver bot +authors must sign. diff --git a/docs/pre-deploy-security-checklist.md b/docs/pre-deploy-security-checklist.md index b5a09c8..f55f0ed 100644 --- a/docs/pre-deploy-security-checklist.md +++ b/docs/pre-deploy-security-checklist.md @@ -18,9 +18,11 @@ checked off as their linked PR is merged into `main`. ### Authorization hardening -- [ ] **#45 — Audit all `require_auth()` call sites for correctness vs. Soroban's `require_auth_for_args`** +- [x] **#45 — Audit all `require_auth()` call sites for correctness vs. Soroban's `require_auth_for_args`** Review all 12 call sites; document per-function conclusion; upgrade any site where scoped authorization meaningfully reduces delegated-execution risk. + See `docs/auth-audit.md`. `submit_intent`, `accept_intent`, and `fill_intent` + upgraded to `require_auth_for_args`; all other sites kept as-is. ### Economic / bond sizing diff --git a/docs/solver-integration-guide.md b/docs/solver-integration-guide.md index 823d652..de355da 100644 --- a/docs/solver-integration-guide.md +++ b/docs/solver-integration-guide.md @@ -215,6 +215,16 @@ Reject the intent immediately if: ## The Accept → Fill Loop +> **Scoped authorization (2026 update):** `accept_intent` and `fill_intent` +> now call `require_auth_for_args` instead of `require_auth`, scoped to +> `(intent_id)` and `(solver, intent_id, fill_amount)` respectively (see +> `docs/auth-audit.md`). If you invoke directly via `stellar contract invoke` +> or the standard SDK contract client with your own solver key, this is +> transparent — the simulated auth entries are signed for you as before. It +> only matters if you construct and sign `SorobanAuthorizationEntry` values +> by hand (e.g. for a delegated/invoker-contract flow): the signed payload +> must now match the specific call's arguments, not just the function name. + ### Step 1 — Accept Call `accept_intent` to claim the exclusive 5-minute fill window: diff --git a/intent_settlement/src/lib.rs b/intent_settlement/src/lib.rs index 6915565..24c2777 100644 --- a/intent_settlement/src/lib.rs +++ b/intent_settlement/src/lib.rs @@ -8,7 +8,7 @@ use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, panic_with_error, token, xdr::ToXdr, - Address, Bytes, BytesN, Env, String, Symbol, Vec, + Address, Bytes, BytesN, Env, IntoVal, String, Symbol, Vec, }; #[cfg(test)] @@ -1176,13 +1176,18 @@ impl IntentSettlement { min_dst_amount: i128, deadline: Option, ) -> BytesN<32> { - // Auth audit: require_auth() is correct. The user must sign to assert - // ownership of the address receiving output tokens (dst). If a third-party - // contract were ever to call submit_intent on a user's behalf, switching to - // require_auth_for_args scoped to (user, dst_token, min_dst_amount) would - // limit the scope of delegated authorisation — noted as a future hardening - // opportunity if composable intent submission is added. - user.require_auth(); + // Auth audit (see docs/auth-audit.md): upgraded from require_auth() to + // require_auth_for_args, scoped to (user, dst_token, min_dst_amount), + // so a delegating invoker contract cannot redirect a user's signed + // submission toward a different destination token or minimum output. + user.require_auth_for_args(Vec::from_array( + &env, + [ + user.into_val(&env), + dst_token.into_val(&env), + min_dst_amount.into_val(&env), + ], + )); Self::require_not_paused(&env); Self::bump_instance_ttl(&env); @@ -1324,12 +1329,10 @@ impl IntentSettlement { /// Solver claims an intent (exclusive fill right for FILL_WINDOW seconds) pub fn accept_intent(env: Env, solver: Address, intent_id: BytesN<32>) { - // Auth audit: require_auth() is correct. The solver must sign to - // voluntarily take on the fill obligation and bond risk associated with - // this intent. require_auth_for_args scoped to intent_id could prevent a - // malicious invoker contract from accepting an unintended intent on the - // solver's behalf; noted as a future hardening opportunity. - solver.require_auth(); + // Auth audit (see docs/auth-audit.md): upgraded from require_auth() to + // require_auth_for_args, scoped to intent_id, so a delegating invoker + // contract cannot accept an unintended intent on the solver's behalf. + solver.require_auth_for_args(Vec::from_array(&env, [intent_id.into_val(&env)])); Self::require_not_paused(&env); Self::bump_instance_ttl(&env); @@ -1418,14 +1421,20 @@ impl IntentSettlement { /// The protocol fee is taken on each individual fill so the fee accounting /// stays consistent regardless of how many fills it takes. pub fn fill_intent(env: Env, solver: Address, intent_id: BytesN<32>, fill_amount: i128) { - // Auth audit: require_auth() is correct. The solver must sign to - // authorise the token transfer from their address to the user and fee - // recipient. This is the highest-value call site: the solver authorises - // a token transfer, so the auth is load-bearing. require_auth_for_args - // scoped to (solver, intent_id, fill_amount) would meaningfully tighten - // the scope if a delegated-execution pattern is ever introduced — noted - // as the strongest candidate for future hardening. - solver.require_auth(); + // Auth audit (see docs/auth-audit.md): upgraded from require_auth() to + // require_auth_for_args, scoped to (solver, intent_id, fill_amount) — + // the highest-value call site, since it authorises a token transfer. + // This closes the gap where a delegating invoker contract could + // authorise a fill of different size or against a different intent + // than the solver actually signed for. + solver.require_auth_for_args(Vec::from_array( + &env, + [ + solver.into_val(&env), + intent_id.into_val(&env), + fill_amount.into_val(&env), + ], + )); Self::require_not_paused(&env); Self::bump_instance_ttl(&env); @@ -1458,12 +1467,14 @@ impl IntentSettlement { panic_with_error!(&env, Error::ZeroAmount); } + let protocol_fee_bps = Self::load_config(&env).protocol_fee_bps; + // Deliver this fill's tokens to the user. let dst_client = token::Client::new(&env, &intent.dst_token); dst_client.transfer(&solver, &intent.user, &fill_amount); // Solver also pays the protocol fee on each fill. - let fee = fill_amount * PROTOCOL_FEE_BPS / 10_000; + let fee = fill_amount * protocol_fee_bps / 10_000; // ── Effects first (CEI) ────────────────────────────────────────────── // Mark the intent Filled and write every state change to storage // *before* any external token transfer executes. A hostile SEP-41 @@ -1482,7 +1493,7 @@ impl IntentSettlement { // 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) + .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)); @@ -1567,7 +1578,7 @@ 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; + let fee = fill_amount * protocol_fee_bps / 10_000; if fee > 0 { let fee_recipient: Address = env .storage() @@ -2164,7 +2175,9 @@ impl IntentSettlement { || chain_is(b"base") || chain_is(b"polygon") || chain_is(b"arbitrum") - || chain_is(b"optimism"); + || chain_is(b"optimism") + || chain_is(b"avalanche") + || chain_is(b"bsc"); if is_evm { // EVM token address: exactly "0x" + 40 hex chars = 42 characters. diff --git a/intent_settlement/src/test.rs b/intent_settlement/src/test.rs index 9ed8561..642bee5 100644 --- a/intent_settlement/src/test.rs +++ b/intent_settlement/src/test.rs @@ -10,8 +10,8 @@ use crate::{ FILL_WINDOW, INTENT_EXPIRY, MIN_BOND, ADMIN_TIMELOCK_DELAY, }; use soroban_sdk::{ - testutils::{Address as _, Ledger}, - token, Address, BytesN, Env, String, Symbol, + testutils::{Address as _, Ledger, MockAuth, MockAuthInvoke}, + token, Address, BytesN, Env, IntoVal, String, Symbol, }; // ─── Test fixture ─────────────────────────────────────────────────────────────── @@ -1687,6 +1687,37 @@ fn solver_record_consistent_with_token_balances_after_register() { assert_eq!(ctx.bond().balance(&ctx.solver), 0); } +// #263 — fill_intent now scopes its auth to (solver, intent_id, fill_amount) +// via require_auth_for_args. A signed auth entry bound to a different +// intent_id than the one actually being filled — the shape a delegating +// invoker contract could otherwise exploit — must be rejected. +#[test] +fn fill_intent_delegated_auth_wrong_intent_id_rejected() { + let ctx = setup(); + let c = ctx.client(); + + ctx.register_solver(); + let id = ctx.submit(); + c.accept_intent(&ctx.solver, &id); + + let wrong_id = BytesN::from_array(&ctx.env, &[0u8; 32]); + let fee = FILL * 5 / 10_000; + ctx.dst_admin().mint(&ctx.solver, &(FILL + fee)); + + ctx.env.mock_auths(&[MockAuth { + address: &ctx.solver, + invoke: &MockAuthInvoke { + contract: &ctx.contract_id, + fn_name: "fill_intent", + args: (ctx.solver.clone(), wrong_id, FILL).into_val(&ctx.env), + sub_invokes: &[], + }, + }]); + + let res = c.try_fill_intent(&ctx.solver, &id, &FILL); + assert!(res.is_err()); +} + // #26 — CEI ordering in fill_intent: state is committed before transfers. // // We verify two complementary properties: @@ -2122,6 +2153,32 @@ fn fill_intent_fee_at_boundary_does_not_overflow() { assert!(c.get_intent(&id).unwrap().state == IntentState::Filled); } +// #260 — fill_intent must charge the live, admin-configured protocol fee +// rate, not the compile-time PROTOCOL_FEE_BPS constant. +#[test] +fn fill_intent_honors_set_config_protocol_fee() { + let ctx = setup(); + let c = ctx.client(); + + // Set a non-default fee rate (1% = 100 bps) via set_config, keeping the + // other three parameters at their existing defaults. + let new_fee_bps: i128 = 100; + c.set_config(&MIN_BOND, &FILL_WINDOW, &INTENT_EXPIRY, &new_fee_bps); + + ctx.register_solver(); + let id = ctx.submit(); + c.accept_intent(&ctx.solver, &id); + + let fee = FILL * new_fee_bps / 10_000; + ctx.dst_admin().mint(&ctx.solver, &(FILL + fee)); + + c.fill_intent(&ctx.solver, &id, &FILL); + + assert_eq!(ctx.dst().balance(&ctx.user), FILL); + assert_eq!(ctx.dst().balance(&ctx.fee_recipient), fee); + assert_eq!(ctx.dst().balance(&ctx.solver), 0); +} + // ─── Issue #32: tiny bond slash floor ──────────────────────────────────────────── /// #32: When a solver's bond has been whittled to a very small value (< 10 in @@ -2655,6 +2712,47 @@ fn valid_evm_token_lowercase_accepted() { ); } +/// Well-formed EVM addresses on "avalanche" and "bsc" are accepted — these +/// two chains were previously missing from the `is_evm` check (#261). +#[test] +fn valid_evm_token_avalanche_and_bsc_accepted() { + let ctx = setup(); + for chain_str in ["avalanche", "bsc"] { + let deadline: Option = None; + ctx.client().submit_intent( + &ctx.user, + &String::from_str(&ctx.env, chain_str), + &String::from_str(&ctx.env, "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"), + &SRC_AMT, + &ctx.dst_token, + &MIN_DST, + &deadline, + ); + } +} + +/// A malformed token address on "avalanche" or "bsc" must be rejected, just +/// like the other five EVM chains (#261 regression test). +#[test] +fn malformed_evm_token_avalanche_and_bsc_rejected() { + let ctx = setup(); + for chain_str in ["avalanche", "bsc"] { + let deadline: Option = None; + let res = ctx.client().try_submit_intent( + &ctx.user, + &String::from_str(&ctx.env, chain_str), + // No "0x" prefix — would previously fall through as "unknown + // chain: skip validation" and pass. + &String::from_str(&ctx.env, "A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"), + &SRC_AMT, + &ctx.dst_token, + &MIN_DST, + &deadline, + ); + assert_eq!(res, Err(Ok(Error::InvalidSrcToken.into()))); + } +} + /// Missing "0x" prefix on an EVM chain is rejected with InvalidSrcToken. #[test] fn evm_token_without_0x_prefix_rejected() { diff --git a/proof_registry/src/lib.rs b/proof_registry/src/lib.rs index fd68b33..e68b76e 100644 --- a/proof_registry/src/lib.rs +++ b/proof_registry/src/lib.rs @@ -97,6 +97,10 @@ pub enum Error { InvalidPayload = 6, /// Contract not initialized (`Admin` key absent). NotInitialized = 7, + /// `set_authorized_emitter` called with a `chain_id` that exceeds + /// `u16::MAX` — the real Wormhole chain-ID space is 16 bits, so such a + /// value can never appear in a decoded VAA. + ChainIdOutOfRange = 8, } // ─── Contract ───────────────────────────────────────────────────────────────── @@ -128,6 +132,9 @@ impl ProofRegistry { /// accepted by `receive_message`. pub fn set_authorized_emitter(env: Env, chain_id: u32, emitter: BytesN<32>) { Self::require_admin(&env); + if chain_id > u16::MAX as u32 { + panic_with_error!(&env, Error::ChainIdOutOfRange); + } env.storage() .instance() .set(&ProofKey::AuthorizedEmitter(chain_id), &emitter); diff --git a/proof_registry/src/test.rs b/proof_registry/src/test.rs index 3d16eb4..ce55c43 100644 --- a/proof_registry/src/test.rs +++ b/proof_registry/src/test.rs @@ -128,6 +128,28 @@ fn get_authorized_emitter_returns_none_if_unset() { assert_eq!(ctx.client().get_authorized_emitter(&2), None); } +// #262 — the real Wormhole chain-ID space is 16 bits; chain_id must be +// rejected once it exceeds u16::MAX. +#[test] +fn set_authorized_emitter_accepts_u16_max_boundary() { + let ctx = setup(); + let c = ctx.client(); + let emitter: BytesN<32> = BytesN::from_array(&ctx.env, &[0xde; 32]); + + c.set_authorized_emitter(&(u16::MAX as u32), &emitter); + assert_eq!(c.get_authorized_emitter(&(u16::MAX as u32)), Some(emitter)); +} + +#[test] +fn set_authorized_emitter_rejects_above_u16_max() { + let ctx = setup(); + let c = ctx.client(); + let emitter: BytesN<32> = BytesN::from_array(&ctx.env, &[0xde; 32]); + + let res = c.try_set_authorized_emitter(&(u16::MAX as u32 + 1), &emitter); + assert_eq!(res, Err(Ok(Error::ChainIdOutOfRange.into()))); +} + #[test] fn remove_authorized_emitter_clears_entry() { let ctx = setup();