From ef12edd9e186ba4dedc3b49de2271eb20d1e1034 Mon Sep 17 00:00:00 2001 From: grunch Date: Mon, 20 Jul 2026 16:39:58 -0300 Subject: [PATCH 1/3] fix: validate the dispute initiator before refunding the escrow (#805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `admin_cancel_action` called `cancel_hold_invoice` — an irreversible refund of the seller's escrow — before resolving the dispute initiator, and that resolution can reject the request with `DisputeEventError` when neither `seller_dispute` nor `buyer_dispute` is set (or both are). On that path the seller was already refunded while the order stayed in `Dispute` and the status transition to `CanceledByAdmin` never ran, leaving the Lightning side and the DB permanently disagreeing with no way back. Move the initiator resolution above the refund so every check that can reject the call runs before the escrow is touched. The valid path is unchanged. Regression test: an order with a hold-invoice hash and no initiator flag now fails with `DisputeEventError` and is left in `Dispute`. Before the fix it returned `LnNodeError` — proof the refund RPC had already been dispatched. Closes #805 --- src/app/admin_cancel.rs | 62 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 7 deletions(-) diff --git a/src/app/admin_cancel.rs b/src/app/admin_cancel.rs index 9ce21ef4..19b2909f 100644 --- a/src/app/admin_cancel.rs +++ b/src/app/admin_cancel.rs @@ -115,6 +115,18 @@ pub async fn admin_cancel_action( let bond_resolution = bond::extract_bond_resolution(&msg); bond::validate_bond_resolution(pool, &order, &bond_resolution).await?; + // Resolve the dispute initiator *before* the escrow is touched (#805). + // This match rejects orders whose initiator flags are unset or ambiguous, + // and `cancel_hold_invoice` below is irreversible: running it first meant + // a rejected request could still refund the seller while leaving the order + // in `Dispute`, so the LN side and the DB disagreed with no way back. + // Every check that can reject the call now precedes the refund. + let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { + (true, false) => "seller", + (false, true) => "buyer", + (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), + }; + if order.hash.is_some() { // We return funds to seller if let Some(hash) = order.hash.as_ref() { @@ -126,13 +138,6 @@ pub async fn admin_cancel_action( // we check if there is a dispute let dispute = find_dispute_by_order_id(pool, order.id).await; - // Get the creator of the dispute - let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; - if let Ok(mut d) = dispute { let dispute_id = d.id; // we update the dispute @@ -561,6 +566,49 @@ mod tests { )); } + /// Regression for #805: an order carrying a hold-invoice `hash` whose + /// dispute-initiator flags are unset must fail validation *before* the + /// irreversible `cancel_hold_invoice`. Reaching the LND seam here would + /// surface as `LnNodeError` and would mean the seller was already + /// refunded on a request that goes on to be rejected. + #[tokio::test] + async fn dispute_without_initiator_flag_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + // Hold invoice present, but neither side is flagged as the dispute + // initiator, so `dispute_initiator` resolution must reject the call. + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the LND cancel, got {result:?}" + ); + + // The order must be left untouched so the solver can retry. + let stored = get_order(&cancel_msg(order.id), ctx.pool()).await.unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + /// A dispute order carrying a hold-invoice `hash` returns funds to the /// seller via `cancel_hold_invoice`, which fails against the dead LND /// endpoint and surfaces as `LnNodeError`. From a88e7946eebebaf78ddc6b45e21546b60143d7e0 Mon Sep 17 00:00:00 2001 From: grunch Date: Fri, 24 Jul 2026 22:20:51 -0300 Subject: [PATCH 2/3] docs: scope the ordering comment to the initiator check Reviewer feedback on #825: the comment claimed every check that can reject the call precedes the refund, which overstates the invariant. The handler still has fallible steps after the refund (dispute/order events, DB updates, DM delivery); atomicity there is tracked in #810. --- src/app/admin_cancel.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/admin_cancel.rs b/src/app/admin_cancel.rs index 19b2909f..f3f33e01 100644 --- a/src/app/admin_cancel.rs +++ b/src/app/admin_cancel.rs @@ -120,7 +120,9 @@ pub async fn admin_cancel_action( // and `cancel_hold_invoice` below is irreversible: running it first meant // a rejected request could still refund the seller while leaving the order // in `Dispute`, so the LN side and the DB disagreed with no way back. - // Every check that can reject the call now precedes the refund. + // This initiator check now precedes the refund. Operations after the + // refund can still fail (dispute/order events, DB updates, DM delivery); + // making that path atomic is tracked separately in #810. let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { (true, false) => "seller", (false, true) => "buyer", From e6f86a57e108cce4a34edcaa7c54136bfcdf3e00 Mon Sep 17 00:00:00 2001 From: grunch Date: Fri, 24 Jul 2026 22:39:35 -0300 Subject: [PATCH 3/3] fix: hoist every rejecting check above the irreversible escrow moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the strict review of #825. The PR moved the dispute-initiator resolution above cancel_hold_invoice, but three gaps of the same class (#805) remained: - admin_cancel resolved the counterparty pubkeys only after the refund, the dispute row and the order status had been written. A missing or unparseable pubkey rejected the call once the money had already moved, past any solver retry (the Dispute guard rejects a second cancel) and before the bond resolution, stranding every Locked bond — only range maker bonds have a reconciler. The resolution now runs next to the initiator check. - The DM fan-out returned on the first failure, skipping the same bond resolution on a mere relay hiccup. Delivery is now best effort with an error log per recipient, mirroring notify_bond_slashed. - admin_settle still resolved the initiator after settle_seller_hold_invoice, which is equally irreversible. Hoisted above the settle. Both ambiguous-flag arms now log order id and both flags: DisputeEventError alone gave the operator nothing to diagnose with. Tests: an unparseable-pubkey rejection and an ambiguous-flag settle are each pinned before their escrow move (both fail against the previous ordering), plus the both-flags-set arm and an assertion that a failed DM no longer aborts admin_cancel before the bond resolution. Stale doc comments on the existing tests corrected. --- src/app/admin_cancel.rs | 191 +++++++++++++++++++++++++++++++++------- src/app/admin_settle.rs | 73 +++++++++++++-- 2 files changed, 225 insertions(+), 39 deletions(-) diff --git a/src/app/admin_cancel.rs b/src/app/admin_cancel.rs index f3f33e01..e875bcf2 100644 --- a/src/app/admin_cancel.rs +++ b/src/app/admin_cancel.rs @@ -42,9 +42,16 @@ use tracing::{error, info}; /// Returns `MostroError` if: /// - Solver is not assigned to the dispute /// - Order/dispute not found +/// - The dispute initiator flags are unset or ambiguous, or a counterparty +/// pubkey is missing/unparseable — both are checked before the refund, so +/// they leave the escrow and the order untouched (#805) /// - Lightning invoice cancellation fails /// - Database update fails /// - Nostr publish fails +/// +/// Failures *after* the refund are logged and swallowed rather than +/// returned (DM fan-out, bond resolution), so the handler always runs to +/// the end once the escrow has moved. Making that tail atomic is #810. pub async fn admin_cancel_action( ctx: &AppContext, msg: Message, @@ -120,13 +127,44 @@ pub async fn admin_cancel_action( // and `cancel_hold_invoice` below is irreversible: running it first meant // a rejected request could still refund the seller while leaving the order // in `Dispute`, so the LN side and the DB disagreed with no way back. - // This initiator check now precedes the refund. Operations after the - // refund can still fail (dispute/order events, DB updates, DM delivery); - // making that path atomic is tracked separately in #810. + // This initiator check now precedes the refund, as does the counterparty + // resolution below. What remains after the refund either can't reject the + // call (the DM fan-out is best effort) or is a DB/event write whose + // atomicity is tracked separately in #810. let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { (true, false) => "seller", (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), + (seller_dispute, buyer_dispute) => { + // A bare `DisputeEventError` doesn't tell the operator why the + // cancel bounces. The pair is only reachable through a corrupted + // row — `dispute_action` gates on `Active`/`FiatSent`, so exactly + // one flag is set by the time an order reaches `Dispute`. + error!( + order_id = %order.id, + seller_dispute, + buyer_dispute, + "admin_cancel: ambiguous dispute initiator flags; refusing before the escrow is touched" + ); + return Err(MostroInternalErr(ServiceError::DisputeEventError)); + } + }; + + // Same reasoning as the initiator check above: resolving the + // counterparties is pure validation over the `order` snapshot, so it + // belongs before the irreversible refund rather than after it. Left + // where it was, an unparseable or missing pubkey aborted the handler + // once the escrow was already refunded and the order already + // `CanceledByAdmin` — past the point where a solver retry is possible + // (the `Dispute` guard above rejects it) and before the bond resolution + // below, stranding every `Locked` bond with no reconciler. + let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { + (Some(seller), Some(buyer)) => ( + PublicKey::from_str(seller.as_str()) + .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, + PublicKey::from_str(buyer.as_str()) + .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, + ), + (None, _) | (_, None) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), }; if order.hash.is_some() { @@ -201,27 +239,24 @@ pub async fn admin_cancel_action( let message = message .as_json() .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - // Message to admin - send_dm(event.sender, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - - let (seller_pubkey, buyer_pubkey) = match (&order.seller_pubkey, &order.buyer_pubkey) { - (Some(seller), Some(buyer)) => ( - PublicKey::from_str(seller.as_str()) - .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, - PublicKey::from_str(buyer.as_str()) - .map_err(|_| MostroInternalErr(ServiceError::InvalidPubkey))?, - ), - (None, _) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), - (_, None) => return Err(MostroInternalErr(ServiceError::InvalidPubkey)), - }; - send_dm(seller_pubkey, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; - send_dm(buyer_pubkey, my_keys, &message, None) - .await - .map_err(|e| MostroInternalErr(ServiceError::NostrError(e.to_string())))?; + // The escrow is already refunded and the order already `CanceledByAdmin` + // by this point, so a relay hiccup on a notification must not abort the + // handler: an early return here skips the bond resolution below and + // strands every `Locked` bond with no retry path (a second admin cancel + // is rejected by the `Dispute` guard, and only range maker bonds have a + // reconciler). Delivery is best effort, mirroring `notify_bond_slashed`. + for (role, destination) in [ + ("admin", event.sender), + ("seller", seller_pubkey), + ("buyer", buyer_pubkey), + ] { + if let Err(e) = send_dm(destination, my_keys, &message, None).await { + error!( + order_id = %order.id, + "admin_cancel: failed to notify the {role} of the cancellation: {e}" + ); + } + } // Phase 2: apply the solver's `BondResolution` to the bond rows // (release-by-default when absent). The buyer/seller pubkeys on @@ -534,9 +569,11 @@ mod tests { )); } - /// A dispute order with no `hash` skips the LND cancel and, when - /// neither `seller_dispute` nor `buyer_dispute` is set, fails the - /// dispute-initiator resolution with `DisputeEventError`. + /// With neither `seller_dispute` nor `buyer_dispute` set, the + /// dispute-initiator resolution fails with `DisputeEventError`. The + /// order carries no `hash`, so this covers the resolution itself + /// independently of the escrow; the pre-refund ordering is pinned by + /// `dispute_without_initiator_flag_errors_before_refunding` below. #[tokio::test] async fn dispute_without_initiator_flag_errors() { let pool = setup_pool().await; @@ -573,6 +610,12 @@ mod tests { /// irreversible `cancel_hold_invoice`. Reaching the LND seam here would /// surface as `LnNodeError` and would mean the seller was already /// refunded on a request that goes on to be rejected. + /// + /// The discriminator is the error *type*, which depends on `dead_lnd` + /// making every RPC fail: if the LND seam is ever replaced by a mock + /// that returns `Ok`, the pre-fix ordering would also end in + /// `DisputeEventError` and this test would stop guarding the invariant. + /// Assert on a cancel-call spy instead if that day comes. #[tokio::test] async fn dispute_without_initiator_flag_errors_before_refunding() { let pool = setup_pool().await; @@ -611,6 +654,84 @@ mod tests { assert_eq!(stored.status, Status::Dispute.to_string()); } + /// Both initiator flags set is as ambiguous as neither: the same arm + /// rejects it before the escrow is touched, so a corrupted row can't + /// pick an arbitrary side for the kind-38386 `initiator` tag. + #[tokio::test] + async fn dispute_with_both_initiator_flags_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + order.seller_dispute = true; + order.buyer_dispute = true; + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the LND cancel, got {result:?}" + ); + } + + /// Counterparty resolution is validation too, so a missing pubkey must + /// be rejected before the refund (#805). Pre-fix this parsed only after + /// `cancel_hold_invoice`, the dispute row and the order status had all + /// been written — a rejection that had already moved the money and left + /// the bonds `Locked` past any retry. + #[tokio::test] + async fn missing_counterparty_pubkey_errors_before_refunding() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + let mut order = dispute_order(seller, buyer); + order.seller_dispute = true; + order.buyer_pubkey = None; + order.hash = Some("11".repeat(32)); + let order = order.create(ctx.pool()).await.unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_cancel_action( + &ctx, + cancel_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroInternalErr(ServiceError::InvalidPubkey))), + "expected the pubkey check to reject before the LND cancel, got {result:?}" + ); + + // Untouched: no refund, no status change, so the row is still + // recoverable once the pubkey is repaired. + let stored = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + /// A dispute order carrying a hold-invoice `hash` returns funds to the /// seller via `cancel_hold_invoice`, which fails against the dead LND /// endpoint and surfaces as `LnNodeError`. @@ -648,9 +769,10 @@ mod tests { /// Full no-LND cancel path: a seller-initiated dispute with no hold /// invoice hash. The dispute row is moved to `SellerRefunded` and the - /// order to `CanceledByAdmin` before the DM fan-out. Those DB writes are - /// deterministic; the terminal `send_dm` depends on the process-global - /// Nostr client, so the top-level result is not asserted. + /// order to `CanceledByAdmin`, and the handler runs to completion even + /// though the DM fan-out fails here (no process-global Nostr client) — + /// notifications are best effort precisely so a relay failure can't skip + /// the bond resolution that follows them. #[tokio::test] async fn seller_dispute_without_hash_refunds_and_cancels() { let pool = setup_pool().await; @@ -669,7 +791,7 @@ mod tests { .unwrap() .id; - let _ = admin_cancel_action( + let result = admin_cancel_action( &ctx, cancel_msg(order.id), &admin_event(admin.public_key()), @@ -678,6 +800,11 @@ mod tests { ) .await; + assert!( + result.is_ok(), + "a failed DM must not abort the handler before bond resolution: {result:?}" + ); + let stored_order = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); assert_eq!(stored_order.status, Status::CanceledByAdmin.to_string()); let stored_dispute = Dispute::by_id(ctx.pool(), dispute_id) diff --git a/src/app/admin_settle.rs b/src/app/admin_settle.rs index 5dcacca5..b02240bf 100644 --- a/src/app/admin_settle.rs +++ b/src/app/admin_settle.rs @@ -91,6 +91,28 @@ pub async fn admin_settle_action( let bond_resolution = bond::extract_bond_resolution(&msg); bond::validate_bond_resolution(pool, &order, &bond_resolution).await?; + // Resolve the dispute initiator *before* the settle (#805, same class as + // the fix applied to `admin_cancel`). `settle_seller_hold_invoice` is + // irreversible: resolving the initiator afterwards meant a rejected + // request had already moved the escrow, and the early return then also + // skipped the `AdminSettled` fan-out and the bond resolution below. + let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { + (true, false) => "seller", + (false, true) => "buyer", + (seller_dispute, buyer_dispute) => { + // Only reachable through a corrupted row — `dispute_action` + // gates on `Active`/`FiatSent`, so exactly one flag is set by + // the time an order reaches `Dispute`. + error!( + order_id = %order.id, + seller_dispute, + buyer_dispute, + "admin_settle: ambiguous dispute initiator flags; refusing before the escrow is settled" + ); + return Err(MostroInternalErr(ServiceError::DisputeEventError)); + } + }; + // Settle seller hold invoice settle_seller_hold_invoice(event, ln_client, Action::AdminSettled, true, &order) .await @@ -130,13 +152,6 @@ pub async fn admin_settle_action( .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - // Get the creator of the dispute - let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) { - (true, false) => "seller", - (false, true) => "buyer", - (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)), - }; - // We create a tag to show status of the dispute let tags: Tags = Tags::from_list(vec![ Tag::custom( @@ -538,6 +553,50 @@ mod handler_tests { )); } + /// Same invariant as `admin_cancel` (#805): with the initiator flags + /// unset, the request must be rejected *before* the irreversible + /// `settle_seller_hold_invoice`. Pre-fix the initiator was resolved + /// after the settle, so this order reached the settle seam and returned + /// `LnNodeError` — the escrow moved on a request that was then rejected, + /// skipping the `AdminSettled` fan-out and the bond resolution. + #[tokio::test] + async fn dispute_without_initiator_flag_errors_before_settling() { + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let mut ln = dead_lnd().await; + let admin = Keys::generate(); + let seller = Keys::generate().public_key(); + let buyer = Keys::generate().public_key(); + + // Neither side flagged as initiator; `preimage` is left as the + // dispute-order default so the settle seam is genuinely reachable. + let order = dispute_order(seller, buyer) + .create(ctx.pool()) + .await + .unwrap(); + assign_solver(ctx.pool(), order.id, &admin.public_key()).await; + + let result = admin_settle_action( + &ctx, + settle_msg(order.id), + &admin_event(admin.public_key()), + &admin, + &mut ln, + ) + .await; + + assert!( + matches!( + result, + Err(MostroInternalErr(ServiceError::DisputeEventError)) + ), + "expected the initiator check to reject before the settle, got {result:?}" + ); + + let stored = Order::by_id(ctx.pool(), order.id).await.unwrap().unwrap(); + assert_eq!(stored.status, Status::Dispute.to_string()); + } + /// A genuine dispute settle reaches `settle_seller_hold_invoice`, which /// short-circuits on the missing preimage before any LND call and is /// mapped to `LnNodeError`. The LND settle + `do_payment` tail beyond