fix: cycle_group validate_is_on_curve bug (#4494)

Fixes the validate_is_on_curve function in cycle_group. Previously this
code would incorrectly succeed given a point that wasn't on a curve. It
multiplied by a boolean `is_point_at_infinity()`, which for normal
points, was false or 0, which set res to 0, thereby passing the
following check that res was 0. If a point was marked as the point at
infinity, then is_point_at_infinity() would be true or 1, and this would
incorrect fail even though the point was the point at infinity.

The fix is simple - reverse when we multiply by 1 and when we multiply
by 0. This will now correctly multiply by 0 when the point is marked as
the point at infinity, and also correctly multiply by 1 when the point
is not marked as infinity.

---------

Co-authored-by: Innokentii Sennovskii <isennovskiy@gmail.com>

barretenberg/cpp/src/barretenberg/join_split_example/proofs/join_split/join_split.test.cpp

@@ -707,7 +707,7 @@ TEST_F(join_split_tests, test_0_input_notes_and_detect_circuit_change)
     // The below part detects any changes in the join-split circuit
     constexpr uint32_t CIRCUIT_GATE_COUNT = 49492;
     constexpr uint32_t GATES_NEXT_POWER_OF_TWO = 65535;
-    const uint256_t VK_HASH("cc3b14fad5465fe9b8c714e8a5d79012b86a70f6e37dfc23054e6def7eb1770f");
+    const uint256_t VK_HASH("e253629a7f74dd33ac4288473df5fac928aae029cb8f5867bb413366b54c02ba");
 
     auto number_of_gates_js = result.number_of_gates;
     std::cout << get_verification_key()->sha256_hash() << std::endl;

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.cpp

@@ -160,7 +160,9 @@ template <typename Composer> void cycle_group<Composer>::validate_is_on_curve()
     auto xx = x * x;
     auto xxx = xx * x;
     auto res = y.madd(y, -xxx - Group::curve_b);
-    res *= is_point_at_infinity();
+    // if the point is marked as the point at infinity, then res should be changed to 0, but otherwise, we leave res
+    // unchanged from the original value
+    res *= !is_point_at_infinity();
     res.assert_is_zero();
 }
 

barretenberg/cpp/src/barretenberg/stdlib/primitives/group/cycle_group.test.cpp

@@ -48,6 +48,60 @@ template <class Builder> class CycleGroupTest : public ::testing::Test {
 using CircuitTypes = ::testing::Types<bb::StandardCircuitBuilder, bb::UltraCircuitBuilder>;
 TYPED_TEST_SUITE(CycleGroupTest, CircuitTypes);
 
+/**
+ * @brief Checks that a point on the curve passes the validate_is_on_curve check
+ *
+ */
+TYPED_TEST(CycleGroupTest, TestValidateOnCurveSucceed)
+{
+    STDLIB_TYPE_ALIASES;
+    Builder builder;
+
+    auto lhs = TestFixture::generators[0];
+    cycle_group_ct a = cycle_group_ct::from_witness(&builder, lhs);
+    a.validate_is_on_curve();
+    EXPECT_FALSE(builder.failed());
+    EXPECT_TRUE(builder.check_circuit());
+}
+
+/**
+ * @brief Checks that a point that is not on the curve but marked as the point at infinity passes the
+ * validate_is_on_curve check
+ * @details Should pass since marking it with _is_infinity=true makes whatever other point data invalid.
+ */
+TYPED_TEST(CycleGroupTest, TestValidateOnCurveInfinitySucceed)
+{
+    STDLIB_TYPE_ALIASES;
+    Builder builder;
+
+    auto x = stdlib::field_t<Builder>::from_witness(&builder, 1);
+    auto y = stdlib::field_t<Builder>::from_witness(&builder, 1);
+
+    cycle_group_ct a(x, y, /*_is_infinity=*/true); // marks this point as the point at infinity
+    a.validate_is_on_curve();
+    EXPECT_FALSE(builder.failed());
+    EXPECT_TRUE(builder.check_circuit());
+}
+
+/**
+ * @brief Checks that a point that is not on the curve but *not* marked as the point at infinity fails the
+ * validate_is_on_curve check
+ * @details (1, 1) is not on the either the Grumpkin curve or the BN254 curve.
+ */
+TYPED_TEST(CycleGroupTest, TestValidateOnCurveFail)
+{
+    STDLIB_TYPE_ALIASES;
+    Builder builder;
+
+    auto x = stdlib::field_t<Builder>::from_witness(&builder, 1);
+    auto y = stdlib::field_t<Builder>::from_witness(&builder, 1);
+
+    cycle_group_ct a(x, y, /*_is_infinity=*/false);
+    a.validate_is_on_curve();
+    EXPECT_TRUE(builder.failed());
+    EXPECT_FALSE(builder.check_circuit());
+}
+
 TYPED_TEST(CycleGroupTest, TestDbl)
 {
     STDLIB_TYPE_ALIASES;
@@ -436,8 +490,8 @@ TYPED_TEST(CycleGroupTest, TestBatchMul)
         EXPECT_TRUE(result.is_point_at_infinity().get_value());
     }
 
-    // case 5, fixed-base MSM with inputs that are combinations of constant and witnesses (group elements are in lookup
-    // table)
+    // case 5, fixed-base MSM with inputs that are combinations of constant and witnesses (group elements are in
+    // lookup table)
     {
         std::vector<cycle_group_ct> points;
         std::vector<typename cycle_group_ct::cycle_scalar> scalars;
@@ -465,8 +519,8 @@ TYPED_TEST(CycleGroupTest, TestBatchMul)
         EXPECT_EQ(result.get_value(), crypto::pedersen_commitment::commit_native(scalars_native));
     }
 
-    // case 6, fixed-base MSM with inputs that are combinations of constant and witnesses (some group elements are in
-    // lookup table)
+    // case 6, fixed-base MSM with inputs that are combinations of constant and witnesses (some group elements are
+    // in lookup table)
     {
         std::vector<cycle_group_ct> points;
         std::vector<typename cycle_group_ct::cycle_scalar> scalars;