finding 7: add subgroup check on g2 points.

Author: suyash67

barretenberg/cpp/src/barretenberg/bbapi/bbapi_ecc.cpp

@@ -99,6 +99,12 @@ Bn254G2Mul::Response Bn254G2Mul::execute(BBApiRequest& request) &&
     if (!point.on_curve()) {
         BBAPI_ERROR(request, "Input point must be on the curve");
     }
+    // BN254 G2 has cofactor h2 ≈ 2^254. An on-curve point may lie in a cofactor subgroup of order
+    // dividing h2 rather than the prime-order subgroup; we do not want to allow such points
+    // as inputs to bbapi.
+    if (!point.is_in_prime_subgroup()) {
+        BBAPI_ERROR(request, "Input point must lie in the prime-order subgroup");
+    }
     auto result = point * scalar;
     if (!result.on_curve()) {
         BBAPI_ERROR(request, "Output point must be on the curve");

barretenberg/cpp/src/barretenberg/bbapi/bbapi_srs.cpp

@@ -50,8 +50,15 @@ SrsInitSrs::Response SrsInitSrs::execute(BB_UNUSED BBApiRequest& request) &&
                        std::to_string(bytes_per_point));
     }
 
-    // Parse G2 point from buffer (128 bytes)
+    // Parse G2 point from buffer (128 bytes). `serialize_from_buffer` validates that the bytes
+    // decode to a curve point but does NOT enforce subgroup membership. BN254 G2 has a non-trivial
+    // cofactor (h2 ≈ 2^254), so a curve point may lie in a small cofactor subgroup of order
+    // dividing h2 rather than the prime-order subgroup of order r. Reject anything outside
+    // the prime-order subgroup before it reaches the SRS factory.
     auto g2_point_elem = from_buffer<g2::affine_element>(g2_point.data());
+    if (!g2_point_elem.is_in_prime_subgroup()) {
+        throw_or_abort("SrsInitSrs: g2_point is not in the BN254 G2 prime-order subgroup");
+    }
 
     // Initialize BN254 SRS
     bb::srs::init_bn254_mem_crs_factory(g1_points, g2_point_elem);

barretenberg/cpp/src/barretenberg/ecc/curves/bn254/g2.test.cpp

@@ -182,3 +182,39 @@ TEST(g2, GeneratorIsCorrect)
                                       fq("0x090689d0585ff075ec9e99ad690c3395bc4b313370b38ef355acdadcd122975b") } };
     EXPECT_EQ(generator, expected);
 }
+
+// The generator, infinity, and arbitrary scalar multiples of the generator must be accepted as
+// members of the BN254 G2 prime-order subgroup.
+TEST(g2, IsInPrimeSubgroupAcceptsSubgroupPoints)
+{
+    const g2::affine_element gen(Bn254G2Params::one_x, Bn254G2Params::one_y);
+    EXPECT_TRUE(gen.is_in_prime_subgroup());
+    EXPECT_TRUE(g2::affine_element::infinity().is_in_prime_subgroup());
+
+    for (size_t i = 0; i < 4; ++i) {
+        const g2::affine_element P(g2::element(gen) * fr::random_element());
+        EXPECT_TRUE(P.is_in_prime_subgroup());
+    }
+}
+
+// BN254 G2 has cofactor h2 ≈ 2^254, so on-curve does NOT imply prime-order subgroup membership. The hardcoded point
+// below was constructed by sampling x = i + u (for the smallest positive integer i that yields a curve point) and
+// recovering y via Fq2 sqrt; because only a 1/h2 fraction of E'(Fq2) lies in G_r, this specimen lies in a cofactor
+// subgroup. Such a point must be rejected. Coordinates are in Montgomery form to match `Bn254G2Params::one_x` etc.
+TEST(g2, IsInPrimeSubgroupRejectsCofactorPoint)
+{
+    const g2::affine_element off_subgroup{
+        fq2{ fq{ 0xa6ba871b8b1e1b3a, 0x14f1d651eb8e167b, 0xccdd46def0f28c58, 0x1c14ef83340fbe5e },
+             fq{ 0xd35d438dc58f0d9d, 0x0a78eb28f5c70b3d, 0x666ea36f7879462c, 0x0e0a77c19a07df2f } },
+        fq2{ fq{ 0x0294a5225573dc93, 0x53874be07988f4f1, 0x2a05d8b41ccce7d3, 0x20045194f06acd0e },
+             fq{ 0x3814c8e5e4179a98, 0x793241f4d911e617, 0x28cf8e4b0df4482e, 0x0d612bd6f79bd361 } }
+    };
+    ASSERT_TRUE(off_subgroup.on_curve());
+    EXPECT_FALSE(off_subgroup.is_in_prime_subgroup());
+
+    // Sanity check that scalar multiplication via the Fr-typed `*` operator does NOT detect
+    // subgroup membership — multiplying by `Fr(0)` (the additive identity, which equals `r mod r`)
+    // gives infinity for every input, including off-subgroup points. This is precisely why
+    // is_in_prime_subgroup() routes through a uint256_t scalar instead.
+    EXPECT_TRUE((off_subgroup * fr::zero()).is_point_at_infinity());
+}

barretenberg/cpp/src/barretenberg/ecc/curves/bn254/pairing.hpp

@@ -195,10 +195,13 @@ constexpr fq12 final_exponentiation_tricky_part(const fq12& elt);
 // ======================
 // Pairing
 //
-// NOTE: All points supplied for pairing calculations are checked to be on the curve. This is equivalent to a subgroup
-// membership check for points in G1 = BN254. We don't implement subgroup membership checks for G2 because the only
-// place in the codebase where we use pairings is in PairingPoints::check(), which takes two points P1, P2 in G1
-// and checks e(P1, [1]) * e(P2, [x]) = 1. The points [1] and [x] are taken from the SRS, so we know they belong to G2.
+// NOTE: All points supplied for pairing calculations are checked to be on the curve. For G1 = BN254 this is equivalent
+// to a subgroup membership check (cofactor 1). For G2 = BN254 twist, on-curve does NOT imply membership in the
+// prime-order subgroup because the cofactor is non-trivial. The two G2 points consumed inside pairings come from
+// PairingPoints::check(), which feeds them the SRS values [1]_2 and [x]_2 — the SRS ingress (bbapi::SrsInitSrs)
+// invokes `affine_element::is_in_prime_subgroup()` on the user-supplied [x]_2 before installation, so by the time
+// they reach this file they are already known to be in the prime-order subgroup. Any future code path that brings a
+// new G2 point in from outside the SRS must run the subgroup check itself.
 //
 // ======================
 

barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.hpp

@@ -92,6 +92,22 @@ template <typename Fq_, typename Fr_, typename Params_> class alignas(64) affine
 
     [[nodiscard]] constexpr bool on_curve() const noexcept;
 
+    /**
+     * @brief Check that the point lies in the prime-order subgroup of size `Fr::modulus`.
+     *
+     * @details For curves whose cofactor is 1 (e.g. BN254 G1, Grumpkin) every on-curve point trivially
+     * satisfies this, so callers that already validated `on_curve()` can skip the call. For curves with
+     * non-trivial cofactor (notably BN254 G2, with cofactor h2 ≈ 2^254), `on_curve()` alone is
+     * insufficient: an attacker can supply a curve point that lies in a small subgroup of order
+     * dividing h2 and pass `on_curve()`. This routine performs the full subgroup check via `[r]·P == ∞`.
+     *
+     * @note Not constant-time. Intended for one-shot validation of public, externally-supplied points
+     * (e.g. at the bbapi boundary or when loading SRS bytes).
+     *
+     * @return true iff `*this` is the point at infinity or has order dividing `Fr::modulus`.
+     */
+    [[nodiscard]] bool is_in_prime_subgroup() const noexcept;
+
     static constexpr std::optional<affine_element> derive_from_x_coordinate(const Fq& x, bool sign_bit) noexcept;
 
     /**

barretenberg/cpp/src/barretenberg/ecc/groups/affine_element.test.cpp

@@ -349,6 +349,19 @@ template <typename G1> class TestAffineElement : public testing::Test {
         auto reconstructed = FrCodec::deserialize_from_fields<affine_element>(limbs);
         EXPECT_EQ(reconstructed, point);
     }
+
+    // The point at infinity, the generator, and any scalar multiple of the generator must all be
+    // recognized as members of the prime-order subgroup.
+    static void test_is_in_prime_subgroup_accepts_subgroup_points()
+    {
+        EXPECT_TRUE(affine_element::infinity().is_in_prime_subgroup());
+        EXPECT_TRUE(affine_element::one().is_in_prime_subgroup());
+
+        for (size_t i = 0; i < 8; ++i) {
+            affine_element P = affine_element(element::random_element());
+            EXPECT_TRUE(P.is_in_prime_subgroup());
+        }
+    }
 };
 
 // using TestTypes = testing::Types<bb::g1>;
@@ -539,6 +552,12 @@ TYPED_TEST(TestAffineElement, DeserializeOffCurveThrows)
     TestFixture::test_deserialize_off_curve_throws();
 }
 
+// Verify is_in_prime_subgroup accepts known prime-order subgroup points
+TYPED_TEST(TestAffineElement, IsInPrimeSubgroupAcceptsSubgroupPoints)
+{
+    TestFixture::test_is_in_prime_subgroup_accepts_subgroup_points();
+}
+
 // Verify that from_compressed returns the (0,0) sentinel for x values with no valid y.
 TYPED_TEST(TestAffineElement, PointCompressionInvalidX)
 {

barretenberg/cpp/src/barretenberg/ecc/groups/affine_element_impl.hpp

@@ -135,6 +135,29 @@ template <class Fq, class Fr, class T> constexpr bool affine_element<Fq, Fr, T>:
     return (xxx == yy);
 }
 
+template <class Fq, class Fr, class T> bool affine_element<Fq, Fr, T>::is_in_prime_subgroup() const noexcept
+{
+    if (is_point_at_infinity()) {
+        return true;
+    }
+    using Element = element<Fq, Fr, T>;
+
+    // To compute r * P, we convert modulus r to u256 and perform a left-to-right double-and-add.
+    constexpr uint256_t r = Fr::modulus;
+    const uint64_t r_msb = r.get_msb();
+
+    // Left-to-right double-and-add over the bits of r below the MSB. The MSB itself is consumed by
+    // initializing `acc` with `*this`. Loop terminates via unsigned underflow (i wraps past 0).
+    Element acc(*this);
+    for (uint64_t i = r_msb - 1; i < r_msb; --i) {
+        acc.self_dbl();
+        if (r.get_bit(i)) {
+            acc += *this;
+        }
+    }
+    return acc.is_point_at_infinity();
+}
+
 template <class Fq, class Fr, class T>
 constexpr bool affine_element<Fq, Fr, T>::operator==(const affine_element& other) const noexcept
 {

barretenberg/cpp/src/barretenberg/ecc/groups/element.hpp

@@ -22,10 +22,12 @@ namespace bb::group_elements {
  * @brief element class. Implements ecc group arithmetic using Jacobian coordinates
  * See https://hyperelliptic.org/EFD/g1p/auto-shortw-jacobian-0.html#doubling-dbl-2009-l
  *
- * Note: Currently subgroup checks are NOT IMPLEMENTED
- * Our current implementation uses G1 points that have a cofactor of 1.
- * All G2 points are precomputed (generator [1]_2 and trusted setup point [x]_2).
- * Explicitly assume precomputed points are valid members of the prime-order subgroup for G2.
+ * Note: BN254 / Grumpkin G1 have cofactor 1, so on-curve membership coincides with prime-order
+ * subgroup membership. BN254 G2 has a non-trivial cofactor; an explicit subgroup check is provided
+ * by `affine_element::is_in_prime_subgroup()` and must be applied to externally-supplied G2 bytes
+ * (see bbapi). The arithmetic in this file does not rederive subgroup membership and assumes the
+ * caller already ensured operands are valid prime-order subgroup elements.
+ *
  * @tparam Fq prime field the curve is defined over
  * @tparam Fr prime field whose characteristic equals the size of the prime-order elliptic curve subgroup
  * @tparam Params curve parameters

barretenberg/cpp/src/barretenberg/ecc/groups/group.hpp

@@ -24,10 +24,12 @@ namespace bb {
  * @brief group class. Represents an elliptic curve group element.
  * Group is parametrised by Fq and Fr
  *
- * Note: Currently subgroup checks are NOT IMPLEMENTED
- * Our current implementation uses G1 points that have a cofactor of 1.
- * All G2 points are precomputed (generator [1]_2 and trusted setup point [x]_2).
- * Explicitly assume precomputed points are valid members of the prime-order subgroup for G2.
+ * Note: BN254 / Grumpkin G1 have cofactor 1, so `affine_element::on_curve()` is itself a subgroup
+ * check. BN254 G2 has a non-trivial cofactor, so callers that accept externally-supplied G2 bytes
+ * must additionally invoke `affine_element::is_in_prime_subgroup()` to reject cofactor-subgroup
+ * points before they reach pairing-based verifiers; routine internal G2 arithmetic stays inside
+ * the prime-order subgroup because every starting point is the precomputed generator [1]_2 or the
+ * SRS point [x]_2.
  *
  * @tparam Fq
  * @tparam subgroup_field

barretenberg/cpp/src/barretenberg/srs/factories/crs_factory.test.cpp

@@ -123,6 +123,16 @@ TEST(CrsFactory, DISABLED_Bn254Fallback)
     fs::remove_all(temp_crs_path);
 }
 
+// The hardcoded `[x]_2` baked into the BB native binary must be a member of the BN254 G2
+// prime-order subgroup. Pinning this in a test catches accidental corruption in any future change
+// to `bn254_crs_data.hpp` (or to the deserialization path).
+TEST(CrsFactory, Bn254HardcodedG2IsInPrimeSubgroup)
+{
+    auto g2_point = bb::srs::get_bn254_g2_crs_element();
+    ASSERT_TRUE(g2_point.on_curve());
+    EXPECT_TRUE(g2_point.is_in_prime_subgroup());
+}
+
 TEST(CrsFactory, Bn254CompressedChunkHashFirstChunk)
 {
     // Download the first chunk of compressed CRS from CDN and verify its hash.