Repository navigation
fix: never reuse an address whose public key was exposed by spending - #805
Conversation
The receive page could display an address that already had transactions:
- Only checked a single index (lastAddressIndex) and blindly displayed
index+1 when it was used, without ever verifying index+1 itself, so a
stale stored index (restored wallet, second device, server lag) let a
used address through.
- isAddressUsed failed open: every request had .catch(() => ({ is_used:
false })), so any watchtower error was treated as "unused".
- The "Auto generate address" off branch reused lastAddressIndex as-is
with no verification (kept as-is per setting semantics).
- generateNewAddress ("New Address" button / leave rotation) advanced one
index and displayed it unchecked; also had a string-concat "+ 1" bug
and never reset the generating flag on the SLP branch.
Changes in refreshDynamicAddress (auto-generate on):
- Sync lastAddressIndex from watchtower first, never regressing below the
locally tracked index.
- Scan up to 20 indexes via findUnusedAddress and only display an address
positively confirmed unused (zero balance, is_used false).
- Fail closed: if watchtower cannot confirm status, surface the existing
error notification instead of rendering the address.
- Reconcile the store index to the displayed address.
- Console.debug logs each candidate index and verdict for repro debugging.
Auto-generate off: unchanged semantics - reuse lastAddressIndex as-is,
no sync, no usage check, no index advance.
generateNewAddress: numeric index advance, unified bch/slp address-set
handling, always resets generating, then re-resolves through the verified
scan.
New getAddressUsageStatus returns 'used' | 'unused' | 'unknown' instead
of coercing failures to false.
Receiving and change addresses are generated as a pair from one index: lastAddressIndex N gives receiving 0/N and change 1/N. lastAddressIndex is the only cursor; there is no separate change-address index. Previously getChangeAddress returned the stored change address, so two consecutive spends both used 1/N. The second spend put change back onto an address whose public key the first spend had already exposed, so a quantum-capable attacker watching the mempool could derive the key and drain the funds. getChangeAddress now: - Scans the change chain from lastAddressIndex via address-discover (node get_first_use, not the local-DB /isused/, which fails open) and takes the first address with no on-chain history, so an index already spent from (auto-generate off, second device, restored wallet, lagging server index) is skipped instead of reused. - Subscribes that address to watchtower before returning. - Advances the shared cursor to N+1 as soon as the address is handed out, before broadcast, so a second spend cannot pick 1/N again while watchtower has not indexed the first change output. Indices skipped by a cancelled send are harmless. - Throws on subscription or verification failure instead of falling back to a stored change address that may already be used. Since the cursor is shared, a spend also rotates the receiving address, including when "Auto generate address" is off. With the setting off the receive page still reuses the current index for receiving; a payment is what moves the pair forward. Store guards against a lagging server index pulling the cursor back: - setWalletLastAddressAndIndex no longer moves lastAddressIndex backwards. - loadWalletAddresses takes the max of lastAddressAndIndex and lastAddressIndex. - generateNewAddressSet only overwrites the fields it is given, so the cursor can advance without clobbering the cached pair. Consolidated the duplicated change-address implementations in Connecta, marketplace order/checkout and the JPP payment panel into the shared helper. send.vue skips the helper when the user picked a change address, so an unused address does not advance the cursor. JPP passes the change address to prepareTransaction. Settings copy now states that the pair moves forward after every payment, and why. Scope: this guarantee holds only for spend paths that route through getChangeAddress (send page BCH/SLP, Connecta, marketplace order and checkout, JPP, e-load, AI-admin, Rewards redeem, Lift token). It does not yet cover call sites that omit the change address or hardcode 1/0: the watchtower-cash-js SDK then derives change from the first input key, which sends change back to the address whose public key the transaction just exposed. Those call sites are a separate follow-up: - CashInDialog, RampDepositInfo, EscrowTransfer, gifts/index, gifts/create-gift, services/wallet createFundingUtxo and consolidateUtxos (no change address passed) - marketplace/escrow/tx-builder and payment-hub/services (hardcoded 1/0) - wallet/cauldron/transact change output derived from the first input key
Seven spend paths called sendBch without a change address. The SDK then derives change from the first input key (watchtower-cash-js dist/bch/index.js), so the leftover was returned to the very address the transaction had just exposed a public key for. Pass a verified fresh change address on: - services/wallet createFundingUtxo and consolidateUtxos - card CashInDialog - ramp RampDepositInfo and EscrowTransfer - gifts create-gift and the claim/resubmit path Each call site passes isChipnet from the wallet it actually spends from rather than the store default, so the derived change address matches the network of the transaction. A mismatch there would produce an unspendable output, which is the only failure mode of this change that looks like loss. Both gift paths resolve the change address before the server POST that registers the gift, so a verification failure cannot leave a gift registered server-side but unpaid. Untouched on purpose: - marketplace/escrow/tx-builder and payment-hub/services hardcode 1/0; change already on 1/0 stays spendable because UTXO lookup is wallet-wide, so these need separate review. - Cauldron pool owner addresses (pools, add-pool, trade) are bound to a specific pkh via fetchWalletPools; moving them would make existing pools undiscoverable. Only the change output in cauldron/transact is a candidate, and that needs manual chipnet testing.
getChangeAddress read the cursor, scanned for an unused change index and committed the advance, with network round trips between each step. Two spends started at the same time could read the same index and put their change on the same address. Reserving the index before the first await is not sufficient: when the scan has to skip indices that already have history, both callers converge on the same free index further along, so the reservation alone still collides. Serialise the whole read-scan-commit per wallet type and network instead. Each caller now reads the cursor the previous caller committed. The queue chains on both settle paths, so one failed selection does not block later spends. Verified with a simulation of the same queue and scan: two concurrent callers get distinct indices with the queue (3, 4) and the same index without it (3, 3), and a rejected task does not block the next one. A failed selection still burns an index. Rolling the cursor back would reintroduce reuse if another caller had already advanced past it, and a skipped index is harmless because the scan steps over used indices and the receive page only shows a confirmed-unused address.
…p-ups Two transaction builders hardcoded change to 1/0: - marketplace/escrow/tx-builder generateEscrowFundingTransaction - payment-hub/services topUpSubscription 1/0 is a single fixed address, so every escrow funding and every subscription top-up added change to the same address, whose public key was exposed by the first spend from it. Both now use getChangeAddress, inheriting the chain verification, watchtower subscription and cursor advance. Escrow passes isChipnet from the bchWallet that funds the transaction rather than the store: the caller derives the network from the escrow contract address, so a store default could derive an address for the wrong network and produce an unspendable change output. Change already sitting on 1/0 stays spendable, since UTXO lookup is wallet-wide rather than per-cursor, so no migration is needed. Cauldron pool owner addresses are deliberately unchanged: pools are looked up by pkh from the owner address, so rotating them would make existing pools and their liquidity undiscoverable.
|
I'll provide a thorough security review of this PR focusing on the cryptographic and protocol-level implications, since this directly touches address generation, change output handling, and on-chain privacy. SummaryThe PR addresses a real and material exposure: every prior spend that returned change to Strengths
Issues worth addressing1. Receive page fallback path skips validation (Medium)
Suggested fix: have the fallback path call 2.
|
The freshness scan sent a placeholder wallet hash to watchtower's address-discover endpoint. Send the wallet's actual hash instead, and require it so a caller cannot silently fall back to a sentinel. The receive page's getAddress fallback derived an address from the stored index without any freshness check. Route it through refreshDynamicAddress so a listener or API call never attaches to an unverified address, and never substitute a personal address for a selected POS target.
prepareTransaction derived index 0 for multi-address wallets when no change address was supplied, silently reusing an address whose public key may already be exposed. Throw instead, matching the shared getChangeAddress contract that callers must pass a verified-unused address. In the marketplace, remove checkout's dead change-address lookup (escrow funding builds its own, so it only burned a wallet index per attempt) and derive the order payment's network from the funding wallet rather than the invoice prefix.
Code reviewVerified the seven commits across 22 files. Mostly secondary review; I'm pinpointing a few things the run's review missed or got wrong. Confirmed strengths
Corrections to issues 1 and 2 from the prior reviewIssue #1 (Medium, receive.vue fallback skips validation) — overstated. With Issue #2 ( Issue worth flagging that the prior review did not catch
Other notes (out-of-scope but real)
Test coverageConfirmed missing: Suggested additional testsVerdictApprove. The addressed exposure (post-spend public-key leakage through reused change addresses) is real and material, and the centralisation on |
|
Thanks — three of the four notes are now addressed in the branch, and the JPP one doesn't hold as written. Details below.
|
Problem
Spending from a P2PKH address reveals its public key on chain. Once revealed, the address is exposed to a future key-recovery attack, so any funds later sent to it can be drained. The wallet was doing this in two ways:
changeAddress = privateKeyToCashaddress(inputPrivKey).Approach
Receiving and change addresses are one pair per index.
lastAddressIndexN gives receiving0/Nand change1/N, and that single value is the only cursor. A spend advances it, which rotates both addresses; the next receive therefore also moves to a pair whose change address has never been spent from.getChangeAddressinsrc/utils/send-page-utils.jsis the single implementation: it scans the change chain for the first index with no on-chain history, subscribes the address to watchtower, advances the cursor before broadcast, and throws if it cannot confirm.Changes
Receive page (
transaction/receive.vue) — with auto-generate on, only displays an address positively confirmed unused, viaPOST /api/wallet/address-discover/(get_first_use). Auto-generate off still reuses the current pair by design.Receive page
getAddress()fallback (receive.vue:859) — the fallback used to derive an address straight from the stored index with no freshness check and no0/0guard. It now resolves throughrefreshDynamicAddress(), the same verified path the page uses on mount, so a websocket listener or API call can never attach to an unverified address. It also bails out when a POS target is selected, since POS addresses use a different index scheme and substituting a personal address would be wrong.Freshness check sends the real wallet hash (
address-generation-utils.js) — the discovery request carried a placeholder hash ('0'.repeat(64)). It now computes the actual wallet hash from the mnemonic and derivation path and requires it, so a future server-side change that keys anything onwallet_hashcannot silently attribute scanned addresses to a sentinel wallet. The response stays untrusted: non-booleanhas_historyentries are dropped and the scan aborts instead of assuming freshness.findUnusedAddressIndexnow derives the mnemonic once and passes it intoderiveAddressBatch, instead of re-reading the vault on every batch.Serialised change selection (
send-page-utils.js) —read → scan → commitspans network round trips, so concurrent spends could both settle on the same index. Now queued per wallet type and network.Consolidated call sites — Connecta, marketplace order/checkout, JPP, e-load, AI-admin, Rewards redeem and Lift token all delegate to
getChangeAddress, replacing duplicated copies. Seven paths that passed no change address now pass a verified one:createFundingUtxo,consolidateUtxos,CashInDialog,RampDepositInfo,EscrowTransfer,create-gift, and the gift resubmit flow.1/0builders —escrow/tx-builder.jsandpayment-hub/services.jshardcoded change to1/0, so every escrow funding and subscription top-up piled change onto one fixed address. Both now use the shared helper. Escrow takes the network from thebchWalletthat funds it rather than the store, since the caller derives the network from the escrow contract address and a disagreement would produce an unspendable output.No silent index-0 change for JPP (
payment-uri.js:472-481) — with no change address supplied,prepareTransactionderivedgetAddressSetAt(0).changefor multi-address wallets, reusing an address whose public key may already be exposed. It now throwsMissing change address, matching the contract that callers pass a verified-unused address. TheSingleWalletbranch keeps its own single address, since that address is the wallet.Marketplace —
checkout.vuelooked up a change address that nothing used: escrow funding builds its own insideescrow/tx-builder.js, so the lookup only burned a wallet index and subscribed a dead address on every payment attempt. Removed. Inorder.vue,sendBchPaymentnow initialises the wallet before selecting the change address and derives the network from the funding wallet (BCH_CHIPvsBCH) rather than from the invoice address prefix.Store guards — the cursor never moves backwards (
setWalletLastAddressAndIndexmonotonic,loadWalletAddressestakes the max of both index fields,generateNewAddressSetonly overwrites provided fields), so a lagging server index or a second device cannot roll it back.Settings copy — the "Auto generate address" caption now states the reason: a spent address has its public key published on chain.
What this does not cover
The guarantee applies to paths routed through
getChangeAddress. Deliberately excluded:wallet/cauldron/transact.js:195) derives its locking bytecode fromspendableCoins[0].key, so change can still land on an address just spent from — the same exposure class as the pre-PRprivateKeyToCashaddress(inputPrivKey)default. Not fixed here; it needs chipnet testing, not just unit tests.pools.vue:228,add-pool.vue:586,trade.vue:1207) must stay on0/0.fetchWalletPoolsresolves pools by pkh from the owner address, so rotating them would make existing pools and their liquidity undiscoverable.send.vue:2132-2136) still useuserSelectedChangeAddresswhen the user picked one, bypassinggetChangeAddress. BCH-only sends go through the helper. Left as-is: it is an explicit user choice, and changing it is a separate ticket.Failure behaviour
getChangeAddressfails closed: if watchtower cannot confirm freshness, the send aborts rather than proceeding with a possibly-used address. This adds oneaddress-discoverround trip per send.A failed selection burns an index instead of rolling the cursor back — rolling back would reintroduce reuse if a concurrent call had already advanced past it, and a skipped index is harmless since the scan steps over used ones.
Risk
The only loss-like failure mode is a network mismatch: deriving a change address on the other network produces an unspendable output. Two groups of call sites exist:
isChipnetfrom the wallet being spent.The second group is internally consistent rather than exempt: those paths resolve inputs, signing keys and change all through
getWalletByNetwork(), which itself reads the store, so the network cannot diverge within one transaction. Note that the compositeWalletclass carries noisChipnetof its own — the value lives onBCH/BCH_CHIP— soprops.wallet.isChipnetwould beundefinedand a no-op; an explicit override on such a path must go throughgetWalletByNetwork(wallet, 'bch').isChipnet.Gift claim and creation resolve the change address before the server POST, so a verification failure cannot leave a gift registered but unpaid.
Verification
npm run test:unit— 15 passing.