From e7a1fd6382aa43c1e9d2e1d78b46e578725c677d Mon Sep 17 00:00:00 2001 From: ironclad-x Date: Sun, 26 Jul 2026 15:13:05 +0000 Subject: [PATCH 1/2] fix: enforce CEI ordering in register_solver, deregister_solver, fill_intent - deregister_solver: remove SolverRecord and decrement TotalSolvers before the bond transfer so a re-entrant or double-call sees SolverNotRegistered rather than a live record eligible for a second refund. Closes #28 - register_solver: persist SolverRecord and increment TotalSolvers before client.transfer() pulls funds in so storage is always consistent with actual contract holdings. Closes #27 - fill_intent: write intent state (Filled), filled_at, fill_amount, solver stats, and protocol volume to storage before any dst_client.transfer() executes. A hostile SEP-41 token that re-enters mid-transfer will see the intent already Filled and be rejected. Closes #26 Regression tests added for each fix: - double_deregister_rejected_cleanly - solver_record_consistent_with_token_balances_after_register - fill_intent_state_committed_before_transfer_and_double_fill_rejected --- intent_settlement/src/lib.rs | 82 +++++++++++++++---------- intent_settlement/src/test.rs | 112 ++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 32 deletions(-) diff --git a/intent_settlement/src/lib.rs b/intent_settlement/src/lib.rs index 5d6afa3..d4aaf55 100644 --- a/intent_settlement/src/lib.rs +++ b/intent_settlement/src/lib.rs @@ -302,10 +302,12 @@ impl IntentSettlement { let is_new_solver = existing.is_none(); - let bond_token: Address = env.storage().instance().get(&DataKey::BondToken).unwrap(); - let client = token::Client::new(&env, &bond_token); - client.transfer(&solver, &env.current_contract_address(), &bond_amount); - + // ── Effects first (CEI) ────────────────────────────────────────────── + // Build and persist the SolverRecord *before* pulling funds in so the + // contract's storage is always consistent with what it holds: if the + // transfer were to fail (or a re-entrant call were made mid-transfer), + // the record either doesn't exist yet (new solver) or still reflects + // the pre-topup balance, rather than an inflated balance with no matching funds. let record = match existing { Some(mut s) => { s.bond_amount += bond_amount; @@ -340,6 +342,11 @@ impl IntentSettlement { .set(&DataKey::TotalSolvers, &(total + 1)); } + // ── Interaction: pull bond in ──────────────────────────────────────── + let bond_token: Address = env.storage().instance().get(&DataKey::BondToken).unwrap(); + let client = token::Client::new(&env, &bond_token); + client.transfer(&solver, &env.current_contract_address(), &bond_amount); + env.events().publish( (Symbol::new(&env, "solver_registered"), solver), bond_amount, @@ -360,17 +367,10 @@ impl IntentSettlement { panic_with_error!(&env, Error::SolverHasActiveIntents); } - // Return bond - if record.bond_amount > 0 { - let bond_token: Address = env.storage().instance().get(&DataKey::BondToken).unwrap(); - let client = token::Client::new(&env, &bond_token); - client.transfer( - &env.current_contract_address(), - &solver, - &record.bond_amount, - ); - } - + // ── Effects first (CEI) ────────────────────────────────────────────── + // Remove the solver record and update the counter *before* the external + // token transfer so that any re-entrant call sees no record and would + // panic with SolverNotRegistered rather than processing a double-refund. env.storage() .persistent() .remove(&DataKey::Solver(solver.clone())); @@ -384,6 +384,17 @@ impl IntentSettlement { .instance() .set(&DataKey::TotalSolvers, &total.saturating_sub(1)); + // ── Interaction: return bond ───────────────────────────────────────── + if record.bond_amount > 0 { + let bond_token: Address = env.storage().instance().get(&DataKey::BondToken).unwrap(); + let client = token::Client::new(&env, &bond_token); + client.transfer( + &env.current_contract_address(), + &solver, + &record.bond_amount, + ); + } + env.events().publish( (Symbol::new(&env, "solver_deregistered"), solver), record.bond_amount, @@ -596,23 +607,11 @@ impl IntentSettlement { panic_with_error!(&env, Error::InsufficientOutput); } - // Solver delivers the full requested output 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 (priced into their quote). Taking the - // 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; - if fee > 0 { - let fee_recipient: Address = env - .storage() - .instance() - .get(&DataKey::FeeRecipient) - .unwrap(); - dst_client.transfer(&solver, &fee_recipient, &fee); - } + // ── Effects first (CEI) ────────────────────────────────────────────── + // Mark the intent Filled and write every state change to storage + // *before* any external token transfer executes. A hostile SEP-41 + // token that attempts to re-enter fill_intent or slash_solver during + // the transfer would see the intent already Filled and be rejected. intent.state = IntentState::Filled; intent.filled_at = Some(now); @@ -647,6 +646,25 @@ impl IntentSettlement { .set(&DataKey::Intent(intent_id.clone()), &intent); Self::bump_intent_ttl(&env, &intent_id); + // ── Interactions: token transfers ──────────────────────────────────── + // Solver delivers the full requested output 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 (priced into their quote). Taking the + // 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; + if fee > 0 { + let fee_recipient: Address = env + .storage() + .instance() + .get(&DataKey::FeeRecipient) + .unwrap(); + dst_client.transfer(&solver, &fee_recipient, &fee); + } + env.events().publish( (Symbol::new(&env, "intent_filled"), solver), (intent_id, fill_amount, fee), diff --git a/intent_settlement/src/test.rs b/intent_settlement/src/test.rs index 74e0521..0f9497d 100644 --- a/intent_settlement/src/test.rs +++ b/intent_settlement/src/test.rs @@ -980,6 +980,118 @@ fn state_changing_calls_extend_instance_ttl() { assert!(instance_ttl >= crate::INSTANCE_TTL_EXTEND_TO - 1); } +// ─── CEI regression tests ──────────────────────────────────────────────────────── + +// #28 — double-deregister is rejected cleanly after the storage-first reorder. +// +// Before the fix, the record was still present when the bond transfer ran, so +// a second deregister_solver call before the remove could (in a future async or +// re-entrant context) see the record and transfer the bond a second time. +// After the fix the record is removed first, so the second call panics with +// SolverNotRegistered before it reaches the transfer. +#[test] +fn double_deregister_rejected_cleanly() { + let ctx = setup(); + ctx.register_solver(); + + // First deregister succeeds and removes the record. + ctx.client().deregister_solver(&ctx.solver); + assert!(ctx.client().get_solver(&ctx.solver).is_none()); + // Bond is back with the solver. + assert_eq!(ctx.bond().balance(&ctx.solver), BOND); + + // Second call on the same address must be rejected — no record to refund. + let res = ctx.client().try_deregister_solver(&ctx.solver); + assert_eq!(res, Err(Ok(Error::SolverNotRegistered.into()))); + + // Crucially the contract's bond balance must not have changed further — + // it was zero after the first deregister and should still be zero. + assert_eq!(ctx.bond().balance(&ctx.contract_id), 0); + // Solver's balance should still equal exactly one bond refund, not two. + assert_eq!(ctx.bond().balance(&ctx.solver), BOND); +} + +// #27 — SolverRecord state is consistent with actual token balances after +// register_solver. After the storage-first reorder the record is written +// before the transfer, so if the transfer were ever to fail the record simply +// wouldn't reflect a deposit that never happened. Here we verify the happy +// path: record.bond_amount == tokens held by the contract. +#[test] +fn solver_record_consistent_with_token_balances_after_register() { + let ctx = setup(); + + // Mint exactly BOND to the solver, then register. + ctx.bond_admin().mint(&ctx.solver, &BOND); + ctx.client().register_solver(&ctx.solver, &BOND); + + let record = ctx.client().get_solver(&ctx.solver).unwrap(); + + // Storage says the bond is BOND. + assert_eq!(record.bond_amount, BOND); + // The contract actually holds BOND tokens — no discrepancy. + assert_eq!(ctx.bond().balance(&ctx.contract_id), BOND); + // Solver's wallet is empty — tokens moved. + assert_eq!(ctx.bond().balance(&ctx.solver), 0); + + // Top-up path: record.bond_amount must keep tracking reality after a second deposit. + let topup = 200 * 10_000_000; + ctx.bond_admin().mint(&ctx.solver, &topup); + ctx.client().register_solver(&ctx.solver, &topup); + + let record2 = ctx.client().get_solver(&ctx.solver).unwrap(); + assert_eq!(record2.bond_amount, BOND + topup); + assert_eq!(ctx.bond().balance(&ctx.contract_id), BOND + topup); + assert_eq!(ctx.bond().balance(&ctx.solver), 0); +} + +// #26 — CEI ordering in fill_intent: state is committed before transfers. +// +// We verify two complementary properties: +// +// 1. After a successful fill the intent is Filled in storage *and* tokens +// have moved — state and funds are always in sync (the core CEI invariant). +// +// 2. A second fill_intent call on the same (already-Filled) intent is +// rejected with IntentAlreadyFilled before any transfer attempt. This is +// the on-chain guard that would stop a re-entrant token from triggering a +// double-fill: whatever point during the transfers the re-entrant call is +// made, storage already shows Filled. +#[test] +fn fill_intent_state_committed_before_transfer_and_double_fill_rejected() { + let ctx = setup(); + let c = ctx.client(); + + ctx.register_solver(); + let id = ctx.submit(); + c.accept_intent(&ctx.solver, &id); + + // Fund the solver with enough for the fill + fee. + let fee = FILL * 5 / 10_000; + ctx.dst_admin().mint(&ctx.solver, &(FILL + fee)); + + // Happy-path fill. + c.fill_intent(&ctx.solver, &id, &FILL); + + // 1. Storage reflects Filled and fill_amount is set. + let intent = c.get_intent(&id).unwrap(); + assert_eq!(intent.state, IntentState::Filled); + assert_eq!(intent.fill_amount, Some(FILL)); + + // 2. Tokens have moved: user got FILL, fee_recipient got fee, solver has 0. + 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); + + // 3. A second fill attempt is rejected before any transfer — this is exactly + // what a re-entrant token would hit mid-transfer after the CEI reorder. + ctx.dst_admin().mint(&ctx.solver, &(FILL + fee)); // give solver funds again + let res = c.try_fill_intent(&ctx.solver, &id, &FILL); + assert_eq!(res, Err(Ok(Error::IntentAlreadyFilled.into()))); + + // User's balance must not have increased — no double-payment. + assert_eq!(ctx.dst().balance(&ctx.user), FILL); +} + // ─── Views ────────────────────────────────────────────────────────────────────── #[test] From 3db0b88a19812483dde02a4668c37dd58bd83be5 Mon Sep 17 00:00:00 2001 From: ironclad-x Date: Sun, 26 Jul 2026 15:20:08 +0000 Subject: [PATCH 2/2] fix: submit_intent silently overwrites an existing intent record on collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute_intent_id previously hashed only (user, src_chain, src_amount, timestamp). Two submit_intent calls from the same user with identical args in the same ledger close produced the same intent_id, and the second write silently overwrote the first IntentRecord. Changes: - Add DataKey::UserNonce(Address) — a per-user submit counter stored in instance storage alongside Admin/FeeRecipient etc. - Increment the nonce on every submit_intent call and include it in the compute_intent_id preimage, making same-ledger collisions impossible in practice. - Add Error::IntentAlreadyExists (variant 22) and a .has() guard in submit_intent that rejects rather than silently overwrites on any (extremely unlikely) hash collision. Regression tests added: - same_ledger_identical_intents_produce_distinct_ids - nonce_increments_per_user_across_submissions - different_users_same_params_same_ledger_produce_distinct_ids Closes #25 --- intent_settlement/src/lib.rs | 35 ++++++++++++++-- intent_settlement/src/test.rs | 76 +++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/intent_settlement/src/lib.rs b/intent_settlement/src/lib.rs index d4aaf55..a816da7 100644 --- a/intent_settlement/src/lib.rs +++ b/intent_settlement/src/lib.rs @@ -50,6 +50,7 @@ pub enum DataKey { Paused, AllowedDstToken(Address), // dst_token -> present if allowed DstAllowlistEnabled, + UserNonce(Address), // per-user submit counter to widen intent_id preimage } // ─── Data Structs ───────────────────────────────────────────────────────────── @@ -133,6 +134,7 @@ pub enum Error { DeadlineNotReached = 19, InsufficientBond = 20, DstTokenNotAllowed = 21, + IntentAlreadyExists = 22, } // ─── Contract ───────────────────────────────────────────────────────────────── @@ -477,8 +479,30 @@ impl IntentSettlement { panic_with_error!(&env, Error::InvalidDeadline); } - // Deterministic intent_id = hash(user, src_chain, src_token, src_amount, now) - let intent_id = Self::compute_intent_id(&env, &user, &src_chain, src_amount, now); + // Widen the preimage with a per-user nonce so that two intents from + // the same user with identical (src_chain, src_amount) in the same + // ledger close produce distinct ids rather than colliding silently. + let nonce: u64 = env + .storage() + .instance() + .get(&DataKey::UserNonce(user.clone())) + .unwrap_or(0); + env.storage() + .instance() + .set(&DataKey::UserNonce(user.clone()), &(nonce + 1)); + + // Deterministic intent_id = hash(user, src_chain, src_token, src_amount, now, nonce) + let intent_id = Self::compute_intent_id(&env, &user, &src_chain, src_amount, now, nonce); + + // Guard against an extremely unlikely hash collision: if a record with + // this id somehow already exists, reject rather than silently overwrite. + if env + .storage() + .persistent() + .has(&DataKey::Intent(intent_id.clone())) + { + panic_with_error!(&env, Error::IntentAlreadyExists); + } let intent = IntentRecord { intent_id: intent_id.clone(), @@ -910,15 +934,18 @@ impl IntentSettlement { src_chain: &String, amount: i128, timestamp: u64, + nonce: u64, ) -> BytesN<32> { // Build a collision-resistant preimage from the full intent context, then - // hash to a 32-byte id. Including the user and source chain ensures two - // otherwise-identical intents from different users or chains never collide. + // hash to a 32-byte id. Including the user, source chain, and a + // per-user nonce ensures two otherwise-identical intents from the same + // user in the same ledger always produce distinct ids. let mut preimage = Bytes::new(env); preimage.append(&user.clone().to_xdr(env)); preimage.append(&src_chain.clone().to_xdr(env)); preimage.extend_from_array(&amount.to_be_bytes()); preimage.extend_from_array(×tamp.to_be_bytes()); + preimage.extend_from_array(&nonce.to_be_bytes()); env.crypto().sha256(&preimage).into() } } diff --git a/intent_settlement/src/test.rs b/intent_settlement/src/test.rs index 0f9497d..e4d4332 100644 --- a/intent_settlement/src/test.rs +++ b/intent_settlement/src/test.rs @@ -642,6 +642,82 @@ fn submit_intent_past_deadline_fails() { assert_eq!(res, Err(Ok(Error::InvalidDeadline.into()))); } +// #25 — same-ledger intents with identical params produce distinct ids (nonce). +// +// Before the fix, compute_intent_id hashed only (user, src_chain, src_amount, +// timestamp). Two submit_intent calls in the same ledger with the same args +// produced the same id and the second silently overwrote the first record. +// After the fix a per-user nonce is included in the preimage, so every call +// yields a unique id regardless of timestamp. +#[test] +fn same_ledger_identical_intents_produce_distinct_ids() { + let ctx = setup(); + let c = ctx.client(); + + // Both submits happen at the same ledger timestamp. + let id1 = ctx.submit(); + let id2 = ctx.submit(); + + // The ids must be different — neither intent overwrote the other. + assert_ne!(id1, id2); + + // Both records must be independently retrievable. + assert!(c.get_intent(&id1).is_some()); + assert!(c.get_intent(&id2).is_some()); + + // Total intents counter must reflect both submissions. + assert_eq!(c.get_stats().0, 2); +} + +#[test] +fn nonce_increments_per_user_across_submissions() { + let ctx = setup(); + let c = ctx.client(); + + // Submit three intents from the same user in the same ledger close. + let id1 = ctx.submit(); + let id2 = ctx.submit(); + let id3 = ctx.submit(); + + // All three must be distinct. + assert_ne!(id1, id2); + assert_ne!(id2, id3); + assert_ne!(id1, id3); + + // All three records exist. + assert!(c.get_intent(&id1).is_some()); + assert!(c.get_intent(&id2).is_some()); + assert!(c.get_intent(&id3).is_some()); + + assert_eq!(c.get_stats().0, 3); +} + +#[test] +fn different_users_same_params_same_ledger_produce_distinct_ids() { + // Nonces are per-user so two different users both on nonce 0 at the same + // timestamp must still get different ids (the user address is in the preimage). + let ctx = setup(); + let c = ctx.client(); + + let user2 = Address::generate(&ctx.env); + let deadline: Option = None; + + let id1 = ctx.submit(); // ctx.user, nonce=0 + let id2 = c.submit_intent( + &user2, + &String::from_str(&ctx.env, "ethereum"), + &String::from_str(&ctx.env, "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"), + &SRC_AMT, + &ctx.dst_token, + &MIN_DST, + &deadline, + ); // user2, nonce=0 + + assert_ne!(id1, id2); + assert!(c.get_intent(&id1).is_some()); + assert!(c.get_intent(&id2).is_some()); +} + // ─── Happy path: submit → accept → fill ───────────────────────────────────────── #[test]