From ab4d79628d6345383dc46bfea3b93d92ba2ecffc Mon Sep 17 00:00:00 2001
From: Boog900 <54e72d8a-345f-4599-bd90-c6b9bc7d0ec5@aleeas.com>
Date: Tue, 9 Apr 2024 20:14:52 +0100
Subject: [PATCH] fix CLSAG verification. We were not setting c1 to the last
 calculated c during verification, instead keeping it set to the one provided
 in the signature.

---
 coins/monero/src/ringct/clsag/mod.rs | 16 ++++++++--------
 coins/monero/src/tests/clsag.rs      |  7 ++++++-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/coins/monero/src/ringct/clsag/mod.rs b/coins/monero/src/ringct/clsag/mod.rs
index 1290e3e3..fd8253e8 100644
--- a/coins/monero/src/ringct/clsag/mod.rs
+++ b/coins/monero/src/ringct/clsag/mod.rs
@@ -9,7 +9,7 @@ use std_shims::{
 use rand_core::{RngCore, CryptoRng};
 
 use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing};
-use subtle::{ConstantTimeEq, Choice, CtOption};
+use subtle::{ConstantTimeEq, ConditionallySelectable};
 
 use curve25519_dalek::{
   constants::ED25519_BASEPOINT_TABLE,
@@ -169,13 +169,8 @@ fn core(
   }
 
   // Perform the core loop
-  let mut c1 = CtOption::new(Scalar::ZERO, Choice::from(0));
+  let mut c1 = c;
   for i in (start .. end).map(|i| i % n) {
-    // This will only execute once and shouldn't need to be constant time. Making it constant time
-    // removes the risk of branch prediction creating timing differences depending on ring index
-    // however
-    c1 = c1.or_else(|| CtOption::new(c, i.ct_eq(&0)));
-
     let c_p = mu_P * c;
     let c_c = mu_C * c;
 
@@ -188,10 +183,15 @@ fn core(
     to_hash.extend(L.compress().to_bytes());
     to_hash.extend(R.compress().to_bytes());
     c = hash_to_scalar(&to_hash);
+
+    // This will only execute once and shouldn't need to be constant time. Making it constant time
+    // removes the risk of branch prediction creating timing differences depending on ring index
+    // however
+    c1.conditional_assign(&c, i.ct_eq(&(n - 1)));
   }
 
   // This first tuple is needed to continue signing, the latter is the c to be tested/worked with
-  ((D, c * mu_P, c * mu_C), c1.unwrap_or(c))
+  ((D, c * mu_P, c * mu_C), c1)
 }
 
 /// CLSAG signature, as used in Monero.
diff --git a/coins/monero/src/tests/clsag.rs b/coins/monero/src/tests/clsag.rs
index 59e41ebf..a17d7ba2 100644
--- a/coins/monero/src/tests/clsag.rs
+++ b/coins/monero/src/tests/clsag.rs
@@ -57,7 +57,7 @@ fn clsag() {
     }
 
     let image = generate_key_image(&secrets.0);
-    let (clsag, pseudo_out) = Clsag::sign(
+    let (mut clsag, pseudo_out) = Clsag::sign(
       &mut OsRng,
       vec![(
         secrets.0,
@@ -76,7 +76,12 @@ fn clsag() {
       msg,
     )
     .swap_remove(0);
+
     clsag.verify(&ring, &image, &pseudo_out, &msg).unwrap();
+
+    // make sure verification fails if we throw a random `c1` at it.
+    clsag.c1 = random_scalar(&mut OsRng);
+    assert!(clsag.verify(&ring, &image, &pseudo_out, &msg).is_err());
   }
 }