feat(abstract-utxo): support ZEC v6 psbt decoding - #9727
veetragjain merged 1 commit into
Conversation
|
|
9d329dc to
ea0aced
Compare
OttoAllmendinger
left a comment
There was a problem hiding this comment.
nits I can fix in a follow-up
OttoAllmendinger
left a comment
There was a problem hiding this comment.
Request changes: the stored Unified Address is not authoritative for the recipient. It lives in PSBT proprietary metadata, while the raw Orchard receiver / transparent script is the transaction truth. The builder validates this relationship at construction time, but deserialization does not revalidate it.
This creates a security and display-integrity issue: a serialized v6 PSBT can contain raw receiver R2 and preserved UA U1(R1). explainPsbtWasm currently drops ParsedOutput.script and exposes the preserved address; then parseTransaction reconstructs the actual output script from that address in toComparableOutputsWithExternal. A forged metadata value can therefore be displayed as the recipient and can make recipient verification compare U1 against U1 instead of comparing the requested recipient against R2.
Minimal fix: validate before mapping ParsedOutput to the public explanation, while address, script, and isShielded are still available. For Zcash only, pass coinName into explainPsbtWasm and require:
const recipient = output.isShielded
? zcashAddress.toShieldedReceiverWithCoin(output.address, coinName)
: zcashAddress.toTransparentReceiverWithCoin(output.address, coinName);
if (!Buffer.from(recipient).equals(Buffer.from(output.script))) {
throw new Error(`Zcash output ${i} address does not match its raw recipient`);
}Reject malformed/mismatched metadata rather than falling back to it. Also make outputScriptToAddress validate any preserved address against script (or remove the fallback), and add regression tests for mismatched v6 Orchard and v4 transparent metadata. The existing construction-time checks are not sufficient because the PSBT can be modified after serialization.
Until this raw-output check exists, the PR should not merge.
ea0aced to
e714104
Compare
OttoAllmendinger
left a comment
There was a problem hiding this comment.
The normal explainTransaction() / parseTransaction() flow now validates the raw output bytes. Thank you.
Blocking gap remains: Zec.resolveRecipientsFromPsbt() bypasses AddressCodec.isMatchingScript(). In impl/zec/zec.ts, it calls resolvePsbtRecipients(psbt, walletKeys) directly. impl/zec/recipients.ts then returns output.address / unifiedAddress from PSBT proprietary metadata alongside the raw output.script, without proving they match.
A serialized PSBT can therefore still carry raw recipient R2 and preserved UA U1(R1), and this public API will report U1 as the recipient. This applies to both v4 transparent proprietary UA metadata and v6 Orchard UA metadata. Builder-time checks do not protect this deserialization boundary.
Please pass the Zcash codec into resolvePsbtRecipients() and reject before returning any metadata:
if (!addressCodec.isMatchingScript(output)) {
throw new Error(`Output ${i} address ${output.address} does not match its raw recipient`);
}Add public-path regressions that mutate the proprietary UA only after serializing a valid v4 and v6 PSBT, then assert tzec.resolveRecipientsFromPsbt() rejects. The current isMatchingScript unit tests validate the decoder selection, but they do not cover the attacker-controlled PSBT metadata path.
Also, per the prior design discussion, keep isShielded out of the exported generic AddressCodecOutput contract. The generic codec API should only require address and raw bytes; Zcash-specific receiver selection belongs inside ZecAddressCodec.
e714104 to
3721431
Compare
OttoAllmendinger
left a comment
There was a problem hiding this comment.
The public Zec.resolveRecipientsFromPsbt() now passes this.addressCodec, and isShielded has moved into a Zcash-only type. Those are the right corrections.
The security invariant is still optional in the exported lower-level APIs:
resolvePsbtRecipients(psbt, walletKeys, addressCodec?: AddressCodec)accepts no codec and then returns proprietaryaddress/unifiedAddressmetadata without validation. This function is exported byimpl/zec/index.ts.explainTx(..., addressCodec?: AddressCodec)andexplainPsbtWasm({ addressCodec?: AddressCodec })likewise skip the check when invoked directly.
The comments describe this as compatibility for callers that only need decoded outputs, but those decoded outputs include the untrusted recipient metadata. A caller can therefore still display U1 while the committed output pays R2. Please make the validating codec required on these paths, or expose an explicitly named legacy/raw parser that cannot be mistaken for validated recipient output.
The added tests verify receiver selection and successful constructed PSBTs, but construction already enforces this relation. Add v4 transparent and v6 Orchard tests that mutate the proprietary Unified Address only after serialization, then assert both resolveRecipientsFromPsbt() and the direct resolver/explanation entry point reject. That is the deserialization boundary this change is intended to secure.
3721431 to
2ece608
Compare
2ece608 to
c236121
Compare
OttoAllmendinger
left a comment
There was a problem hiding this comment.
The required-codec changes correctly protect the normal explanation and recipient-resolution paths.
One blocking regression gap remains: the tests exercise only valid PSBTs built through the constructors and direct isMatchingScript() receiver selection. The constructors already validate the UA-to-recipient relationship, so these tests do not cover the attacker-controlled deserialization boundary that prompted this change.
Please add adversarial tests that build a valid PSBT, serialize it, then replace only the proprietary Unified Address metadata before deserializing it:
- v4 transparent: use a valid same-network UA whose transparent script differs from the committed output
scriptPubKey. - v6 shielded: use a valid same-network UA whose Orchard receiver differs from the committed PCZT recipient.
Assert that tzec.explainTransaction(), tzec.parseTransaction(), and tzec.resolveRecipientsFromPsbt() reject the payload. This verifies the invariant at the real trust boundary and guards against later changes to WASM deserialization or codec propagation silently restoring recipient-display / intent-integrity spoofing.
OttoAllmendinger
left a comment
There was a problem hiding this comment.
Non-blocking follow-up: explainTx() now requires an AddressCodec, but its descriptor-wallet branch still calls descriptor.explainPsbt(...) without forwarding or using it. descriptor.explainPsbt returns parsed output.address directly, and the Offline Vault Console is another production caller of that unvalidated descriptor helper.
This does not appear to reopen the Zcash proprietary-UA issue, since current descriptor outputs are derived from transaction scripts rather than those metadata fields. It does make the new generic address/script-validation contract inconsistent by wallet type, though. Please either wire equivalent validation through descriptor explanation and OVC, or scope the new codec argument explicitly to the fixed-script explanation path so callers are not led to expect descriptor coverage.
BREAKING CHANGE: explainTx, explainPsbtWasm and zec's resolvePsbtRecipients now require address codec to decode and verify the psbt output Ticket: CSHLD-1640
c236121 to
494d19e
Compare
Ticket: CSHLD-1640