From d620231530269058994000ad50f5d54c9a4990b4 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Tue, 30 Aug 2022 01:02:55 -0400 Subject: [PATCH] Remove Monero torsion-free requirement and make output keys 32 bytes Maintains the torsion-free requirement in the one place it's used (key images). In the modern day, the output keys are checked to be points, yet in older protocol versions they were allowed to be arbitrary bytes. Closes https://github.com/serai-dex/serai/issues/23 and https://github.com/serai-dex/serai/issues/25. --- coins/monero/src/serialize.rs | 8 ++++++-- coins/monero/src/transaction.rs | 10 +++++----- coins/monero/src/wallet/scan.rs | 26 +++++++++++++++----------- coins/monero/src/wallet/send/mod.rs | 2 +- processor/src/coin/monero.rs | 2 +- 5 files changed, 28 insertions(+), 20 deletions(-) diff --git a/coins/monero/src/serialize.rs b/coins/monero/src/serialize.rs index ee4c715f..5f0840f6 100644 --- a/coins/monero/src/serialize.rs +++ b/coins/monero/src/serialize.rs @@ -104,11 +104,15 @@ pub(crate) fn read_point(r: &mut R) -> io::Result { let bytes = read_bytes(r)?; CompressedEdwardsY(bytes) .decompress() - // Ban torsioned points, and points which are either unreduced or -0 - .filter(|point| point.is_torsion_free() && (point.compress().to_bytes() == bytes)) + // Ban points which are either unreduced or -0 + .filter(|point| point.compress().to_bytes() == bytes) .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "invalid point")) } +pub(crate) fn read_torsion_free_point(r: &mut R) -> io::Result { + read_point(r).ok().filter(|point| point.is_torsion_free()).ok_or(io::Error::new(io::ErrorKind::Other, "invalid point")) +} + pub(crate) fn read_raw_vec io::Result>( f: F, len: usize, diff --git a/coins/monero/src/transaction.rs b/coins/monero/src/transaction.rs index 9dcb3b36..9a6d44ba 100644 --- a/coins/monero/src/transaction.rs +++ b/coins/monero/src/transaction.rs @@ -2,7 +2,7 @@ use core::cmp::Ordering; use zeroize::Zeroize; -use curve25519_dalek::edwards::EdwardsPoint; +use curve25519_dalek::edwards::{EdwardsPoint, CompressedEdwardsY}; use crate::{ Protocol, hash, @@ -46,7 +46,7 @@ impl Input { 2 => Input::ToKey { amount: read_varint(r)?, key_offsets: read_vec(read_varint, r)?, - key_image: read_point(r)?, + key_image: read_torsion_free_point(r)?, }, _ => Err(std::io::Error::new( std::io::ErrorKind::Other, @@ -60,7 +60,7 @@ impl Input { #[derive(Clone, PartialEq, Eq, Debug)] pub struct Output { pub amount: u64, - pub key: EdwardsPoint, + pub key: CompressedEdwardsY, pub view_tag: Option, } @@ -72,7 +72,7 @@ impl Output { pub fn serialize(&self, w: &mut W) -> std::io::Result<()> { write_varint(&self.amount, w)?; w.write_all(&[2 + (if self.view_tag.is_some() { 1 } else { 0 })])?; - write_point(&self.key, w)?; + w.write_all(&self.key.to_bytes())?; if let Some(view_tag) = self.view_tag { w.write_all(&[view_tag])?; } @@ -92,7 +92,7 @@ impl Output { Ok(Output { amount, - key: read_point(r)?, + key: CompressedEdwardsY(read_bytes(r)?), view_tag: if view_tag { Some(read_byte(r)?) } else { None }, }) } diff --git a/coins/monero/src/wallet/scan.rs b/coins/monero/src/wallet/scan.rs index fc4061ac..427853b6 100644 --- a/coins/monero/src/wallet/scan.rs +++ b/coins/monero/src/wallet/scan.rs @@ -65,8 +65,7 @@ pub struct Metadata { pub subaddress: (u32, u32), // Can be an Option, as extra doesn't necessarily have a payment ID, yet all Monero TXs should // have this - // This will be gibberish if the payment ID wasn't intended for the recipient - // This will be [0xff; 8] if the transaction didn't have a payment ID + // This will be gibberish if the payment ID wasn't intended for the recipient or wasn't included // 0xff was chosen as it'd be distinct from [0; 8], enabling atomically incrementing IDs (though // they should be randomly generated) pub payment_id: [u8; 8], @@ -211,14 +210,19 @@ impl Scanner { let mut res = vec![]; for (o, output) in tx.prefix.outputs.iter().enumerate() { - // https://github.com/serai-dex/serai/issues/102 - let output_key_compressed = output.key.compress(); + // https://github.com/serai-dex/serai/issues/106 if let Some(burning_bug) = self.burning_bug.as_ref() { - if burning_bug.contains(&output_key_compressed) { + if burning_bug.contains(&output.key) { continue; } } + let output_key = output.key.decompress(); + if output_key.is_none() { + continue; + } + let output_key = output_key.unwrap(); + for key in &keys { let (view_tag, shared_key, payment_id_xor) = shared_key( if self.burning_bug.is_none() { Some(uniqueness(&tx.prefix.inputs)) } else { None }, @@ -231,7 +235,7 @@ impl Scanner { if let Some(PaymentId::Encrypted(id)) = payment_id.map(|id| id ^ payment_id_xor) { id } else { - [0xff; 8] + payment_id_xor }; if let Some(actual_view_tag) = output.view_tag { @@ -243,15 +247,15 @@ impl Scanner { // P - shared == spend let subaddress = self .subaddresses - .get(&(output.key - (&shared_key * &ED25519_BASEPOINT_TABLE)).compress()); + .get(&(output_key - (&shared_key * &ED25519_BASEPOINT_TABLE)).compress()); if subaddress.is_none() { continue; } // If it has torsion, it'll substract the non-torsioned shared key to a torsioned key // We will not have a torsioned key in our HashMap of keys, so we wouldn't identify it as // ours - // If we did, it'd enable bypassing the included burning bug protection however - debug_assert!(output.key.is_torsion_free()); + // If we did though, it'd enable bypassing the included burning bug protection + debug_assert!(output_key.is_torsion_free()); let key_offset = shared_key + self.pair.subaddress(*subaddress.unwrap()); // Since we've found an output to us, get its amount @@ -282,13 +286,13 @@ impl Scanner { res.push(ReceivedOutput { absolute: AbsoluteId { tx: tx.hash(), o: o.try_into().unwrap() }, - data: OutputData { key: output.key, key_offset, commitment }, + data: OutputData { key: output_key, key_offset, commitment }, metadata: Metadata { subaddress: (0, 0), payment_id }, }); if let Some(burning_bug) = self.burning_bug.as_mut() { - burning_bug.insert(output_key_compressed); + burning_bug.insert(output.key); } } // Break to prevent public keys from being included multiple times, triggering multiple diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index 1ad09a91..5f798fd6 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -305,7 +305,7 @@ impl SignableTransaction { for output in &outputs { tx_outputs.push(Output { amount: 0, - key: output.dest, + key: output.dest.compress(), view_tag: Some(output.view_tag).filter(|_| matches!(self.protocol, Protocol::v16)), }); ecdh_info.push(output.amount); diff --git a/processor/src/coin/monero.rs b/processor/src/coin/monero.rs index f28afad2..8fb9f76e 100644 --- a/processor/src/coin/monero.rs +++ b/processor/src/coin/monero.rs @@ -199,7 +199,7 @@ impl Coin for Monero { Ok(( tx.hash().to_vec(), - tx.prefix.outputs.iter().map(|output| output.key.compress().to_bytes()).collect(), + tx.prefix.outputs.iter().map(|output| output.key.to_bytes()).collect(), )) }