From fccb1aea51f0442efb297104910dd8642e58a84f Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 21 Apr 2024 23:45:07 -0400 Subject: [PATCH] Remove unsafe creation of dalek_ff_group::EdwardsPoint in BP+ --- coins/monero/src/ringct/bulletproofs/mod.rs | 15 +++---------- .../plus/aggregate_range_proof.rs | 22 ++++++++++++------- .../ringct/bulletproofs/plus/transcript.rs | 3 ++- .../plus/aggregate_range_proof.rs | 4 ++-- 4 files changed, 21 insertions(+), 23 deletions(-) diff --git a/coins/monero/src/ringct/bulletproofs/mod.rs b/coins/monero/src/ringct/bulletproofs/mod.rs index ce9f7492..6dc2297b 100644 --- a/coins/monero/src/ringct/bulletproofs/mod.rs +++ b/coins/monero/src/ringct/bulletproofs/mod.rs @@ -87,9 +87,8 @@ impl Bulletproofs { Ok(if !plus { Bulletproofs::Original(OriginalStruct::prove(rng, outputs)) } else { - use dalek_ff_group::EdwardsPoint as DfgPoint; Bulletproofs::Plus( - AggregateRangeStatement::new(outputs.iter().map(|com| DfgPoint(com.calculate())).collect()) + AggregateRangeStatement::new(outputs.iter().map(Commitment::calculate).collect()) .unwrap() .prove(rng, &Zeroizing::new(AggregateRangeWitness::new(outputs.to_vec()).unwrap())) .unwrap(), @@ -104,13 +103,7 @@ impl Bulletproofs { Bulletproofs::Original(bp) => bp.verify(rng, commitments), Bulletproofs::Plus(bp) => { let mut verifier = BatchVerifier::new(1); - // If this commitment is torsioned (which is allowed), this won't be a well-formed - // dfg::EdwardsPoint (expected to be of prime-order) - // The actual BP+ impl will perform a torsion clear though, making this safe - // TODO: Have AggregateRangeStatement take in dalek EdwardsPoint for clarity on this - let Some(statement) = AggregateRangeStatement::new( - commitments.iter().map(|c| dalek_ff_group::EdwardsPoint(*c)).collect(), - ) else { + let Some(statement) = AggregateRangeStatement::new(commitments.to_vec()) else { return false; }; if !statement.verify(rng, &mut verifier, (), bp.clone()) { @@ -135,9 +128,7 @@ impl Bulletproofs { match self { Bulletproofs::Original(bp) => bp.batch_verify(rng, verifier, id, commitments), Bulletproofs::Plus(bp) => { - let Some(statement) = AggregateRangeStatement::new( - commitments.iter().map(|c| dalek_ff_group::EdwardsPoint(*c)).collect(), - ) else { + let Some(statement) = AggregateRangeStatement::new(commitments.to_vec()) else { return false; }; statement.verify(rng, verifier, id, bp.clone()) diff --git a/coins/monero/src/ringct/bulletproofs/plus/aggregate_range_proof.rs b/coins/monero/src/ringct/bulletproofs/plus/aggregate_range_proof.rs index cba95014..62b4cb10 100644 --- a/coins/monero/src/ringct/bulletproofs/plus/aggregate_range_proof.rs +++ b/coins/monero/src/ringct/bulletproofs/plus/aggregate_range_proof.rs @@ -9,6 +9,7 @@ use group::{ ff::{Field, PrimeField}, Group, GroupEncoding, }; +use curve25519_dalek::EdwardsPoint as DalekPoint; use dalek_ff_group::{Scalar, EdwardsPoint}; use crate::{ @@ -28,7 +29,7 @@ use crate::{ #[derive(Clone, Debug)] pub(crate) struct AggregateRangeStatement { generators: Generators, - V: Vec, + V: Vec, } impl Zeroize for AggregateRangeStatement { @@ -57,7 +58,7 @@ pub struct AggregateRangeProof { } impl AggregateRangeStatement { - pub(crate) fn new(V: Vec) -> Option { + pub(crate) fn new(V: Vec) -> Option { if V.is_empty() || (V.len() > MAX_M) { return None; } @@ -98,11 +99,14 @@ impl AggregateRangeStatement { } let mn = V.len() * N; + // 2, 4, 6, 8... powers of z, of length equivalent to the amount of commitments let mut z_pow = Vec::with_capacity(V.len()); + // z**2 + z_pow.push(z * z); let mut d = ScalarVector::new(mn); for j in 1 ..= V.len() { - z_pow.push(z.pow(Scalar::from(2 * u64::try_from(j).unwrap()))); // TODO: Optimize this + z_pow.push(*z_pow.last().unwrap() * z_pow[0]); d = d + &(Self::d_j(j, V.len()) * (z_pow[j - 1])); } @@ -157,7 +161,7 @@ impl AggregateRangeStatement { return None; } for (commitment, witness) in self.V.iter().zip(witness.0.iter()) { - if witness.calculate() != **commitment { + if witness.calculate() != *commitment { return None; } } @@ -170,9 +174,9 @@ impl AggregateRangeStatement { // Commitments aren't transmitted INV_EIGHT though, so this multiplies by INV_EIGHT to enable // clearing its cofactor without mutating the value // For some reason, these values are transcripted * INV_EIGHT, not as transmitted - let mut V = V.into_iter().map(|V| EdwardsPoint(V.0 * crate::INV_EIGHT())).collect::>(); + let V = V.into_iter().map(|V| V * crate::INV_EIGHT()).collect::>(); let mut transcript = initial_transcript(V.iter()); - V.iter_mut().for_each(|V| *V = V.mul_by_cofactor()); + let mut V = V.into_iter().map(|V| EdwardsPoint(V.mul_by_cofactor())).collect::>(); // Pad V while V.len() < padded_pow_of_2(V.len()) { @@ -239,9 +243,11 @@ impl AggregateRangeStatement { ) -> bool { let Self { generators, V } = self; - let mut V = V.into_iter().map(|V| EdwardsPoint(V.0 * crate::INV_EIGHT())).collect::>(); + let V = V.into_iter().map(|V| V * crate::INV_EIGHT()).collect::>(); let mut transcript = initial_transcript(V.iter()); - V.iter_mut().for_each(|V| *V = V.mul_by_cofactor()); + // With the torsion clear, wrap it into a EdwardsPoint from dalek-ff-group + // (which is prime-order) + let V = V.into_iter().map(|V| EdwardsPoint(V.mul_by_cofactor())).collect::>(); let generators = generators.reduce(V.len() * N); diff --git a/coins/monero/src/ringct/bulletproofs/plus/transcript.rs b/coins/monero/src/ringct/bulletproofs/plus/transcript.rs index 2108013b..0e61fdc6 100644 --- a/coins/monero/src/ringct/bulletproofs/plus/transcript.rs +++ b/coins/monero/src/ringct/bulletproofs/plus/transcript.rs @@ -1,6 +1,7 @@ use std_shims::{sync::OnceLock, vec::Vec}; -use dalek_ff_group::{Scalar, EdwardsPoint}; +use curve25519_dalek::EdwardsPoint; +use dalek_ff_group::Scalar; use monero_generators::{hash_to_point as raw_hash_to_point}; use crate::{hash, hash_to_scalar as dalek_hash}; diff --git a/coins/monero/src/tests/bulletproofs/plus/aggregate_range_proof.rs b/coins/monero/src/tests/bulletproofs/plus/aggregate_range_proof.rs index 658da250..1a0e654d 100644 --- a/coins/monero/src/tests/bulletproofs/plus/aggregate_range_proof.rs +++ b/coins/monero/src/tests/bulletproofs/plus/aggregate_range_proof.rs @@ -2,7 +2,7 @@ use rand_core::{RngCore, OsRng}; use multiexp::BatchVerifier; use group::ff::Field; -use dalek_ff_group::{Scalar, EdwardsPoint}; +use dalek_ff_group::Scalar; use crate::{ Commitment, @@ -19,7 +19,7 @@ fn test_aggregate_range_proof() { for _ in 0 .. m { commitments.push(Commitment::new(*Scalar::random(&mut OsRng), OsRng.next_u64())); } - let commitment_points = commitments.iter().map(|com| EdwardsPoint(com.calculate())).collect(); + let commitment_points = commitments.iter().map(Commitment::calculate).collect(); let statement = AggregateRangeStatement::new(commitment_points).unwrap(); let witness = AggregateRangeWitness::new(commitments).unwrap();