From 0880453f827627aea263c6c28232169c2702c166 Mon Sep 17 00:00:00 2001 From: Justin Berman Date: Mon, 19 Feb 2024 17:45:50 -0800 Subject: [PATCH] monero: make dummy payment ID zeroes when it's included in a tx (#514) * monero: make dummy payment ID zeroes when it's included in a tx Also did some minor cleaning of InternalPayment::Change * Lint * Clarify comment --- coins/monero/src/wallet/scan.rs | 35 +++-- coins/monero/src/wallet/send/mod.rs | 135 +++++++++++--------- coins/monero/src/wallet/send/multisig.rs | 12 +- coins/monero/tests/scan.rs | 17 ++- coins/monero/tests/wallet2_compatibility.rs | 14 +- 5 files changed, 129 insertions(+), 84 deletions(-) diff --git a/coins/monero/src/wallet/scan.rs b/coins/monero/src/wallet/scan.rs index d32dc6fb6..c8ed09956 100644 --- a/coins/monero/src/wallet/scan.rs +++ b/coins/monero/src/wallet/scan.rs @@ -101,10 +101,18 @@ pub struct Metadata { /// The subaddress this output was sent to. pub subaddress: Option, /// The payment ID included with this output. - /// This will be gibberish if the payment ID wasn't intended for the recipient or wasn't included. - // Could be an Option, as extra doesn't necessarily have a payment ID, yet all Monero TXs should - // have this making it simplest for it to be as-is. - pub payment_id: [u8; 8], + /// There are 2 circumstances in which the reference wallet2 ignores the payment ID + /// but the payment ID will be returned here anyway: + /// + /// 1) If the payment ID is tied to an output received by a subaddress account + /// that spent Monero in the transaction (the received output is considered + /// "change" and is not considered a "payment" in this case). If there are multiple + /// spending subaddress accounts in a transaction, the highest index spent key image + /// is used to determine the spending subaddress account. + /// + /// 2) If the payment ID is the unencrypted variant and the block's hf version is + /// v12 or higher (https://github.com/serai-dex/serai/issues/512) + pub payment_id: Option, /// Arbitrary data encoded in TX extra. pub arbitrary_data: Vec>, } @@ -114,7 +122,7 @@ impl core::fmt::Debug for Metadata { fmt .debug_struct("Metadata") .field("subaddress", &self.subaddress) - .field("payment_id", &hex::encode(self.payment_id)) + .field("payment_id", &self.payment_id) .field("arbitrary_data", &self.arbitrary_data.iter().map(hex::encode).collect::>()) .finish() } @@ -129,7 +137,13 @@ impl Metadata { } else { w.write_all(&[0])?; } - w.write_all(&self.payment_id)?; + + if let Some(payment_id) = self.payment_id { + w.write_all(&[1])?; + payment_id.write(w)?; + } else { + w.write_all(&[0])?; + } w.write_all(&u32::try_from(self.arbitrary_data.len()).unwrap().to_le_bytes())?; for part in &self.arbitrary_data { @@ -157,7 +171,7 @@ impl Metadata { Ok(Metadata { subaddress, - payment_id: read_bytes(r)?, + payment_id: if read_byte(r)? == 1 { PaymentId::read(r).ok() } else { None }, arbitrary_data: { let mut data = vec![]; for _ in 0 .. read_u32(r)? { @@ -377,12 +391,7 @@ impl Scanner { o, ); - let payment_id = - if let Some(PaymentId::Encrypted(id)) = payment_id.map(|id| id ^ payment_id_xor) { - id - } else { - payment_id_xor - }; + let payment_id = payment_id.map(|id| id ^ payment_id_xor); if let Some(actual_view_tag) = output.view_tag { if actual_view_tag != view_tag { diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index 9553d1877..ff4cfd728 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -69,16 +69,23 @@ impl SendOutput { #[allow(non_snake_case)] fn internal( unique: [u8; 32], - output: (usize, (MoneroAddress, u64)), + output: (usize, (MoneroAddress, u64), bool), ecdh: EdwardsPoint, R: EdwardsPoint, ) -> (SendOutput, Option<[u8; 8]>) { let o = output.0; + let need_dummy_payment_id = output.2; let output = output.1; let (view_tag, shared_key, payment_id_xor) = shared_key(Some(unique).filter(|_| output.0.is_guaranteed()), ecdh, o); + let payment_id = output + .0 + .payment_id() + .or(if need_dummy_payment_id { Some([0u8; 8]) } else { None }) + .map(|id| (u64::from_le_bytes(id) ^ u64::from_le_bytes(payment_id_xor)).to_le_bytes()); + ( SendOutput { R, @@ -87,17 +94,14 @@ impl SendOutput { commitment: Commitment::new(commitment_mask(shared_key), output.1), amount: amount_encryption(output.1, shared_key), }, - output - .0 - .payment_id() - .map(|id| (u64::from_le_bytes(id) ^ u64::from_le_bytes(payment_id_xor)).to_le_bytes()), + payment_id, ) } fn new( r: &Zeroizing, unique: [u8; 32], - output: (usize, (MoneroAddress, u64)), + output: (usize, (MoneroAddress, u64), bool), ) -> (SendOutput, Option<[u8; 8]>) { let address = output.1 .0; SendOutput::internal( @@ -115,7 +119,7 @@ impl SendOutput { fn change( ecdh: EdwardsPoint, unique: [u8; 32], - output: (usize, (MoneroAddress, u64)), + output: (usize, (MoneroAddress, u64), bool), ) -> (SendOutput, Option<[u8; 8]>) { SendOutput::internal(unique, output, ecdh, ED25519_BASEPOINT_POINT) } @@ -291,8 +295,8 @@ impl FeePriority { #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] pub(crate) enum InternalPayment { - Payment((MoneroAddress, u64)), - Change((MoneroAddress, Option>), u64), + Payment((MoneroAddress, u64), bool), + Change((MoneroAddress, u64), Option>), } /// The eventual output of a SignableTransaction. @@ -352,6 +356,14 @@ impl Change { /// Create a fingerprintable change output specification which will harm privacy. Only use this /// if you know what you're doing. + /// + /// If the change address is None, there are 2 potential fingerprints: + /// + /// 1) The change in the tx is shunted to the fee (fingerprintable fee). + /// + /// 2) If there are 2 outputs in the tx, there would be no payment ID as is the case when the + /// reference wallet creates 2 output txs, since monero-serai doesn't know which output + /// to tie the dummy payment ID to. pub fn fingerprintable(address: Option) -> Change { Change { address, view: None } } @@ -362,9 +374,9 @@ fn need_additional(payments: &[InternalPayment]) -> (bool, bool) { let subaddresses = payments .iter() .filter(|payment| match *payment { - InternalPayment::Payment(payment) => payment.0.is_subaddress(), - InternalPayment::Change(change, _) => { - if change.1.is_some() { + InternalPayment::Payment(payment, _) => payment.0.is_subaddress(), + InternalPayment::Change(change, change_view) => { + if change_view.is_some() { has_change_view = true; // It should not be possible to construct a change specification to a subaddress with a // view key @@ -391,7 +403,7 @@ fn sanity_check_change_payment_quantity(payments: &[InternalPayment], has_change payments .iter() .filter(|payment| match *payment { - InternalPayment::Payment(_) => false, + InternalPayment::Payment(_, _) => false, InternalPayment::Change(_, _) => true, }) .count(), @@ -463,6 +475,13 @@ impl SignableTransaction { Err(TransactionError::NoChange)?; } + // All 2 output txs created by the reference wallet have payment IDs to avoid + // fingerprinting integrated addresses. Note: we won't create a dummy payment + // ID if we create a 0-change 2-output tx since we don't know which output should + // receive the payment ID and such a tx is fingerprintable to monero-serai anyway + let need_dummy_payment_id = !has_payment_id && payments.len() == 1; + has_payment_id |= need_dummy_payment_id; + // Get the outgoing amount ignoring fees let out_amount = payments.iter().map(|payment| payment.1).sum::(); @@ -472,19 +491,21 @@ impl SignableTransaction { } // Collect payments in a container that includes a change output if a change address is provided - let mut payments = payments.into_iter().map(InternalPayment::Payment).collect::>(); + let mut payments = payments + .into_iter() + .map(|payment| InternalPayment::Payment(payment, need_dummy_payment_id)) + .collect::>(); + debug_assert!(!need_dummy_payment_id || (payments.len() == 1 && change.address.is_some())); + if let Some(change_address) = change.address.as_ref() { // Push a 0 amount change output that we'll use to do fee calculations. // We'll modify the change amount after calculating the fee - payments.push(InternalPayment::Change((*change_address, change.view.clone()), 0)); + payments.push(InternalPayment::Change((*change_address, 0), change.view.clone())); } // Determine if we'll need additional pub keys in tx extra let (_, additional) = need_additional(&payments); - // Add a dummy payment ID if there's only 2 payments - has_payment_id |= outputs == 2; - // Calculate the extra length let extra = Extra::fee_weight(outputs, additional, has_payment_id, data.as_ref()); @@ -524,8 +545,8 @@ impl SignableTransaction { let change_payment = payments.last_mut().unwrap(); debug_assert!(matches!(change_payment, InternalPayment::Change(_, _))); *change_payment = InternalPayment::Change( - (*change_address, change.view.clone()), - in_amount - out_amount - fee, + (*change_address, in_amount - out_amount - fee), + change.view.clone(), ); } @@ -538,8 +559,8 @@ impl SignableTransaction { payments .iter() .map(|payment| match *payment { - InternalPayment::Payment(payment) => payment.1, - InternalPayment::Change(_, amount) => amount, + InternalPayment::Payment(payment, _) => payment.1, + InternalPayment::Change(change, _) => change.1, }) .sum::() + fee, @@ -606,7 +627,7 @@ impl SignableTransaction { if modified_change_ecdh { for payment in &*payments { match payment { - InternalPayment::Payment(payment) => { + InternalPayment::Payment(payment, _) => { // This should be the only payment and it should be a subaddress debug_assert!(payment.0.is_subaddress()); tx_public_key = tx_key.deref() * payment.0.spend; @@ -625,8 +646,8 @@ impl SignableTransaction { // Downcast the change output to a payment output if it doesn't require special handling // regarding it's view key payment = if !modified_change_ecdh { - if let InternalPayment::Change(change, amount) = &payment { - InternalPayment::Payment((change.0, *amount)) + if let InternalPayment::Change(change, _) = &payment { + InternalPayment::Payment(*change, false) } else { payment } @@ -635,13 +656,14 @@ impl SignableTransaction { }; let (output, payment_id) = match payment { - InternalPayment::Payment(payment) => { + InternalPayment::Payment(payment, need_dummy_payment_id) => { // If this is a subaddress, generate a dedicated r. Else, reuse the TX key let dedicated = Zeroizing::new(random_scalar(&mut rng)); let use_dedicated = additional && payment.0.is_subaddress(); let r = if use_dedicated { &dedicated } else { &tx_key }; - let (mut output, payment_id) = SendOutput::new(r, uniqueness, (o, payment)); + let (mut output, payment_id) = + SendOutput::new(r, uniqueness, (o, payment, need_dummy_payment_id)); if modified_change_ecdh { debug_assert_eq!(tx_public_key, output.R); } @@ -655,11 +677,11 @@ impl SignableTransaction { } (output, payment_id) } - InternalPayment::Change(change, amount) => { + InternalPayment::Change(change, change_view) => { // Instead of rA, use Ra, where R is r * subaddress_spend_key // change.view must be Some as if it's None, this payment would've been downcast - let ecdh = tx_public_key * change.1.unwrap().deref(); - SendOutput::change(ecdh, uniqueness, (o, (change.0, amount))) + let ecdh = tx_public_key * change_view.unwrap().deref(); + SendOutput::change(ecdh, uniqueness, (o, change, false)) } }; @@ -667,16 +689,6 @@ impl SignableTransaction { id = id.or(payment_id); } - // Include a random payment ID if we don't actually have one - // It prevents transactions from leaking if they're sending to integrated addresses or not - // Only do this if we only have two outputs though, as Monero won't add a dummy if there's - // more than two outputs - if outputs.len() <= 2 { - let mut rand = [0; 8]; - rng.fill_bytes(&mut rand); - id = id.or(Some(rand)); - } - (tx_public_key, additional_keys, outputs, id) } @@ -949,21 +961,26 @@ impl Eventuality { fn write_payment(payment: &InternalPayment, w: &mut W) -> io::Result<()> { match payment { - InternalPayment::Payment(payment) => { + InternalPayment::Payment(payment, need_dummy_payment_id) => { w.write_all(&[0])?; write_vec(write_byte, payment.0.to_string().as_bytes(), w)?; - w.write_all(&payment.1.to_le_bytes()) + w.write_all(&payment.1.to_le_bytes())?; + if *need_dummy_payment_id { + w.write_all(&[1]) + } else { + w.write_all(&[0]) + } } - InternalPayment::Change(change, amount) => { + InternalPayment::Change(change, change_view) => { w.write_all(&[1])?; write_vec(write_byte, change.0.to_string().as_bytes(), w)?; - if let Some(view) = change.1.as_ref() { + w.write_all(&change.1.to_le_bytes())?; + if let Some(view) = change_view.as_ref() { w.write_all(&[1])?; - write_scalar(view, w)?; + write_scalar(view, w) } else { - w.write_all(&[0])?; + w.write_all(&[0]) } - w.write_all(&amount.to_le_bytes()) } } } @@ -988,17 +1005,21 @@ impl Eventuality { fn read_payment(r: &mut R) -> io::Result { Ok(match read_byte(r)? { - 0 => InternalPayment::Payment((read_address(r)?, read_u64(r)?)), + 0 => InternalPayment::Payment( + (read_address(r)?, read_u64(r)?), + match read_byte(r)? { + 0 => false, + 1 => true, + _ => Err(io::Error::other("invalid need additional"))?, + }, + ), 1 => InternalPayment::Change( - ( - read_address(r)?, - match read_byte(r)? { - 0 => None, - 1 => Some(Zeroizing::new(read_scalar(r)?)), - _ => Err(io::Error::other("invalid change payment"))?, - }, - ), - read_u64(r)?, + (read_address(r)?, read_u64(r)?), + match read_byte(r)? { + 0 => None, + 1 => Some(Zeroizing::new(read_scalar(r)?)), + _ => Err(io::Error::other("invalid change view"))?, + }, ), _ => Err(io::Error::other("invalid payment"))?, }) diff --git a/coins/monero/src/wallet/send/multisig.rs b/coins/monero/src/wallet/send/multisig.rs index f3c437e56..02626e6a7 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/coins/monero/src/wallet/send/multisig.rs @@ -120,16 +120,20 @@ impl SignableTransaction { for payment in &self.payments { match payment { - InternalPayment::Payment(payment) => { + InternalPayment::Payment(payment, need_dummy_payment_id) => { transcript.append_message(b"payment_address", payment.0.to_string().as_bytes()); transcript.append_message(b"payment_amount", payment.1.to_le_bytes()); + transcript.append_message( + b"need_dummy_payment_id", + [if *need_dummy_payment_id { 1u8 } else { 0u8 }], + ); } - InternalPayment::Change(change, amount) => { + InternalPayment::Change(change, change_view) => { transcript.append_message(b"change_address", change.0.to_string().as_bytes()); - if let Some(view) = change.1.as_ref() { + transcript.append_message(b"change_amount", change.1.to_le_bytes()); + if let Some(view) = change_view.as_ref() { transcript.append_message(b"change_view_key", Zeroizing::new(view.to_bytes())); } - transcript.append_message(b"change_amount", amount.to_le_bytes()); } } } diff --git a/coins/monero/tests/scan.rs b/coins/monero/tests/scan.rs index 0938b95b1..3e9c9069e 100644 --- a/coins/monero/tests/scan.rs +++ b/coins/monero/tests/scan.rs @@ -1,6 +1,9 @@ use rand::RngCore; -use monero_serai::{transaction::Transaction, wallet::address::SubaddressIndex}; +use monero_serai::{ + transaction::Transaction, + wallet::{address::SubaddressIndex, extra::PaymentId}, +}; mod runner; @@ -16,6 +19,8 @@ test!( |_, tx: Transaction, _, mut state: Scanner| async move { let output = state.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); + let dummy_payment_id = PaymentId::Encrypted([0u8; 8]); + assert_eq!(output.metadata.payment_id, Some(dummy_payment_id)); }, ), ); @@ -57,7 +62,7 @@ test!( |_, tx: Transaction, _, mut state: (Scanner, [u8; 8])| async move { let output = state.0.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); - assert_eq!(output.metadata.payment_id, state.1); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(state.1))); }, ), ); @@ -140,7 +145,7 @@ test!( |_, tx: Transaction, _, mut state: (Scanner, [u8; 8])| async move { let output = state.0.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); - assert_eq!(output.metadata.payment_id, state.1); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(state.1))); }, ), ); @@ -174,7 +179,7 @@ test!( |_, tx: Transaction, _, mut state: (Scanner, [u8; 8], SubaddressIndex)| async move { let output = state.0.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); - assert_eq!(output.metadata.payment_id, state.1); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(state.1))); assert_eq!(output.metadata.subaddress, Some(state.2)); }, ), @@ -259,7 +264,7 @@ test!( |_, tx: Transaction, _, mut state: (Scanner, [u8; 8])| async move { let output = state.0.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); - assert_eq!(output.metadata.payment_id, state.1); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(state.1))); }, ), ); @@ -293,7 +298,7 @@ test!( |_, tx: Transaction, _, mut state: (Scanner, [u8; 8], SubaddressIndex)| async move { let output = state.0.scan_transaction(&tx).not_locked().swap_remove(0); assert_eq!(output.commitment().amount, 5); - assert_eq!(output.metadata.payment_id, state.1); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(state.1))); assert_eq!(output.metadata.subaddress, Some(state.2)); }, ), diff --git a/coins/monero/tests/wallet2_compatibility.rs b/coins/monero/tests/wallet2_compatibility.rs index f308a6ec3..9f9892f87 100644 --- a/coins/monero/tests/wallet2_compatibility.rs +++ b/coins/monero/tests/wallet2_compatibility.rs @@ -10,7 +10,7 @@ use monero_serai::{ rpc::{EmptyResponse, HttpRpc, Rpc}, wallet::{ address::{Network, AddressSpec, SubaddressIndex, MoneroAddress}, - extra::{MAX_TX_EXTRA_NONCE_SIZE, Extra}, + extra::{MAX_TX_EXTRA_NONCE_SIZE, Extra, PaymentId}, Scanner, }, }; @@ -113,13 +113,17 @@ async fn from_wallet_rpc_to_self(spec: AddressSpec) { // runner::check_weight_and_fee(&tx, fee_rate); match spec { - AddressSpec::Subaddress(index) => assert_eq!(output.metadata.subaddress, Some(index)), + AddressSpec::Subaddress(index) => { + assert_eq!(output.metadata.subaddress, Some(index)); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted([0u8; 8]))); + } AddressSpec::Integrated(payment_id) => { - assert_eq!(output.metadata.payment_id, payment_id); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted(payment_id))); assert_eq!(output.metadata.subaddress, None); } AddressSpec::Standard | AddressSpec::Featured { .. } => { - assert_eq!(output.metadata.subaddress, None) + assert_eq!(output.metadata.subaddress, None); + assert_eq!(output.metadata.payment_id, Some(PaymentId::Encrypted([0u8; 8]))); } } assert_eq!(output.commitment().amount, 1000000000000); @@ -181,6 +185,7 @@ test!( .unwrap(); assert_eq!(transfer.transfer.subaddr_index, Index { major: 0, minor: 0 }); assert_eq!(transfer.transfer.amount, 1000000); + assert_eq!(transfer.transfer.payment_id, hex::encode([0u8; 8])); }, ), ); @@ -218,6 +223,7 @@ test!( .unwrap(); assert_eq!(transfer.transfer.subaddr_index, Index { major: data.1, minor: 0 }); assert_eq!(transfer.transfer.amount, 1000000); + assert_eq!(transfer.transfer.payment_id, hex::encode([0u8; 8])); // Make sure only one R was included in TX extra assert!(Extra::read::<&[u8]>(&mut tx.prefix.extra.as_ref())