From 7ce9def2b837bea83ad7848dd63de8b03ca70aa8 Mon Sep 17 00:00:00 2001
From: Chris Sosnin <48099298+slumber@users.noreply.github.com>
Date: Sat, 19 Aug 2023 23:51:58 +0300
Subject: [PATCH] Handle zero-case in group scalar multiplication (#124)

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
---
 CHANGELOG.md                               |  1 +
 Cargo.toml                                 |  1 +
 src/groups/curves/short_weierstrass/mod.rs | 72 +++++++++++++++++++++-
 src/pairing/mod.rs                         |  3 +-
 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2f7fa41..c0a1fa0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Pending
 - [\#117](https://github.com/arkworks-rs/r1cs-std/pull/117) Fix result of `precomputed_base_scalar_mul_le` to not discard previous value.
+- [\#124](https://github.com/arkworks-rs/r1cs-std/pull/124) Fix `scalar_mul_le` constraints unsatisfiability when short Weierstrass point is zero.
 
 ### Breaking changes
 
diff --git a/Cargo.toml b/Cargo.toml
index 0c63a8c..5757368 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -35,6 +35,7 @@ ark-mnt4-753 = { version = "0.4.0", features = ["curve"], default-features = fal
 ark-mnt6-298 = { version = "0.4.0", default-features = false  }
 ark-mnt6-753 = { version = "0.4.0", default-features = false  }
 ark-pallas = { version = "0.4.0", features = ["curve"],  default-features = false  }
+ark-bn254 = { version = "0.4.0", features = ["curve"], default-features = false  }
 
 [features]
 default = ["std"]
diff --git a/src/groups/curves/short_weierstrass/mod.rs b/src/groups/curves/short_weierstrass/mod.rs
index b41d959..9c36b50 100644
--- a/src/groups/curves/short_weierstrass/mod.rs
+++ b/src/groups/curves/short_weierstrass/mod.rs
@@ -509,8 +509,18 @@ where
         }
         let self_affine = self.to_affine()?;
         let (x, y, infinity) = (self_affine.x, self_affine.y, self_affine.infinity);
-        // We first handle the non-zero case, and then later
-        // will conditionally select zero if `self` was zero.
+        // We first handle the non-zero case, and then later will conditionally select
+        // zero if `self` was zero. However, we also want to make sure that generated
+        // constraints are satisfiable in both cases.
+        //
+        // In particular, using non-sensible values for `x` and `y` in zero-case may cause
+        // `unchecked` operations to generate constraints that can never be satisfied, depending
+        // on the curve equation coefficients.
+        //
+        // The safest approach is to use coordinates of some point from the curve, thus not
+        // violating assumptions of `NonZeroAffine`. For instance, generator point.
+        let x = infinity.select(&F::constant(P::GENERATOR.x), &x)?;
+        let y = infinity.select(&F::constant(P::GENERATOR.y), &y)?;
         let non_zero_self = NonZeroAffineVar::new(x, y);
 
         let mut bits = bits.collect::<Vec<_>>();
@@ -965,3 +975,61 @@ where
         Ok(bytes)
     }
 }
+
+#[cfg(test)]
+mod test_sw_curve {
+    use crate::{
+        alloc::AllocVar,
+        eq::EqGadget,
+        fields::{fp::FpVar, nonnative::NonNativeFieldVar},
+        groups::{curves::short_weierstrass::ProjectiveVar, CurveVar},
+        ToBitsGadget,
+    };
+    use ark_ec::{
+        short_weierstrass::{Projective, SWCurveConfig},
+        CurveGroup,
+    };
+    use ark_ff::PrimeField;
+    use ark_relations::r1cs::{ConstraintSystem, Result};
+    use ark_std::UniformRand;
+    use num_traits::Zero;
+
+    fn zero_point_scalar_mul_satisfied<G>() -> Result<bool>
+    where
+        G: CurveGroup,
+        G::BaseField: PrimeField,
+        G::Config: SWCurveConfig,
+    {
+        let mut rng = ark_std::test_rng();
+
+        let cs = ConstraintSystem::new_ref();
+        let point_in = Projective::<G::Config>::zero();
+        let point_out = Projective::<G::Config>::zero();
+        let scalar = G::ScalarField::rand(&mut rng);
+
+        let point_in =
+            ProjectiveVar::<G::Config, FpVar<G::BaseField>>::new_witness(cs.clone(), || {
+                Ok(point_in)
+            })?;
+        let point_out =
+            ProjectiveVar::<G::Config, FpVar<G::BaseField>>::new_input(cs.clone(), || {
+                Ok(point_out)
+            })?;
+        let scalar = NonNativeFieldVar::new_input(cs.clone(), || Ok(scalar))?;
+
+        let mul = point_in.scalar_mul_le(scalar.to_bits_le().unwrap().iter())?;
+
+        point_out.enforce_equal(&mul)?;
+
+        cs.is_satisfied()
+    }
+
+    #[test]
+    fn test_zero_point_scalar_mul() {
+        assert!(zero_point_scalar_mul_satisfied::<ark_bls12_381::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_pallas::Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_mnt4_298::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_mnt6_298::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_bn254::G1Projective>().unwrap());
+    }
+}
diff --git a/src/pairing/mod.rs b/src/pairing/mod.rs
index 958134e..55cc139 100644
--- a/src/pairing/mod.rs
+++ b/src/pairing/mod.rs
@@ -1,6 +1,5 @@
 use crate::prelude::*;
-use ark_ec::pairing::Pairing;
-use ark_ec::CurveGroup;
+use ark_ec::{pairing::Pairing, CurveGroup};
 use ark_ff::Field;
 use ark_relations::r1cs::SynthesisError;
 use core::fmt::Debug;