fix: don't mark p2sh address as change
What the developer wrote
Authored by soralit
fix: don't mark p2sh address as change
What changed, and why it matters
This commit fixes a bug in the Keystone hardware wallet's Zcash transaction parsing. Previously, the wallet incorrectly labeled certain P2SH (pay-to-script-hash) outputs as 'change' going back to the user's own wallet, when it should not have. This could mislead a user into thinking a payment was just internal change, when it was actually going to someone else. The fix hardcodes these outputs as not-change.
Review whether the same unsafe change heuristic exists for other output types or other coins. Verify that P2SH change outputs from the wallet itself are still correctly identified through another code path, or confirm the wallet does not generate P2SH change. Consider adding UI warnings when a transaction contains P2SH outputs without a clear change label.
Security signals we found
UI deception risk: user may approve a transaction believing a P2SH output is change rather than a payment
Incorrect change detection bypasses the usual 'sending to self' safety signal
P2SH multisig/script-hash outputs cannot be reliably identified as change via seed fingerprint alone
Fix removes unsafe heuristic and replaces with conservative false
Evidence from the diff
In rust/apps/zcash/src/pczt/parse.rs, parse_transparent_output() previously computed is_change for P2SH transparent outputs by comparing the output’s BIP32 derivation seed fingerprint to the wallet’s seed fingerprint. This logic was removed and replaced with a constant false. A test was added to verify that a P2SH output with a matching seed fingerprint is never marked as change. The change suggests the prior heuristic was unsafe for P2SH because BIP32 derivation data can be present for outputs that are not actually wallet-controlled change.
Changed components
rust/apps/zcash/src/pczt/parse.rsZcash PCZT (Partially Created Zcash Transaction) transparent output parsingKeystone 3 firmware Zcash appInspect captured patch +46 / −8
diff --git a/rust/apps/zcash/src/pczt/parse.rs b/rust/apps/zcash/src/pczt/parse.rs
index be0b9f8..cb7fed5 100644
--- a/rust/apps/zcash/src/pczt/parse.rs
+++ b/rust/apps/zcash/src/pczt/parse.rs
@@ -388,17 +388,11 @@ fn parse_transparent_output(
ZcashError::InvalidPczt("missing user address for transparent output".into())
})?;
let zec_value = format_zec_value(output.value().into_u64() as f64);
- // we only consider the simple p2sh script at the moment. multisig is not considered;
- let is_change = output
- .bip32_derivation()
- .first_key_value()
- .map(|(_, derivation)| seed_fingerprint == derivation.seed_fingerprint())
- .unwrap_or(false);
Ok(ParsedTo::new(
address,
zec_value,
output.value().into_u64(),
- is_change,
+ false,
false,
None,
))
@@ -683,11 +677,45 @@ fn decode_memo(memo_bytes: [u8; 512]) -> Option<String> {
#[cfg(feature = "cypherpunk")]
#[cfg(test)]
mod tests {
+ use alloc::collections::BTreeMap;
use super::*;
- use zcash_vendor::zcash_protocol::consensus::MAIN_NETWORK;
+ use zcash_vendor::{
+ transparent::pczt,
+ zcash_address::ZcashAddress,
+ zcash_protocol::consensus::{Parameters, MAIN_NETWORK},
+ };
extern crate std;
+ fn p2sh_output_with_matching_seed_fingerprint(
+ seed_fingerprint: [u8; 32],
+ ) -> transparent::pczt::Output {
+ let hash = [0x11; 20];
+ let script_pubkey = {
+ let mut script = vec![0xa9, 0x14];
+ script.extend_from_slice(&hash);
+ script.push(0x87);
+ script
+ };
+ let user_address =
+ ZcashAddress::from_transparent_p2sh(MAIN_NETWORK.network_type(), hash).encode();
+ let mut bip32_derivation = BTreeMap::new();
+ bip32_derivation.insert(
+ [0x02; 33],
+ pczt::Bip32Derivation::parse(seed_fingerprint, vec![0]).unwrap(),
+ );
+
+ pczt::Output::parse(
+ 42_000,
+ script_pubkey,
+ Some(vec![0x51]),
+ bip32_derivation,
+ Some(user_address),
+ BTreeMap::new(),
+ )
+ .unwrap()
+ }
+
#[test]
fn test_format_zec_value() {
let value = 10000;
@@ -779,6 +807,16 @@ mod tests {
assert_eq!(result.get_fee_value(), "0.0002 ZEC");
}
+ #[test]
+ fn test_parse_p2sh_output_is_never_marked_as_change() {
+ let seed_fingerprint = [0x22; 32];
+ let output = p2sh_output_with_matching_seed_fingerprint(seed_fingerprint);
+
+ let parsed = parse_transparent_output(&seed_fingerprint, &output).unwrap();
+
+ assert!(!parsed.get_is_change());
+ }
+
#[test]
fn test_decode_memo_with_hex_content() {
{
Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.