Skip to content

Commit

Permalink
monero: make dummy payment ID zeroes when it's included in a tx (#514)
Browse files Browse the repository at this point in the history 
* monero: make dummy payment ID zeroes when it's included in a tx

Also did some minor cleaning of InternalPayment::Change

* Lint

* Clarify comment
  • Loading branch information
@j-berman
j-berman authored Feb 20, 2024
1 parent ebdfc9a commit 0880453
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 84 deletions.
35 changes: 22 additions & 13 deletions coins/monero/src/wallet/scan.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,18 @@ pub struct Metadata {
/// The subaddress this output was sent to.
pub subaddress: Option<SubaddressIndex>,
/// 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<PaymentId>,
/// Arbitrary data encoded in TX extra.
pub arbitrary_data: Vec<Vec<u8>>,
}
Expand All @@ -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::<Vec<_>>())
.finish()
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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)? {
Expand Down Expand Up @@ -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 {
Expand Down
135 changes: 78 additions & 57 deletions coins/monero/src/wallet/send/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<Scalar>,
unique: [u8; 32],
output: (usize, (MoneroAddress, u64)),
output: (usize, (MoneroAddress, u64), bool),
) -> (SendOutput, Option<[u8; 8]>) {
let address = output.1 .0;
SendOutput::internal(
Expand All @@ -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)
}
Expand Down Expand Up @@ -291,8 +295,8 @@ impl FeePriority {

#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
pub(crate) enum InternalPayment {
Payment((MoneroAddress, u64)),
Change((MoneroAddress, Option<Zeroizing<Scalar>>), u64),
Payment((MoneroAddress, u64), bool),
Change((MoneroAddress, u64), Option<Zeroizing<Scalar>>),
}

/// The eventual output of a SignableTransaction.
Expand Down Expand Up @@ -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<MoneroAddress>) -> Change {
Change { address, view: None }
}
Expand All @@ -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
Expand All @@ -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(),
Expand Down Expand Up @@ -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::<u64>();

Expand All @@ -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::<Vec<_>>();
let mut payments = payments
.into_iter()
.map(|payment| InternalPayment::Payment(payment, need_dummy_payment_id))
.collect::<Vec<_>>();
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());

Expand Down Expand Up @@ -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(),
);
}

Expand All @@ -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::<u64>() +
fee,
Expand Down Expand Up @@ -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;
Expand All @@ -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
}
Expand All @@ -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);
}
Expand All @@ -655,28 +677,18 @@ 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))
}
};

outputs.push(output);
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)
}

Expand Down Expand Up @@ -949,21 +961,26 @@ impl Eventuality {

fn write_payment<W: io::Write>(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())
}
}
}
Expand All @@ -988,17 +1005,21 @@ impl Eventuality {

fn read_payment<R: io::Read>(r: &mut R) -> io::Result<InternalPayment> {
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"))?,
})
Expand Down
12 changes: 8 additions & 4 deletions coins/monero/src/wallet/send/multisig.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
Expand Down
Loading

0 comments on commit 0880453

Please sign in to comment.