Remove Clone from ClsagMultisigMask{Sender, Receiver}

This had ill-defined properties on Clone, as a mask could be sent multiple times
(unintended) and multiple algorithms may receive the same mask from a singular
sender.

Requires removing the Clone bound within modular-frost and expanding the test
helpers accordingly.

This was not raised in the audit yet upon independent review.

crypto/frost/src/algorithm.rs

@@ -25,7 +25,7 @@ pub trait Addendum: Send + Sync + Clone + PartialEq + Debug + WriteAddendum {}
 impl<A: Send + Sync + Clone + PartialEq + Debug + WriteAddendum> Addendum for A {}
 
 /// Algorithm trait usable by the FROST signing machine to produce signatures..
-pub trait Algorithm<C: Curve>: Send + Sync + Clone {
+pub trait Algorithm<C: Curve>: Send + Sync {
   /// The transcript format this algorithm uses. This likely should NOT be the IETF-compatible
   /// transcript included in this crate.
   type Transcript: Sync + Clone + Debug + Transcript;

crypto/frost/src/sign.rs

@@ -47,7 +47,7 @@ impl<T: Writable> Writable for Vec<T> {
 }
 
 // Pairing of an Algorithm with a ThresholdKeys instance.
-#[derive(Clone, Zeroize)]
+#[derive(Zeroize)]
 struct Params<C: Curve, A: Algorithm<C>> {
   // Skips the algorithm due to being too large a bound to feasibly enforce on users
   #[zeroize(skip)]
@@ -193,7 +193,7 @@ impl<C: Curve> SignatureShare<C> {
 /// Trait for the second machine of a two-round signing protocol.
 pub trait SignMachine<S>: Send + Sync + Sized {
   /// Params used to instantiate this machine which can be used to rebuild from a cache.
-  type Params: Clone;
+  type Params;
   /// Keys used for signing operations.
   type Keys;
   /// Preprocess message for this machine.
@@ -397,7 +397,7 @@ impl<C: Curve, A: Algorithm<C>> SignMachine<A::Signature> for AlgorithmSignMachi
 
     Ok((
       AlgorithmSignatureMachine {
-        params: self.params.clone(),
+        params: self.params,
         view,
         B,
         Rs,

crypto/frost/src/tests/mod.rs

@@ -37,10 +37,10 @@ pub fn clone_without<K: Clone + core::cmp::Eq + core::hash::Hash, V: Clone>(
 }
 
 /// Spawn algorithm machines for a random selection of signers, each executing the given algorithm.
-pub fn algorithm_machines<R: RngCore, C: Curve, A: Algorithm<C>>(
+pub fn algorithm_machines_without_clone<R: RngCore, C: Curve, A: Algorithm<C>>(
   rng: &mut R,
-  algorithm: &A,
   keys: &HashMap<Participant, ThresholdKeys<C>>,
+  machines: HashMap<Participant, AlgorithmMachine<C, A>>,
 ) -> HashMap<Participant, AlgorithmMachine<C, A>> {
   let mut included = vec![];
   while included.len() < usize::from(keys[&Participant::new(1).unwrap()].params().t()) {
@@ -54,18 +54,28 @@ pub fn algorithm_machines<R: RngCore, C: Curve, A: Algorithm<C>>(
     included.push(n);
   }
 
-  keys
+  machines
-    .iter()
+    .into_iter()
-    .filter_map(|(i, keys)| {
+    .filter_map(|(i, machine)| if included.contains(&i) { Some((i, machine)) } else { None })
-      if included.contains(i) {
-        Some((*i, AlgorithmMachine::new(algorithm.clone(), keys.clone())))
-      } else {
-        None
-      }
-    })
     .collect()
 }
 
+/// Spawn algorithm machines for a random selection of signers, each executing the given algorithm.
+pub fn algorithm_machines<R: RngCore, C: Curve, A: Clone + Algorithm<C>>(
+  rng: &mut R,
+  algorithm: &A,
+  keys: &HashMap<Participant, ThresholdKeys<C>>,
+) -> HashMap<Participant, AlgorithmMachine<C, A>> {
+  algorithm_machines_without_clone(
+    rng,
+    keys,
+    keys
+      .values()
+      .map(|keys| (keys.params().i(), AlgorithmMachine::new(algorithm.clone(), keys.clone())))
+      .collect(),
+  )
+}
+
 // Run the preprocess step
 pub(crate) fn preprocess<
   R: RngCore + CryptoRng,
@@ -165,10 +175,10 @@ pub fn sign_without_caching<R: RngCore + CryptoRng, M: PreprocessMachine>(
 
 /// Execute the signing protocol, randomly caching various machines to ensure they can cache
 /// successfully.
-pub fn sign<R: RngCore + CryptoRng, M: PreprocessMachine>(
+pub fn sign_without_clone<R: RngCore + CryptoRng, M: PreprocessMachine>(
   rng: &mut R,
-  params: &<M::SignMachine as SignMachine<M::Signature>>::Params,
   mut keys: HashMap<Participant, <M::SignMachine as SignMachine<M::Signature>>::Keys>,
+  mut params: HashMap<Participant, <M::SignMachine as SignMachine<M::Signature>>::Params>,
   machines: HashMap<Participant, M>,
   msg: &[u8],
 ) -> M::Signature {
@@ -183,7 +193,8 @@ pub fn sign<R: RngCore + CryptoRng, M: PreprocessMachine>(
           let cache = machines.remove(&i).unwrap().cache();
           machines.insert(
             i,
-            M::SignMachine::from_cache(params.clone(), keys.remove(&i).unwrap(), cache).0,
+            M::SignMachine::from_cache(params.remove(&i).unwrap(), keys.remove(&i).unwrap(), cache)
+              .0,
           );
         }
       }
@@ -192,6 +203,22 @@ pub fn sign<R: RngCore + CryptoRng, M: PreprocessMachine>(
   )
 }
 
+/// Execute the signing protocol, randomly caching various machines to ensure they can cache
+/// successfully.
+pub fn sign<
+  R: RngCore + CryptoRng,
+  M: PreprocessMachine<SignMachine: SignMachine<M::Signature, Params: Clone>>,
+>(
+  rng: &mut R,
+  params: &<M::SignMachine as SignMachine<M::Signature>>::Params,
+  keys: HashMap<Participant, <M::SignMachine as SignMachine<M::Signature>>::Keys>,
+  machines: HashMap<Participant, M>,
+  msg: &[u8],
+) -> M::Signature {
+  let params = keys.keys().map(|i| (*i, params.clone())).collect();
+  sign_without_clone(rng, keys, params, machines, msg)
+}
+
 /// Test a basic Schnorr signature with the provided keys.
 pub fn test_schnorr_with_keys<R: RngCore + CryptoRng, C: Curve, H: Hram<C>>(
   rng: &mut R,

networks/monero/ringct/clsag/src/multisig.rs

@@ -57,11 +57,11 @@ impl ClsagContext {
 /// A channel to send the mask to use for the pseudo-out (rerandomized commitment) with.
 ///
 /// A mask must be sent along this channel before any preprocess addendums are handled.
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct ClsagMultisigMaskSender {
   buf: Arc<Mutex<Option<Scalar>>>,
 }
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 struct ClsagMultisigMaskReceiver {
   buf: Arc<Mutex<Option<Scalar>>>,
 }
@@ -73,6 +73,8 @@ impl ClsagMultisigMaskSender {
 
   /// Send a mask to a CLSAG multisig instance.
   pub fn send(self, mask: Scalar) {
+    // There is no risk this was prior set as this consumes `self`, which does not implement
+    // `Clone`
     *self.buf.lock() = Some(mask);
   }
 }
@@ -118,7 +120,7 @@ struct Interim {
 /// The message signed is expected to be a 32-byte value. Per Monero, it's the keccak256 hash of
 /// the transaction data which is signed. This will panic if the message is not a 32-byte value.
 #[allow(non_snake_case)]
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct ClsagMultisig {
   transcript: RecommendedTranscript,
 

networks/monero/ringct/clsag/src/tests.rs

@@ -19,7 +19,8 @@ use crate::ClsagMultisig;
 #[cfg(feature = "multisig")]
 use frost::{
   Participant,
-  tests::{key_gen, algorithm_machines, sign},
+  sign::AlgorithmMachine,
+  tests::{key_gen, algorithm_machines_without_clone, sign_without_clone},
 };
 
 const RING_LEN: u64 = 11;
@@ -99,6 +100,8 @@ fn clsag_multisig() {
     ring.push([dest, Commitment::new(mask, amount).calculate()]);
   }
 
+  let mask = Scalar::random(&mut OsRng);
+  let params = || {
     let (algorithm, mask_send) = ClsagMultisig::new(
       RecommendedTranscript::new(b"Monero Serai CLSAG Test"),
       ClsagContext::new(
@@ -107,13 +110,22 @@ fn clsag_multisig() {
       )
       .unwrap(),
     );
-  mask_send.send(Scalar::random(&mut OsRng));
+    mask_send.send(mask);
+    algorithm
+  };
 
-  sign(
+  sign_without_clone(
     &mut OsRng,
-    &algorithm,
     keys.clone(),
-    algorithm_machines(&mut OsRng, &algorithm, &keys),
+    keys.values().map(|keys| (keys.params().i(), params())).collect(),
+    algorithm_machines_without_clone(
+      &mut OsRng,
+      &keys,
+      keys
+        .values()
+        .map(|keys| (keys.params().i(), AlgorithmMachine::new(params(), keys.clone())))
+        .collect(),
+    ),
     &[1; 32],
   );
 }