Skip to content

fix(vault): harden pending pegin localStorage validation#1406

Open
jeremy-babylonlabs wants to merge 1 commit intomainfrom
fix/pegin-storage-integrity
Open

fix(vault): harden pending pegin localStorage validation#1406
jeremy-babylonlabs wants to merge 1 commit intomainfrom
fix/pegin-storage-integrity

Conversation

@jeremy-babylonlabs
Copy link
Copy Markdown
Contributor

Summary

Closes audit finding #37 (MEDIUM): unsigned tx hex stored in localStorage was passed to the UTXO reservation and broadcast paths without integrity checks, leaving them vulnerable to tampering by XSS, browser extensions, or manual devtools edits.

Changes

storage/peginStorage.ts

Strict shape/format validation at the read boundary. Each entry must pass:

  • id and peginTxHash — 32-byte hex (0x-prefix optional for legacy entries).
  • timestamp — finite non-negative number.
  • unsignedTxHex — empty (cross-device marker) or non-empty even-byte hex; rejects bare 0x and odd lengths.
  • Every selectedUTXOs entry — 64-char hex txid, integer vout ≥ 0, value in safe integer range, even-byte scriptPubKey.

Invalid entries are dropped individually with a logger.warn — the storage key is no longer wiped from a single bad record. Also closes a DoS where a non-string id would throw inside normalizeTransactionId and clear all pending pegins.

services/vault/utxoReservation.ts

  • collectReservedUtxoRefs now derives reservation refs only from unsignedTxHex. selectedUTXOs is no longer trusted for reservation, eliminating the sidecar inconsistency window.
  • Pending pegins whose id matches an indexed on-chain vault are skipped so the trusted indexer copy wins.
  • extractInputUtxoRefs logs on parse failure instead of silently returning an empty array (per CLAUDE.md's "never swallow errors silently").

Tests

  • New storage/__tests__/peginStorage.test.ts (20 tests): DoS guard, legacy non-prefixed ids, bare 0x, odd-length hex, wrong-length txid, wrong-length id/peginTxHash, malformed scriptPubKey, mixed valid/tampered batches.
  • services/vault/__tests__/utxoReservation.test.ts updated for the simpler reservation logic plus new on-chain preference cases.

Residual risk (documented, out of scope for this PR)

  • collectReservedUtxoRefs still derives refs from a localStorage-sourced unsignedTxHex after only a format check. A tampered-but-parseable hex could reserve UTXOs the attacker chose, causing selectUtxosForDeposit (from fix(vault): utxo reservation - prevent double spend failures #1398) to refuse a new deposit — a DoS that requires XSS / browser-extension write access. Fully closing it means passing vaults from the indexer into collectReservedUtxoRefs so indexed pegins use canonical on-chain data; that's a call-site change in useMultiVaultDepositFlow.ts needing useVaults wired in. Worth a follow-up.
  • useVaultActions.handleBroadcast still passes localStorage selectedUTXOs to utxosToExpectedRecord as trusted prevout data. Tampering causes P2TR signing failure (BIP 341 commits prevouts), not a silent exploit — fails loud, no funds at risk.

Test plan

  • pnpm --filter vault exec vitest run — 1055 passed, 4 skipped, 65 files
  • pnpm --filter vault exec eslint <changed files> — 0 errors, 0 new warnings
  • pnpm --filter vault exec tsc -b --noEmit tsconfig.lib.json — clean
  • pnpm run build — all 7 projects + 1 dependent task green

Closes audit finding #37 (MEDIUM): unsigned tx hex stored in localStorage
was passed to UTXO reservation and broadcast paths without integrity
checks, leaving them vulnerable to tampering by XSS, browser extensions,
or manual devtools edits.

peginStorage.ts: strict shape/format validation at the read boundary.
Each entry must pass checks for `id` and `peginTxHash` (32-byte hex),
`timestamp` (finite non-negative number), `unsignedTxHex` (empty or
non-empty even-byte hex; rejects bare `0x` and odd lengths), and every
selectedUTXOs field (64-char hex txid, integer vout >= 0, value in safe
integer range, even-byte scriptPubKey). Invalid entries are dropped per
entry with a logger.warn — the storage key is no longer wiped from a
single bad record. Also closes a DoS where a non-string id would throw
inside normalizeTransactionId and clear all pending pegins.

utxoReservation.ts: collectReservedUtxoRefs now derives reservation refs
only from unsignedTxHex (selectedUTXOs is no longer trusted for
reservation, eliminating the sidecar inconsistency window). Pending
pegins whose id matches an indexed on-chain vault are skipped so the
trusted indexer copy wins. extractInputUtxoRefs logs on parse failure
instead of silently returning an empty array.

Adds peginStorage.test.ts (20 tests) covering the validation matrix:
DoS guard, legacy non-prefixed ids, bare `0x`, odd-length hex,
wrong-length txid, wrong-length id and peginTxHash, malformed
scriptPubKey, mixed valid/tampered batches. utxoReservation.test.ts
updated to reflect the simpler reservation logic and added on-chain
preference cases.
@github-actions
Copy link
Copy Markdown

🔐 Commit Signature Verification

All 1 commit(s) passed verification

Commit Author Signature Key Type Key Check
93653af60e35 jeremy-babylonlabs sk-ssh-ed25519

Summary

  • Commits verified: 1
  • Signature check: ✅ All passed
  • Key type enforcement: ✅ All sk-ssh-ed25519

Required key type: sk-ssh-ed25519 (FIDO2 hardware key)

Last verified: 2026-04-15 10:45 UTC

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This PR closes audit finding #37 by hardening the localStorage read boundary for pending pegins: it adds hasValidSecurityFields to drop tampered or malformed entries individually, and changes collectReservedUtxoRefs to derive refs exclusively from unsignedTxHex instead of the untrusted selectedUTXOs sidecar. On-chain vault data is now treated as canonical when a vault ID match is found, preventing localStorage from poisoning the reservation set post-indexing. Tests cover the key edge cases (DoS guard, legacy non-prefixed IDs, bare 0x, odd-length hex, mixed valid/tampered batches).

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style suggestions with no impact on the security guarantees introduced.

The core security improvements (strict validation at the read boundary, dropping untrusted selectedUTXOs from reservation, canonical on-chain preference) are correct and well-tested. The only open finding is a P2 normalization gap for peginTxHash that is pre-existing in character and unlikely to affect real data since peginTxHash has been typed and written as Hex throughout. P2 findings do not reduce the score.

services/vault/src/storage/peginStorage.ts — minor peginTxHash normalization gap at the validated-but-not-normalized path.

Important Files Changed

Filename Overview
services/vault/src/storage/peginStorage.ts Adds strict shape/format validation at the localStorage read boundary; one minor gap: peginTxHash is accepted in legacy no-prefix form by BYTES32_HEX_RE but is not normalized like id is, risking a silent Hex-type contract violation for legacy entries.
services/vault/src/services/vault/utxoReservation.ts Drops selectedUTXOs sidecar from reservation logic; skips localStorage entries when a matching on-chain vault is found; adds warn logging on parse failure. Logic is clean and correct.
services/vault/src/storage/tests/peginStorage.test.ts New test file with 20 cases covering the full validation surface: DoS guard, legacy IDs, bare 0x, odd-length hex, wrong-length IDs, malformed UTXOs, mixed batches. Coverage is thorough.
services/vault/src/services/vault/tests/utxoReservation.test.ts Updated to reflect the new reservation-from-txHex logic; adds cases for on-chain preference (PENDING and ACTIVE vault status) and parse-failure logging. Well-structured with clear named constants.

Sequence Diagram

sequenceDiagram
    participant LS as localStorage
    participant PS as peginStorage.getPendingPegins
    participant V as hasValidSecurityFields
    participant CR as collectReservedUtxoRefs
    participant EI as extractInputUtxoRefs
    participant IDX as On-chain Vaults (indexer)

    PS->>LS: JSON.parse(stored)
    LS-->>PS: raw unknown[]
    PS->>V: hasValidSecurityFields(entry)
    alt invalid shape / tampered hex
        V-->>PS: false → logger.warn, skip entry
    else valid
        V-->>PS: true → normalize id (0x prefix)
    end
    PS-->>CR: PendingPeginRequest[]

    CR->>IDX: vaults[] (canonical)
    IDX-->>CR: Vault[] with unsignedPrePeginTx

    loop each pending pegin
        alt id matches on-chain vault ID
            CR->>CR: skip localStorage entry entirely
        else not yet indexed
            CR->>EI: extractInputUtxoRefs(unsignedTxHex)
            alt parse failure
                EI-->>CR: [] + logger.warn
            else success
                EI-->>CR: UtxoRef[]
            end
        end
    end

    loop each PENDING/VERIFIED vault
        CR->>EI: extractInputUtxoRefs(unsignedPrePeginTx)
        EI-->>CR: UtxoRef[]
    end

    CR-->>CR: return reserved UtxoRef[]
Loading

Comments Outside Diff (1)

  1. services/vault/src/storage/peginStorage.ts, line 196-201 (link)

    P2 peginTxHash validated but not normalized

    BYTES32_HEX_RE explicitly allows the legacy no-prefix form for both id and peginTxHash, but only id is run through normalizeTransactionId in the map below. If a stored entry has a non-prefixed peginTxHash (e.g. "3333...3333" instead of "0x3333...3333"), it passes validation and is returned with the wrong shape for the Hex type. Any downstream code that compares or concatenates peginTxHash as a 0x-prefixed hash would silently operate on the wrong value.

Reviews (1): Last reviewed commit: "fix(vault): harden pending pegin localSt..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant