Reject torsioned spend keys to ensure we can spend the outputs we scan

coins/monero/wallet/src/scan.rs

@@ -172,7 +172,6 @@ impl InternalScanner {
           // Our subtracting of a prime-order element means any torsion will be preserved
           // If someone wanted to malleate output keys with distinct torsions, only one will be
           // scanned accordingly (the one which has matching torsion of the spend key)
-          // TODO: If there's a torsioned spend key, can we spend outputs to it?
           let subaddress_spend_key =
             output_key - (&output_derivations.shared_key * ED25519_BASEPOINT_TABLE);
           self.subaddresses.get(&subaddress_spend_key.compress())

coins/monero/wallet/src/view_pair.rs

@@ -9,6 +9,21 @@ use crate::{
   address::{Network, AddressType, SubaddressIndex, MoneroAddress},
 };
 
+/// An error while working with a ViewPair.
+#[derive(Clone, PartialEq, Eq, Debug)]
+#[cfg_attr(feature = "std", derive(thiserror::Error))]
+pub enum ViewPairError {
+  /// The spend key was torsioned.
+  ///
+  /// Torsioned spend keys are of questionable spendability. This library avoids that question by
+  /// rejecting such ViewPairs.
+  // CLSAG seems to support it if the challenge does a torsion clear, FCMP++ should ship with a
+  // torsion clear, yet it's not worth it to modify CLSAG sign to generate challenges until the
+  // torsion clears and ensure spendability (nor can we reasonably guarantee that in the future)
+  #[cfg_attr(feature = "std", error("torsioned spend key"))]
+  TorsionedSpendKey,
+}
+
 /// The pair of keys necessary to scan transactions.
 ///
 /// This is composed of the public spend key and the private view key.
@@ -20,8 +35,11 @@ pub struct ViewPair {
 
 impl ViewPair {
   /// Create a new ViewPair.
-  pub fn new(spend: EdwardsPoint, view: Zeroizing<Scalar>) -> Self {
+  pub fn new(spend: EdwardsPoint, view: Zeroizing<Scalar>) -> Result<Self, ViewPairError> {
-    ViewPair { spend, view }
+    if !spend.is_torsion_free() {
+      Err(ViewPairError::TorsionedSpendKey)?;
+    }
+    Ok(ViewPair { spend, view })
   }
 
   /// The public spend key for this ViewPair.
@@ -86,8 +104,8 @@ pub struct GuaranteedViewPair(pub(crate) ViewPair);
 
 impl GuaranteedViewPair {
   /// Create a new GuaranteedViewPair.
-  pub fn new(spend: EdwardsPoint, view: Zeroizing<Scalar>) -> Self {
+  pub fn new(spend: EdwardsPoint, view: Zeroizing<Scalar>) -> Result<Self, ViewPairError> {
-    GuaranteedViewPair(ViewPair::new(spend, view))
+    ViewPair::new(spend, view).map(GuaranteedViewPair)
   }
 
   /// The public spend key for this GuaranteedViewPair.

coins/monero/wallet/tests/runner/mod.rs

@@ -35,7 +35,7 @@ pub fn random_address() -> (Scalar, ViewPair, MoneroAddress) {
   let view = Zeroizing::new(Scalar::random(&mut OsRng));
   (
     spend,
-    ViewPair::new(spend_pub, view.clone()),
+    ViewPair::new(spend_pub, view.clone()).unwrap(),
     MoneroAddress::new(
       Network::Mainnet,
       AddressType::Legacy,
@@ -52,7 +52,7 @@ pub fn random_guaranteed_address() -> (Scalar, GuaranteedViewPair, MoneroAddress
   let view = Zeroizing::new(Scalar::random(&mut OsRng));
   (
     spend,
-    GuaranteedViewPair::new(spend_pub, view.clone()),
+    GuaranteedViewPair::new(spend_pub, view.clone()).unwrap(),
     MoneroAddress::new(
       Network::Mainnet,
       AddressType::Legacy,
@@ -240,7 +240,7 @@ macro_rules! test {
           let view_priv = Zeroizing::new(Scalar::random(&mut OsRng));
           let mut outgoing_view = Zeroizing::new([0; 32]);
           OsRng.fill_bytes(outgoing_view.as_mut());
-          let view = ViewPair::new(spend_pub, view_priv.clone());
+          let view = ViewPair::new(spend_pub, view_priv.clone()).unwrap();
           let addr = view.legacy_address(Network::Mainnet);
 
           let miner_tx = get_miner_tx_output(&rpc, &view).await;
@@ -258,7 +258,7 @@ macro_rules! test {
               &ViewPair::new(
                 &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE,
                 Zeroizing::new(Scalar::random(&mut OsRng))
-              ),
+              ).unwrap(),
             ),
             rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
           );

coins/monero/wallet/tests/send.rs

@@ -109,7 +109,8 @@ test!(
       let mut outgoing_view = Zeroizing::new([0; 32]);
       OsRng.fill_bytes(outgoing_view.as_mut());
       let change_view =
-        ViewPair::new(&Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, view_priv.clone());
+        ViewPair::new(&Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, view_priv.clone())
+          .unwrap();
 
       let mut builder = SignableTransactionBuilder::new(
         rct_type,
@@ -123,7 +124,8 @@ test!(
       let sub_view = ViewPair::new(
         &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE,
         Zeroizing::new(Scalar::random(&mut OsRng)),
-      );
+      )
+      .unwrap();
       builder
         .add_payment(sub_view.subaddress(Network::Mainnet, SubaddressIndex::new(0, 1).unwrap()), 1);
       (builder.build().unwrap(), (change_view, sub_view))

processor/src/networks/monero.rs

@@ -256,7 +256,7 @@ impl Monero {
   }
 
   fn view_pair(spend: EdwardsPoint) -> GuaranteedViewPair {
-    GuaranteedViewPair::new(spend.0, Zeroizing::new(additional_key::<Monero>(0).0))
+    GuaranteedViewPair::new(spend.0, Zeroizing::new(additional_key::<Monero>(0).0)).unwrap()
   }
 
   fn address_internal(spend: EdwardsPoint, subaddress: Option<SubaddressIndex>) -> Address {
@@ -351,6 +351,7 @@ impl Monero {
       payments.push(Payment {
         address: Address::new(
           ViewPair::new(EdwardsPoint::generator().0, Zeroizing::new(Scalar::ONE.0))
+            .unwrap()
             .legacy_address(MoneroNetwork::Mainnet),
         )
         .unwrap(),
@@ -413,7 +414,7 @@ impl Monero {
 
   #[cfg(test)]
   fn test_view_pair() -> ViewPair {
-    ViewPair::new(*EdwardsPoint::generator(), Zeroizing::new(Scalar::ONE.0))
+    ViewPair::new(*EdwardsPoint::generator(), Zeroizing::new(Scalar::ONE.0)).unwrap()
   }
 
   #[cfg(test)]

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

@@ -90,6 +90,7 @@ async fn mint_and_burn_test() {
         use monero_wallet::{rpc::Rpc, ViewPair, address::Network};
 
         let addr = ViewPair::new(ED25519_BASEPOINT_POINT, Zeroizing::new(Scalar::ONE))
+          .unwrap()
           .legacy_address(Network::Mainnet);
 
         let rpc = producer_handles.monero(ops).await;
@@ -353,7 +354,7 @@ async fn mint_and_burn_test() {
 
       // Grab the first output on the chain
       let rpc = handles[0].monero(&ops).await;
-      let view_pair = ViewPair::new(ED25519_BASEPOINT_POINT, Zeroizing::new(Scalar::ONE));
+      let view_pair = ViewPair::new(ED25519_BASEPOINT_POINT, Zeroizing::new(Scalar::ONE)).unwrap();
       let mut scanner = Scanner::new(view_pair.clone());
       let output = scanner
         .scan(&rpc, &rpc.get_block_by_number(1).await.unwrap())
@@ -578,7 +579,8 @@ async fn mint_and_burn_test() {
     {
       use monero_wallet::{transaction::Transaction, rpc::Rpc, ViewPair, Scanner};
       let rpc = handles[0].monero(&ops).await;
-      let mut scanner = Scanner::new(ViewPair::new(monero_spend, Zeroizing::new(monero_view)));
+      let mut scanner =
+        Scanner::new(ViewPair::new(monero_spend, Zeroizing::new(monero_view)).unwrap());
 
       // Check for up to 5 minutes
       let mut found = false;

tests/processor/src/lib.rs

@@ -411,6 +411,7 @@ impl Coordinator {
         rpc
           .generate_blocks(
             &ViewPair::new(ED25519_BASEPOINT_POINT, Zeroizing::new(Scalar::ONE))
+              .unwrap()
               .legacy_address(Network::Mainnet),
             1,
           )

tests/processor/src/networks.rs

@@ -194,7 +194,7 @@ impl Wallet {
         let view_key = Scalar::random(&mut OsRng);
 
         let view_pair =
-          ViewPair::new(ED25519_BASEPOINT_POINT * spend_key, Zeroizing::new(view_key));
+          ViewPair::new(ED25519_BASEPOINT_POINT * spend_key, Zeroizing::new(view_key)).unwrap();
 
         let rpc = SimpleRequestRpc::new(rpc_url).await.expect("couldn't connect to the Monero RPC");