monero: match monero's stricter check when decompressing points (#515)

* monero: match monero's stricter check when decompressing points

* Reverted type change for output key

coins/monero/generators/Cargo.toml

@@ -26,6 +26,9 @@ curve25519-dalek = { version = "4", default-features = false, features = ["alloc
 group = { version = "0.13", default-features = false }
 dalek-ff-group = { path = "../../../crypto/dalek-ff-group", version = "0.4", default-features = false }
 
+[dev-dependencies]
+hex = "0.4"
+
 [features]
 std = ["std-shims/std", "subtle/std", "sha3/std", "dalek-ff-group/std"]
 default = ["std"]

coins/monero/generators/src/hash_to_point.rs

@@ -7,7 +7,16 @@ use dalek_ff_group::FieldElement;
 
 use crate::hash;
 
-/// Monero's hash to point function, as named `ge_fromfe_frombytes_vartime`.
+/// Decompress canonically encoded ed25519 point
+/// It does not check if the point is in the prime order subgroup
+pub fn decompress_point(bytes: [u8; 32]) -> Option<EdwardsPoint> {
+  CompressedEdwardsY(bytes)
+    .decompress()
+    // Ban points which are either unreduced or -0
+    .filter(|point| point.compress().to_bytes() == bytes)
+}
+
+/// Monero's hash to point function, as named `hash_to_ec`.
 pub fn hash_to_point(bytes: [u8; 32]) -> EdwardsPoint {
   #[allow(non_snake_case)]
   let A = FieldElement::from(486662u64);
@@ -47,5 +56,5 @@ pub fn hash_to_point(bytes: [u8; 32]) -> EdwardsPoint {
   let mut bytes = Y.to_repr();
   bytes[31] |= sign.unwrap_u8() << 7;
 
-  CompressedEdwardsY(bytes).decompress().unwrap().mul_by_cofactor()
+  decompress_point(bytes).unwrap().mul_by_cofactor()
 }

coins/monero/generators/src/lib.rs

@@ -9,7 +9,7 @@ use std_shims::{sync::OnceLock, vec::Vec};
 
 use sha3::{Digest, Keccak256};
 
-use curve25519_dalek::edwards::{EdwardsPoint as DalekPoint, CompressedEdwardsY};
+use curve25519_dalek::edwards::{EdwardsPoint as DalekPoint};
 
 use group::{Group, GroupEncoding};
 use dalek_ff_group::EdwardsPoint;
@@ -18,7 +18,10 @@ mod varint;
 use varint::write_varint;
 
 mod hash_to_point;
-pub use hash_to_point::hash_to_point;
+pub use hash_to_point::{hash_to_point, decompress_point};
+
+#[cfg(test)]
+mod tests;
 
 fn hash(data: &[u8]) -> [u8; 32] {
   Keccak256::digest(data).into()
@@ -29,10 +32,7 @@ static H_CELL: OnceLock<DalekPoint> = OnceLock::new();
 #[allow(non_snake_case)]
 pub fn H() -> DalekPoint {
   *H_CELL.get_or_init(|| {
-    CompressedEdwardsY(hash(&EdwardsPoint::generator().to_bytes()))
-      .decompress()
-      .unwrap()
-      .mul_by_cofactor()
+    decompress_point(hash(&EdwardsPoint::generator().to_bytes())).unwrap().mul_by_cofactor()
   })
 }
 

coins/monero/generators/src/tests/hash_to_point.rs

@@ -0,0 +1,38 @@
+use crate::{decompress_point, hash_to_point};
+
+#[test]
+fn crypto_tests() {
+  // tests.txt file copied from monero repo
+  // https://github.com/monero-project/monero/
+  //   blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/tests/crypto/tests.txt
+  let reader = include_str!("./tests.txt");
+
+  for line in reader.lines() {
+    let mut words = line.split_whitespace();
+    let command = words.next().unwrap();
+
+    match command {
+      "check_key" => {
+        let key = words.next().unwrap();
+        let expected = match words.next().unwrap() {
+          "true" => true,
+          "false" => false,
+          _ => unreachable!("invalid result"),
+        };
+
+        let actual = decompress_point(hex::decode(key).unwrap().try_into().unwrap());
+
+        assert_eq!(actual.is_some(), expected);
+      }
+      "hash_to_ec" => {
+        let bytes = words.next().unwrap();
+        let expected = words.next().unwrap();
+
+        let actual = hash_to_point(hex::decode(bytes).unwrap().try_into().unwrap());
+
+        assert_eq!(hex::encode(actual.compress().to_bytes()), expected);
+      }
+      _ => unreachable!("unknown command"),
+    }
+  }
+}

coins/monero/generators/src/tests/mod.rs

@@ -0,0 +1 @@
+mod hash_to_point;

coins/monero/src/bin/reserialize_chain.rs

@@ -2,10 +2,7 @@
 mod binaries {
   pub(crate) use std::sync::Arc;
 
-  pub(crate) use curve25519_dalek::{
-    scalar::Scalar,
-    edwards::{CompressedEdwardsY, EdwardsPoint},
-  };
+  pub(crate) use curve25519_dalek::{scalar::Scalar, edwards::EdwardsPoint};
 
   pub(crate) use multiexp::BatchVerifier;
 
@@ -20,6 +17,8 @@ mod binaries {
     rpc::{RpcError, Rpc, HttpRpc},
   };
 
+  pub(crate) use monero_generators::decompress_point;
+
   pub(crate) use tokio::task::JoinHandle;
 
   pub(crate) async fn check_block(rpc: Arc<Rpc<HttpRpc>>, block_i: usize) {
@@ -201,13 +200,12 @@ mod binaries {
                 };
 
                 let rpc_point = |point: &str| {
-                  CompressedEdwardsY(
+                  decompress_point(
                     hex::decode(point)
                       .expect("invalid hex for ring member")
                       .try_into()
                       .expect("invalid point len for ring member"),
                   )
-                  .decompress()
                   .expect("invalid point for ring member")
                 };
 

coins/monero/src/lib.rs

@@ -16,7 +16,7 @@ use sha3::{Digest, Keccak256};
 
 use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, scalar::Scalar, edwards::EdwardsPoint};
 
-pub use monero_generators::H;
+pub use monero_generators::{H, decompress_point};
 
 mod merkle;
 

coins/monero/src/rpc/mod.rs

@@ -9,7 +9,9 @@ use std_shims::{
 
 use async_trait::async_trait;
 
-use curve25519_dalek::edwards::{EdwardsPoint, CompressedEdwardsY};
+use curve25519_dalek::edwards::EdwardsPoint;
+
+use monero_generators::decompress_point;
 
 use serde::{Serialize, Deserialize, de::DeserializeOwned};
 use serde_json::{Value, json};
@@ -86,10 +88,9 @@ fn hash_hex(hash: &str) -> Result<[u8; 32], RpcError> {
 }
 
 fn rpc_point(point: &str) -> Result<EdwardsPoint, RpcError> {
-  CompressedEdwardsY(
+  decompress_point(
     rpc_hex(point)?.try_into().map_err(|_| RpcError::InvalidPoint(point.to_string()))?,
   )
-  .decompress()
   .ok_or_else(|| RpcError::InvalidPoint(point.to_string()))
 }
 
@@ -585,12 +586,11 @@ impl<R: RpcConnection> Rpc<R> {
         // Only valid keys can be used in CLSAG proofs, hence the need for re-selection, yet
         // invalid keys may honestly exist on the blockchain
         // Only a recent hard fork checked output keys were valid points
-        let Some(key) = CompressedEdwardsY(
+        let Some(key) = decompress_point(
           rpc_hex(&out.key)?
             .try_into()
             .map_err(|_| RpcError::InvalidNode("non-32-byte point".to_string()))?,
-        )
-        .decompress() else {
+        ) else {
           return Ok(None);
         };
         Ok(

coins/monero/src/serialize.rs

@@ -4,10 +4,9 @@ use std_shims::{
   io::{self, Read, Write},
 };
 
-use curve25519_dalek::{
-  scalar::Scalar,
-  edwards::{EdwardsPoint, CompressedEdwardsY},
-};
+use curve25519_dalek::{scalar::Scalar, edwards::EdwardsPoint};
+
+use monero_generators::decompress_point;
 
 const VARINT_CONTINUATION_MASK: u8 = 0b1000_0000;
 
@@ -136,11 +135,7 @@ pub(crate) fn read_scalar<R: Read>(r: &mut R) -> io::Result<Scalar> {
 
 pub(crate) fn read_point<R: Read>(r: &mut R) -> io::Result<EdwardsPoint> {
   let bytes = read_bytes(r)?;
-  CompressedEdwardsY(bytes)
-    .decompress()
-    // Ban points which are either unreduced or -0
-    .filter(|point| point.compress().to_bytes() == bytes)
-    .ok_or_else(|| io::Error::other("invalid point"))
+  decompress_point(bytes).ok_or_else(|| io::Error::other("invalid point"))
 }
 
 pub(crate) fn read_torsion_free_point<R: Read>(r: &mut R) -> io::Result<EdwardsPoint> {

coins/monero/src/tests/address.rs

@@ -2,7 +2,9 @@ use hex_literal::hex;
 
 use rand_core::{RngCore, OsRng};
 
-use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, edwards::CompressedEdwardsY};
+use curve25519_dalek::constants::ED25519_BASEPOINT_TABLE;
+
+use monero_generators::decompress_point;
 
 use crate::{
   random_scalar,
@@ -142,14 +144,8 @@ fn featured_vectors() {
       }
       _ => panic!("Unknown network"),
     };
-    let spend = CompressedEdwardsY::from_slice(&hex::decode(vector.spend).unwrap())
-      .unwrap()
-      .decompress()
-      .unwrap();
-    let view = CompressedEdwardsY::from_slice(&hex::decode(vector.view).unwrap())
-      .unwrap()
-      .decompress()
-      .unwrap();
+    let spend = decompress_point(hex::decode(vector.spend).unwrap().try_into().unwrap()).unwrap();
+    let view = decompress_point(hex::decode(vector.view).unwrap().try_into().unwrap()).unwrap();
 
     let addr = MoneroAddress::from_str(network, &vector.address).unwrap();
     assert_eq!(addr.spend, spend);

coins/monero/src/tests/bulletproofs/mod.rs

@@ -1,7 +1,8 @@
 use hex_literal::hex;
 use rand_core::OsRng;
 
-use curve25519_dalek::{scalar::Scalar, edwards::CompressedEdwardsY};
+use curve25519_dalek::scalar::Scalar;
+use monero_generators::decompress_point;
 use multiexp::BatchVerifier;
 
 use crate::{
@@ -14,7 +15,7 @@ mod plus;
 #[test]
 fn bulletproofs_vector() {
   let scalar = |scalar| Scalar::from_canonical_bytes(scalar).unwrap();
-  let point = |point| CompressedEdwardsY(point).decompress().unwrap();
+  let point = |point| decompress_point(point).unwrap();
 
   // Generated from Monero
   assert!(Bulletproofs::Original(OriginalStruct {

coins/monero/src/wallet/address.rs

@@ -3,7 +3,9 @@ use std_shims::string::{String, ToString};
 
 use zeroize::Zeroize;
 
-use curve25519_dalek::edwards::{EdwardsPoint, CompressedEdwardsY};
+use curve25519_dalek::edwards::EdwardsPoint;
+
+use monero_generators::decompress_point;
 
 use base58_monero::base58::{encode_check, decode_check};
 
@@ -240,12 +242,10 @@ impl<B: AddressBytes> Address<B> {
     }
 
     let mut meta = AddressMeta::from_byte(raw[0])?;
-    let spend = CompressedEdwardsY(raw[1 .. 33].try_into().unwrap())
-      .decompress()
-      .ok_or(AddressError::InvalidKey)?;
-    let view = CompressedEdwardsY(raw[33 .. 65].try_into().unwrap())
-      .decompress()
-      .ok_or(AddressError::InvalidKey)?;
+    let spend =
+      decompress_point(raw[1 .. 33].try_into().unwrap()).ok_or(AddressError::InvalidKey)?;
+    let view =
+      decompress_point(raw[33 .. 65].try_into().unwrap()).ok_or(AddressError::InvalidKey)?;
     let mut read = 65;
 
     if matches!(meta.kind, AddressType::Featured { .. }) {

coins/monero/src/wallet/scan.rs

@@ -9,6 +9,8 @@ use zeroize::{Zeroize, ZeroizeOnDrop};
 
 use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, scalar::Scalar, edwards::EdwardsPoint};
 
+use monero_generators::decompress_point;
+
 use crate::{
   Commitment,
   serialize::{read_byte, read_u32, read_u64, read_bytes, read_scalar, read_point, read_raw_vec},
@@ -349,7 +351,7 @@ impl Scanner {
         }
       }
 
-      let output_key = output.key.decompress();
+      let output_key = decompress_point(output.key.to_bytes());
       if output_key.is_none() {
         continue;
       }

tests/full-stack/src/tests/mint_and_burn.rs

@@ -336,16 +336,15 @@ async fn mint_and_burn_test() {
 
     // Send in XMR
     {
-      use curve25519_dalek::{
-        constants::ED25519_BASEPOINT_POINT, scalar::Scalar, edwards::CompressedEdwardsY,
-      };
+      use curve25519_dalek::{constants::ED25519_BASEPOINT_POINT, scalar::Scalar};
       use monero_serai::{
         Protocol,
         transaction::Timelock,
         wallet::{
           ViewPair, Scanner, Decoys, Change, FeePriority, SignableTransaction,
           address::{Network, AddressType, AddressMeta, MoneroAddress},
         },
+        decompress_point,
       };
 
       // Grab the first output on the chain
@@ -382,9 +381,7 @@ async fn mint_and_burn_test() {
               Network::Mainnet,
               AddressType::Featured { guaranteed: true, subaddress: false, payment_id: None },
             ),
-            CompressedEdwardsY(monero_key_pair.1.to_vec().try_into().unwrap())
-              .decompress()
-              .unwrap(),
+            decompress_point(monero_key_pair.1.to_vec().try_into().unwrap()).unwrap(),
             ED25519_BASEPOINT_POINT *
               processor::additional_key::<processor::networks::monero::Monero>(0).0,
           ),

tests/processor/src/networks.rs

@@ -283,14 +283,15 @@ impl Wallet {
       }
 
       Wallet::Monero { handle, ref spend_key, ref view_pair, ref mut inputs } => {
-        use curve25519_dalek::{constants::ED25519_BASEPOINT_POINT, edwards::CompressedEdwardsY};
+        use curve25519_dalek::constants::ED25519_BASEPOINT_POINT;
         use monero_serai::{
           Protocol,
           wallet::{
             address::{Network, AddressType, AddressMeta, Address},
             SpendableOutput, Decoys, Change, FeePriority, Scanner, SignableTransaction,
           },
           rpc::HttpRpc,
+          decompress_point,
         };
         use processor::{additional_key, networks::Monero};
 
@@ -317,8 +318,7 @@ impl Wallet {
         .await
         .unwrap();
 
-        let to_spend_key =
-          CompressedEdwardsY(<[u8; 32]>::try_from(to.as_ref()).unwrap()).decompress().unwrap();
+        let to_spend_key = decompress_point(<[u8; 32]>::try_from(to.as_ref()).unwrap()).unwrap();
         let to_view_key = additional_key::<Monero>(0);
         let to_addr = Address::new(
           AddressMeta::new(