From 397fca748f29fb594c0e06e42cbed5cf653d1574 Mon Sep 17 00:00:00 2001 From: Justin Berman Date: Fri, 8 Dec 2023 04:42:02 -0800 Subject: [PATCH] monero-serai: make it clear that not providing a change address is fingerprintable (#472) * Make it clear not providing a change address is fingerprintable When no change address is provided, all change is shunted to the fee. This PR makes it clear to the caller that it is fingerprintable when the caller does this. * Review comments --- coins/monero/src/wallet/send/builder.rs | 6 +- coins/monero/src/wallet/send/mod.rs | 64 +++++++++++---------- coins/monero/src/wallet/send/multisig.rs | 4 +- coins/monero/tests/runner.rs | 10 +++- coins/monero/tests/send.rs | 43 +++++++++++++- processor/src/networks/monero.rs | 4 +- tests/full-stack/src/tests/mint_and_burn.rs | 2 +- tests/processor/src/networks.rs | 2 +- 8 files changed, 91 insertions(+), 44 deletions(-) diff --git a/coins/monero/src/wallet/send/builder.rs b/coins/monero/src/wallet/send/builder.rs index 080504c00..eaa199c41 100644 --- a/coins/monero/src/wallet/send/builder.rs +++ b/coins/monero/src/wallet/send/builder.rs @@ -18,14 +18,14 @@ struct SignableTransactionBuilderInternal { r_seed: Option>, inputs: Vec<(SpendableOutput, Decoys)>, payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change_address: Change, data: Vec>, } impl SignableTransactionBuilderInternal { // Takes in the change address so users don't miss that they have to manually set one // If they don't, all leftover funds will become part of the fee - fn new(protocol: Protocol, fee_rate: Fee, change_address: Option) -> Self { + fn new(protocol: Protocol, fee_rate: Fee, change_address: Change) -> Self { Self { protocol, fee_rate, @@ -90,7 +90,7 @@ impl SignableTransactionBuilder { Self(self.0.clone()) } - pub fn new(protocol: Protocol, fee_rate: Fee, change_address: Option) -> Self { + pub fn new(protocol: Protocol, fee_rate: Fee, change_address: Change) -> Self { Self(Arc::new(RwLock::new(SignableTransactionBuilderInternal::new( protocol, fee_rate, diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index f770e270c..616158666 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -292,7 +292,7 @@ impl FeePriority { #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] pub(crate) enum InternalPayment { Payment((MoneroAddress, u64)), - Change(Change, u64), + Change((MoneroAddress, Option>), u64), } /// The eventual output of a SignableTransaction. @@ -324,7 +324,7 @@ pub struct SignableTransaction { /// Specification for a change output. #[derive(Clone, PartialEq, Eq, Zeroize)] pub struct Change { - address: MoneroAddress, + address: Option, view: Option>, } @@ -338,21 +338,21 @@ impl Change { /// Create a change output specification from a ViewPair, as needed to maintain privacy. pub fn new(view: &ViewPair, guaranteed: bool) -> Change { Change { - address: view.address( + address: Some(view.address( Network::Mainnet, if !guaranteed { AddressSpec::Standard } else { AddressSpec::Featured { subaddress: None, payment_id: None, guaranteed: true } }, - ), + )), view: Some(view.view.clone()), } } /// Create a fingerprintable change output specification which will harm privacy. Only use this /// if you know what you're doing. - pub fn fingerprintable(address: MoneroAddress) -> Change { + pub fn fingerprintable(address: Option) -> Change { Change { address, view: None } } } @@ -364,13 +364,13 @@ fn need_additional(payments: &[InternalPayment]) -> (bool, bool) { .filter(|payment| match *payment { InternalPayment::Payment(payment) => payment.0.is_subaddress(), InternalPayment::Change(change, _) => { - if change.view.is_some() { + if change.1.is_some() { has_change_view = true; // It should not be possible to construct a change specification to a subaddress with a // view key - debug_assert!(!change.address.is_subaddress()); + debug_assert!(!change.0.is_subaddress()); } - change.address.is_subaddress() + change.0.is_subaddress() } }) .count() != @@ -415,7 +415,7 @@ impl SignableTransaction { r_seed: Option>, inputs: Vec<(SpendableOutput, Decoys)>, payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change: Change, data: Vec>, fee_rate: Fee, ) -> Result { @@ -430,8 +430,8 @@ impl SignableTransaction { for payment in &payments { count(payment.0); } - if let Some(change) = change_address.as_ref() { - count(change.address); + if let Some(change_address) = change.address.as_ref() { + count(*change_address); } if payment_ids > 1 { Err(TransactionError::MultiplePaymentIds)?; @@ -459,24 +459,24 @@ impl SignableTransaction { } // If we don't have two outputs, as required by Monero, error - if (payments.len() == 1) && change_address.is_none() { + if (payments.len() == 1) && change.address.is_none() { Err(TransactionError::NoChange)?; } // Get the outgoing amount ignoring fees let out_amount = payments.iter().map(|payment| payment.1).sum::(); - let outputs = payments.len() + usize::from(change_address.is_some()); + let outputs = payments.len() + usize::from(change.address.is_some()); if outputs > MAX_OUTPUTS { Err(TransactionError::TooManyOutputs)?; } // 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::>(); - if 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.clone().unwrap(), 0)); + payments.push(InternalPayment::Change((*change_address, change.view.clone()), 0)); } // Determine if we'll need additional pub keys in tx extra @@ -517,21 +517,23 @@ impl SignableTransaction { } // Sanity check we have the expected number of change outputs - sanity_check_change_payment_quantity(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change.address.is_some()); // Modify the amount of the change output - if change_address.is_some() { + if let Some(change_address) = change.address.as_ref() { let change_payment = payments.last_mut().unwrap(); debug_assert!(matches!(change_payment, InternalPayment::Change(_, _))); - *change_payment = - InternalPayment::Change(change_address.clone().unwrap(), in_amount - out_amount - fee); + *change_payment = InternalPayment::Change( + (*change_address, change.view.clone()), + in_amount - out_amount - fee, + ); } // Sanity check the change again after modifying - sanity_check_change_payment_quantity(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change.address.is_some()); // Sanity check outgoing amount + fee == incoming amount - if change_address.is_some() { + if change.address.is_some() { debug_assert_eq!( payments .iter() @@ -551,7 +553,7 @@ impl SignableTransaction { r_seed, inputs, payments, - has_change: change_address.is_some(), + has_change: change.address.is_some(), data, fee, fee_rate, @@ -624,7 +626,7 @@ impl SignableTransaction { // regarding it's view key payment = if !modified_change_ecdh { if let InternalPayment::Change(change, amount) = &payment { - InternalPayment::Payment((change.address, *amount)) + InternalPayment::Payment((change.0, *amount)) } else { payment } @@ -656,8 +658,8 @@ impl SignableTransaction { InternalPayment::Change(change, amount) => { // 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.view.unwrap().deref(); - SendOutput::change(ecdh, uniqueness, (o, (change.address, amount))) + let ecdh = tx_public_key * change.1.unwrap().deref(); + SendOutput::change(ecdh, uniqueness, (o, (change.0, amount))) } }; @@ -954,8 +956,8 @@ impl Eventuality { } InternalPayment::Change(change, amount) => { w.write_all(&[1])?; - write_vec(write_byte, change.address.to_string().as_bytes(), w)?; - if let Some(view) = change.view.as_ref() { + write_vec(write_byte, change.0.to_string().as_bytes(), w)?; + if let Some(view) = change.1.as_ref() { w.write_all(&[1])?; write_scalar(view, w)?; } else { @@ -988,14 +990,14 @@ impl Eventuality { Ok(match read_byte(r)? { 0 => InternalPayment::Payment((read_address(r)?, read_u64(r)?)), 1 => InternalPayment::Change( - Change { - address: read_address(r)?, - view: match read_byte(r)? { + ( + 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)?, ), _ => 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 39e79437b..eecfd3fe7 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/coins/monero/src/wallet/send/multisig.rs @@ -125,8 +125,8 @@ impl SignableTransaction { transcript.append_message(b"payment_amount", payment.1.to_le_bytes()); } InternalPayment::Change(change, amount) => { - transcript.append_message(b"change_address", change.address.to_string().as_bytes()); - if let Some(view) = change.view.as_ref() { + 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_view_key", Zeroizing::new(view.to_bytes())); } transcript.append_message(b"change_amount", amount.to_le_bytes()); diff --git a/coins/monero/tests/runner.rs b/coins/monero/tests/runner.rs index e85268fd8..cb0a38088 100644 --- a/coins/monero/tests/runner.rs +++ b/coins/monero/tests/runner.rs @@ -213,13 +213,13 @@ macro_rules! test { let builder = SignableTransactionBuilder::new( protocol, rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), - Some(Change::new( + Change::new( &ViewPair::new( &random_scalar(&mut OsRng) * ED25519_BASEPOINT_TABLE, Zeroizing::new(random_scalar(&mut OsRng)) ), false - )), + ), ); let sign = |tx: SignableTransaction| { @@ -298,7 +298,11 @@ macro_rules! test { rpc.publish_transaction(&signed).await.unwrap(); mine_until_unlocked(&rpc, &random_address().2.to_string(), signed.hash()).await; let tx = rpc.get_transaction(signed.hash()).await.unwrap(); - check_weight_and_fee(&tx, fee_rate); + if stringify!($name) != "spend_one_input_to_two_outputs_no_change" { + // Skip weight and fee check for the above test because when there is no change, + // the change is added to the fee + check_weight_and_fee(&tx, fee_rate); + } #[allow(unused_assignments)] { let scanner = diff --git a/coins/monero/tests/send.rs b/coins/monero/tests/send.rs index b2abcb67b..cd1b919d5 100644 --- a/coins/monero/tests/send.rs +++ b/coins/monero/tests/send.rs @@ -111,7 +111,7 @@ test!( let mut builder = SignableTransactionBuilder::new( protocol, rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), - Some(Change::new(&change_view, false)), + Change::new(&change_view, false), ); add_inputs(protocol, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; @@ -273,3 +273,44 @@ test!( }, ), ); + +test!( + spend_one_input_to_two_outputs_no_change, + ( + |_, mut builder: Builder, addr| async move { + builder.add_payment(addr, 1000000000000); + (builder.build().unwrap(), ()) + }, + |_, tx: Transaction, mut scanner: Scanner, _| async move { + let mut outputs = scanner.scan_transaction(&tx).not_locked(); + outputs.sort_by(|x, y| x.commitment().amount.cmp(&y.commitment().amount)); + assert_eq!(outputs[0].commitment().amount, 1000000000000); + outputs + }, + ), + ( + |protocol, rpc: Rpc<_>, _, addr, outputs: Vec| async move { + use monero_serai::wallet::FeePriority; + + let mut builder = SignableTransactionBuilder::new( + protocol, + rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), + Change::fingerprintable(None), + ); + add_inputs(protocol, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; + builder.add_payment(addr, 10000); + builder.add_payment(addr, 50000); + + (builder.build().unwrap(), ()) + }, + |_, tx: Transaction, mut scanner: Scanner, _| async move { + let mut outputs = scanner.scan_transaction(&tx).not_locked(); + outputs.sort_by(|x, y| x.commitment().amount.cmp(&y.commitment().amount)); + assert_eq!(outputs[0].commitment().amount, 10000); + assert_eq!(outputs[1].commitment().amount, 50000); + + // The remainder should get shunted to fee, which is fingerprintable + assert_eq!(tx.rct_signatures.base.fee, 1000000000000 - 10000 - 50000); + }, + ), +); diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index 7bcf80977..d2ac279d6 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -384,7 +384,7 @@ impl Monero { Some(Zeroizing::new(*plan_id)), inputs.clone(), payments, - change.clone().map(|change| Change::fingerprintable(change.into())), + Change::fingerprintable(change.as_ref().map(|change| change.clone().into())), vec![], fee_rate, ) { @@ -753,7 +753,7 @@ impl Network for Monero { None, inputs, vec![(address.into(), amount - fee)], - Some(Change::fingerprintable(Self::test_address().into())), + Change::fingerprintable(Some(Self::test_address().into())), vec![], self.rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), ) diff --git a/tests/full-stack/src/tests/mint_and_burn.rs b/tests/full-stack/src/tests/mint_and_burn.rs index 799385965..856834fa9 100644 --- a/tests/full-stack/src/tests/mint_and_burn.rs +++ b/tests/full-stack/src/tests/mint_and_burn.rs @@ -395,7 +395,7 @@ async fn mint_and_burn_test() { ), 1_100_000_000_000, )], - Some(Change::new(&view_pair, false)), + Change::new(&view_pair, false), vec![Shorthand::transfer(None, serai_addr).encode()], rpc.get_fee(Protocol::v16, FeePriority::Low).await.unwrap(), ) diff --git a/tests/processor/src/networks.rs b/tests/processor/src/networks.rs index 140f096e0..db02686e5 100644 --- a/tests/processor/src/networks.rs +++ b/tests/processor/src/networks.rs @@ -361,7 +361,7 @@ impl Wallet { None, these_inputs.drain(..).zip(decoys.drain(..)).collect(), vec![(to_addr, AMOUNT)], - Some(Change::new(view_pair, false)), + Change::new(view_pair, false), data, rpc.get_fee(Protocol::v16, FeePriority::Low).await.unwrap(), )