Skip to content

SplicePendingInbox HashMap loses concurrent splice events, leaks stale outpoints #63

@benthecarman

Description

@benthecarman

Severity: MEDIUM · CWE: CWE-362
Location: orange-sdk/src/lightning_wallet.rs:58-68
Scanner: llm-code-review · Fingerprint: afa3147299d3ff77…

Description

SplicePendingInbox is documented as "a queue rather than a watch so each consumer takes its own event", but it is implemented as a Mutex<HashMap<u128, OutPoint>> keyed by user_channel_id. deliver() calls HashMap::insert, which silently overwrites any pending entry for the same channel.

Both splice-in (splice_all_into_channel / rebalancer) and splice-out (pay() -> splice_out) target the same LSP channel and therefore share the same user_channel_id. The handler in event.rs deliver()s every SplicePending into this inbox before either consumer awakens. When the two operations race:

  1. Rebalancer initiates splice-in on channel C; SplicePending for the splice-in arrives -> deliver(C, txo_in).
  2. pay() initiates splice-out on channel C; SplicePending for the splice-out arrives before either await runs -> deliver(C, txo_out); txo_in is permanently lost.
  3. The first awaiter (either rebalancer or pay) wins the lock and consumes txo_out, regardless of which splice it initiated.
  4. The other awaiter blocks forever because no event remains.

Impact: the splice-out's PaymentId(funding_txo.txid) and the persisted PaymentDetails.txid (line 374-394) are recorded against the wrong on-chain transaction (potentially an inbound splice-in tx the user did not authorize as their outbound payment), corrupting splice_out storage; meanwhile the racing splice can hang the calling task indefinitely. A leftover entry from any earlier splice whose await was skipped (cancelled task, error path) is similarly consumed by the next await_splice_pending instead of waiting for the new event. The regression test demonstrates that two deliveries for the same channel id drop the first event, breaking the documented "queue" contract.

CWE-362 (race) / CWE-672 (use of resource after expiration/release).

Proof of concept (regression test)

--- a/orange-sdk/src/lightning_wallet.rs
+++ b/orange-sdk/src/lightning_wallet.rs
@@ -606,4 +606,33 @@ impl From<&PaymentDetails> for PaymentType {
 			(_, false) => PaymentType::IncomingLightning {},
 		}
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use ldk_node::bitcoin::Txid;
+
+	fn dummy_outpoint(seed: u8) -> OutPoint {
+		let bytes = [seed; 32];
+		OutPoint { txid: Txid::from_byte_array(bytes), vout: seed as u32 }
+	}
+
+	#[test]
+	fn splice_pending_inbox_preserves_distinct_events() {
+		let inbox = SplicePendingInbox {
+			pending: Mutex::new(HashMap::new()),
+			notify: Notify::new(),
+		};
+		let channel_id: u128 = 7;
+		let first = dummy_outpoint(0xaa);
+		let second = dummy_outpoint(0xbb);
+
+		inbox.deliver(channel_id, first);
+		inbox.deliver(channel_id, second);
+
+		let got_first = inbox.pending.lock().unwrap().remove(&channel_id);
+		let got_second = inbox.pending.lock().unwrap().remove(&channel_id);
+
+		assert_eq!(got_first, Some(first), "first delivered SplicePending must not be overwritten");
+		assert_eq!(got_second, Some(second), "second delivered SplicePending must still be retrievable");
+	}
+}

Reported by loupe scan, finding #3 (repo 1, job 3)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions