From ca2e738b677b946396193e98f8aaff0d8ecb219c Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Thu, 11 Apr 2024 20:23:30 -0400 Subject: [PATCH 1/4] catch panics from EC point creation (e.g. the point is at infinity) --- .gitignore | 1 + .../src/fixed_base_scalar_mul.rs | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 9a829afab8b..2c877a4d02c 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ examples/**/target/ examples/9 node_modules pkg/ +.idea # Yarn .pnp.* diff --git a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs index 5e68c7d4030..f1c77dfd434 100644 --- a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs +++ b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs @@ -53,8 +53,21 @@ pub fn embedded_curve_add( input2_x: FieldElement, input2_y: FieldElement, ) -> Result<(FieldElement, FieldElement), BlackBoxResolutionError> { - let mut point1 = grumpkin::SWAffine::new(input1_x.into_repr(), input1_y.into_repr()); - let point2 = grumpkin::SWAffine::new(input2_x.into_repr(), input2_y.into_repr()); + fn create_point( + x: FieldElement, + y: FieldElement, + ) -> Result { + match std::panic::catch_unwind(|| grumpkin::SWAffine::new(x.into_repr(), y.into_repr())) { + Ok(point) => Ok(point), + Err(_) => Err(BlackBoxResolutionError::Failed( + BlackBoxFunc::EmbeddedCurveAdd, + format!("Point ({}, {}) is not on curve", x.to_hex(), y.to_hex()), + )), + } + } + + let mut point1 = create_point(input1_x, input1_y)?; + let point2 = create_point(input2_x, input2_y)?; let res = point1 + point2; point1 = res.into(); if let Some((res_x, res_y)) = point1.xy() { From ae7e02476d928750459b7dcce71a0168cdf281ab Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Thu, 11 Apr 2024 20:31:37 -0400 Subject: [PATCH 2/4] add test --- .../src/fixed_base_scalar_mul.rs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs index f1c77dfd434..b1debf41c13 100644 --- a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs +++ b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs @@ -85,6 +85,7 @@ mod grumpkin_fixed_base_scalar_mul { use ark_ff::BigInteger; use super::*; + #[test] fn smoke_test() -> Result<(), BlackBoxResolutionError> { let input = FieldElement::one(); @@ -97,6 +98,7 @@ mod grumpkin_fixed_base_scalar_mul { assert_eq!(y, res.1.to_hex()); Ok(()) } + #[test] fn low_high_smoke_test() -> Result<(), BlackBoxResolutionError> { let low = FieldElement::one(); @@ -116,9 +118,9 @@ mod grumpkin_fixed_base_scalar_mul { let max_limb = FieldElement::from(u128::MAX); let invalid_limb = max_limb + FieldElement::one(); - let expected_error = Err(BlackBoxResolutionError::Failed( + let expected_error = Err(BlackBoxResolutionError::Failed( BlackBoxFunc::FixedBaseScalarMul, - "Limb 0000000000000000000000000000000100000000000000000000000000000000 is not less than 2^128".into() + "Limb 0000000000000000000000000000000100000000000000000000000000000000 is not less than 2^128".into(), )); let res = fixed_base_scalar_mul(&invalid_limb, &FieldElement::zero()); @@ -141,7 +143,23 @@ mod grumpkin_fixed_base_scalar_mul { res, Err(BlackBoxResolutionError::Failed( BlackBoxFunc::FixedBaseScalarMul, - "30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47 is not a valid grumpkin scalar".into() + "30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47 is not a valid grumpkin scalar".into(), + )) + ); + } + + #[test] + fn rejects_addition_of_points_not_in_curve() { + let x = FieldElement::from(1u128); + let y = FieldElement::from(2u128); + + let res = embedded_curve_add(x, y, x, y); + + assert_eq!( + res, + Err(BlackBoxResolutionError::Failed( + BlackBoxFunc::EmbeddedCurveAdd, + "Point (0000000000000000000000000000000000000000000000000000000000000001, 0000000000000000000000000000000000000000000000000000000000000002) is not on curve".into(), )) ); } From e461c8dc8f1204340668320d411120794d772812 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 15 Apr 2024 13:20:35 -0400 Subject: [PATCH 3/4] clean based on review comments --- .../src/fixed_base_scalar_mul.rs | 42 +++++++++++-------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs index b1debf41c13..43dde849327 100644 --- a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs +++ b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs @@ -47,30 +47,36 @@ pub fn fixed_base_scalar_mul( } } +fn create_point( + x: FieldElement, + y: FieldElement, +) -> Result { + let point = grumpkin::SWAffine::new_unchecked(x.into_repr(), y.into_repr()); + if !point.is_on_curve() { + return Err(format!("Point ({}, {}) is not on curve", x.to_hex(), y.to_hex())); + }; + if !point.is_in_correct_subgroup_assuming_on_curve() { + return Err(format!("Point ({}, {}) is not in correct subgroup", x.to_hex(), y.to_hex())); + }; + Ok(point) +} + pub fn embedded_curve_add( input1_x: FieldElement, input1_y: FieldElement, input2_x: FieldElement, input2_y: FieldElement, ) -> Result<(FieldElement, FieldElement), BlackBoxResolutionError> { - fn create_point( - x: FieldElement, - y: FieldElement, - ) -> Result { - match std::panic::catch_unwind(|| grumpkin::SWAffine::new(x.into_repr(), y.into_repr())) { - Ok(point) => Ok(point), - Err(_) => Err(BlackBoxResolutionError::Failed( - BlackBoxFunc::EmbeddedCurveAdd, - format!("Point ({}, {}) is not on curve", x.to_hex(), y.to_hex()), - )), - } - } - - let mut point1 = create_point(input1_x, input1_y)?; - let point2 = create_point(input2_x, input2_y)?; - let res = point1 + point2; - point1 = res.into(); - if let Some((res_x, res_y)) = point1.xy() { + let point1 = create_point(input1_x, input1_y).map_err(|e| BlackBoxResolutionError::Failed( + BlackBoxFunc::EmbeddedCurveAdd, + e, + ))?; + let point2 = create_point(input2_x, input2_y).map_err(|e| BlackBoxResolutionError::Failed( + BlackBoxFunc::EmbeddedCurveAdd, + e, + ))?; + let res = grumpkin::SWAffine::from(point1 + point2); + if let Some((res_x, res_y)) = res.xy() { Ok((FieldElement::from_repr(*res_x), FieldElement::from_repr(*res_y))) } else { Err(BlackBoxResolutionError::Failed( From ea23f74c84d120c46791ffc8b505165febb0ede6 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 15 Apr 2024 19:17:06 +0100 Subject: [PATCH 4/4] chore: fmt --- .../src/fixed_base_scalar_mul.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs index 43dde849327..cd91c290f49 100644 --- a/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs +++ b/acvm-repo/bn254_blackbox_solver/src/fixed_base_scalar_mul.rs @@ -47,10 +47,7 @@ pub fn fixed_base_scalar_mul( } } -fn create_point( - x: FieldElement, - y: FieldElement, -) -> Result { +fn create_point(x: FieldElement, y: FieldElement) -> Result { let point = grumpkin::SWAffine::new_unchecked(x.into_repr(), y.into_repr()); if !point.is_on_curve() { return Err(format!("Point ({}, {}) is not on curve", x.to_hex(), y.to_hex())); @@ -67,14 +64,10 @@ pub fn embedded_curve_add( input2_x: FieldElement, input2_y: FieldElement, ) -> Result<(FieldElement, FieldElement), BlackBoxResolutionError> { - let point1 = create_point(input1_x, input1_y).map_err(|e| BlackBoxResolutionError::Failed( - BlackBoxFunc::EmbeddedCurveAdd, - e, - ))?; - let point2 = create_point(input2_x, input2_y).map_err(|e| BlackBoxResolutionError::Failed( - BlackBoxFunc::EmbeddedCurveAdd, - e, - ))?; + let point1 = create_point(input1_x, input1_y) + .map_err(|e| BlackBoxResolutionError::Failed(BlackBoxFunc::EmbeddedCurveAdd, e))?; + let point2 = create_point(input2_x, input2_y) + .map_err(|e| BlackBoxResolutionError::Failed(BlackBoxFunc::EmbeddedCurveAdd, e))?; let res = grumpkin::SWAffine::from(point1 + point2); if let Some((res_x, res_y)) = res.xy() { Ok((FieldElement::from_repr(*res_x), FieldElement::from_repr(*res_y)))