From 875bda3f13b6ecb73352af2ac81bfcbec4b67e3e Mon Sep 17 00:00:00 2001
From: Innokentii Sennovskii <isennovskiy@gmail.com>
Date: Mon, 7 Nov 2022 19:10:41 +0000
Subject: [PATCH] Fixing errors in addition and subtraction of field elements
 with moduli > 254 bits (#1702)

* Added test

* Added bugfixes

* Added comments
---
 .../ecc/curves/secp256r1/secp256r1.test.cpp   | 20 ++++++++++++++++++
 src/aztec/ecc/fields/field_impl_generic.hpp   | 21 +++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/aztec/ecc/curves/secp256r1/secp256r1.test.cpp b/src/aztec/ecc/curves/secp256r1/secp256r1.test.cpp
index 1bf7aa76de6..28405903757 100644
--- a/src/aztec/ecc/curves/secp256r1/secp256r1.test.cpp
+++ b/src/aztec/ecc/curves/secp256r1/secp256r1.test.cpp
@@ -422,6 +422,26 @@ TEST(secp256r1, msb_bug_regression_check)
     EXPECT_EQ(result, expected_result);
 }
 
+/**
+ * @brief We had an issue where we added field elements and subtracted a prime depending on the 2²⁵⁶ overflow. This
+ * was incorrect. Sometimes we need to subtract the prime twice. The same is true for subtractions
+ *
+ */
+TEST(secp256r1, addition_subtraction_regression_check)
+{
+    secp256r1::fq fq1(uint256_t{ 0xfffffe0000000200, 0x200fffff9ff, 0xfffffbfffffffe00, 0xfffffbff00000400 });
+    secp256r1::fq fq2(uint256_t{ 0xfffffe0000000200, 0x200fffff9ff, 0xfffffbfffffffe00, 0xfffffbff00000400 });
+    secp256r1::fq fq3(0);
+    secp256r1::fq fq4(0);
+    fq1 += secp256r1::fq(secp256r1::fq::modulus_minus_two);
+    fq1 += secp256r1::fq(2);
+
+    fq3 -= fq1;
+    fq4 -= fq2;
+    EXPECT_EQ(fq1 + fq1, fq2 + fq2);
+    EXPECT_EQ(fq3, fq4);
+}
+
 /* TODO (#LARGE_MODULUS_AFFINE_POINT_COMPRESSION): Rewrite this test after designing point compression for p>2^255
 TEST(secp256r1, derive_generators)
 {
diff --git a/src/aztec/ecc/fields/field_impl_generic.hpp b/src/aztec/ecc/fields/field_impl_generic.hpp
index d1e0ef0bfc5..a0ba8f9ae21 100644
--- a/src/aztec/ecc/fields/field_impl_generic.hpp
+++ b/src/aztec/ecc/fields/field_impl_generic.hpp
@@ -207,6 +207,15 @@ template <class T> constexpr field<T> field<T>::add(const field& other) const no
             r1 = sbb(r1, modulus.data[1], b, b);
             r2 = sbb(r2, modulus.data[2], b, b);
             r3 = sbb(r3, modulus.data[3], b, b);
+            // Since both values are in [0, 2**256), the result is in [0, 2**257-2]. Subtracting one p might not be
+            // enough. We need to ensure that we've underflown the 0 and that might require subtracting an additional p
+            if (!b) {
+                b = 0;
+                r0 = sbb(r0, modulus.data[0], b, b);
+                r1 = sbb(r1, modulus.data[1], b, b);
+                r2 = sbb(r2, modulus.data[2], b, b);
+                r3 = sbb(r3, modulus.data[3], b, b);
+            }
         }
         return { r0, r1, r2, r3 };
     } else {
@@ -245,8 +254,16 @@ template <class T> constexpr field<T> field<T>::subtract(const field& other) con
     uint64_t carry = r0 < (modulus.data[0] & borrow);
     r1 = addc(r1, modulus.data[1] & borrow, carry, carry);
     r2 = addc(r2, modulus.data[2] & borrow, carry, carry);
-    r3 += (modulus.data[3] & borrow) + carry;
-
+    r3 = addc(r3, (modulus.data[3] & borrow), carry, carry);
+    // The value being subtracted is in [0, 2**256), if we subtract 0 - 2*255 and then add p, the value will stay
+    // negative. If we are adding p, we need to check that we've overflown 2**256. If not, we should add p again
+    if (!carry) {
+        r0 += (modulus.data[0] & borrow);
+        uint64_t carry = r0 < (modulus.data[0] & borrow);
+        r1 = addc(r1, modulus.data[1] & borrow, carry, carry);
+        r2 = addc(r2, modulus.data[2] & borrow, carry, carry);
+        r3 = addc(r3, (modulus.data[3] & borrow), carry, carry);
+    }
     return { r0, r1, r2, r3 };
 }