diff --git a/bridgelet-sdk-audit/postmortems/expire-account-race-window.md b/bridgelet-sdk-audit/postmortems/expire-account-race-window.md new file mode 100644 index 0000000..c336371 --- /dev/null +++ b/bridgelet-sdk-audit/postmortems/expire-account-race-window.md @@ -0,0 +1,78 @@ +# The check-then-call race window in `expireAccount()` + +**Status:** Documented / accepted risk (low) +**Area:** Bridgelet SDK — account expiry path +**Pattern:** Time-of-check to time-of-use (TOCTOU) + +--- + +## What TOCTOU is + +A **time-of-check to time-of-use** (TOCTOU) bug is a race between two steps that are not atomic: + +1. **Check** — read some state and decide an action is allowed. +2. **Use** — perform that action, assuming the check still holds. + +Anything that can change the relevant state in the gap—another request, another worker, an on-chain transaction—can invalidate the decision the check made. Classic examples include filesystem `access()` then `open()`, or “is this coupon unused?” then “redeem it” without a lock or atomic update. + +In distributed systems the gap is often unavoidable: an off-chain service checks a database or cached view, then submits a blockchain transaction that settles later. The pattern is not automatically a vulnerability; severity depends on what can happen if the world changes between check and use, and on whether the **use** step is itself safe under concurrent or repeated execution. + +--- + +## This instance: `expireAccount()` + +In the SDK expiry path, `expireAccount()` (and related helpers) typically follow a check-then-call shape: + +1. **Check** — confirm the ephemeral account looks eligible to expire (status, wall-clock or ledger-derived expiry, not already claimed/expired in local state). +2. **Call** — invoke the on-chain `expire()` entrypoint so unclaimed funds (and related reserves, where implemented) move to the recovery address. + +Between those steps, concurrent activity can intervene. Examples: + +- A recipient claim/sweep lands and completes on-chain. +- Another expiry worker or retry submits `expire()` for the same account. +- Local DB status lags behind chain state, so the check is optimistic. + +That gap is a real TOCTOU window: the check’s conclusion is not guaranteed to still be true when `expire()` executes. + +--- + +## Why this instance is likely low-risk + +On-chain `expire()` is designed to be **idempotent** (and gated by contract state): once the account is past `expiry_ledger` and still in a state that allows recovery, a successful `expire()` transitions the account into a terminal expired/recovered state. A second `expire()` after that should no-op or fail cleanly rather than move funds again. Contract-level rules also prevent treating an already-swept/claimed account as if it still held reclaimable payment funds for a second payout. + +Given that: + +| Concurrent event between check and call | Likely outcome | +| ---------------------------------------- | -------------- | +| Second `expire()` for the same account | Duplicate call is harmless if the first already succeeded; at worst a failed or no-op transaction and noisy logs/retries. | +| Claim/sweep completes first | `expire()` should reject or find nothing left to recover for the payment path; funds already went to the claim destination, which is the intended success path—not a double payout to recovery. | +| Stale “still unclaimed” local check | Chain is source of truth at use-time; a bad check may waste a transaction attempt, not invent a new entitlement. | + +So the race tends to produce **operational friction** (failed txs, retries, temporary DB/chain skew) more than **fund-loss or double-spend** scenarios, as long as `expire()` remains the authoritative, idempotent sink for recovery. + +That is **not** the same as risk-free. Residual concerns remain if, for example: + +- Off-chain bookkeeping marks an account expired or recovered before the chain confirms, and later logic trusts that mark for payouts or UX. +- Callers assume “check passed ⇒ call will succeed” and skip handling already-claimed / already-expired errors. +- Contract semantics for `expire()` change (lose idempotency, broaden who can call it, or alter recovery amounts) without revisiting this path. +- Retries amplify load or confuse monitoring without careful deduplication. + +Treat the window as **present but mitigated by on-chain semantics**, not as absent. + +--- + +## Lesson: write down “probably fine because X” + +Findings like this are easy to close with an unspoken assumption: “it’s fine—`expire()` is idempotent.” That reasoning is load-bearing. If someone later: + +- softens on-chain checks, +- adds a new off-chain side effect between check and call (webhooks, balance credits, status flips), +- or ports expiry to a path that is **not** idempotent, + +the same TOCTOU shape can become high-severity without anyone noticing the original justification disappeared. + +**Practice:** when accepting a race as low-risk, record the explicit premise in the same place as the finding—for example: + +> Probably fine **because** on-chain `expire()` is idempotent and is the sole authority for recovery transfers; the race should not create a second payout. + +Revisit that sentence whenever `expire()`, account status transitions, or the SDK’s check-then-call sequence change. Implicit safety assumptions are the ones that rot first.