🐛 fix inlined ec_Dbl in shamir-strauss

In the FCL repository, @nlordell pointed out the mulmuladd function had a bug revealed by the edge cases committed by @rdubois-crypto. This PR fixes that bug like @nlordell did and add the relevant edge cases. Link of the initial PR: rdubois-crypto/FreshCryptoLib#5 Co-authored-by: Nicholas Rodrigues Lordello <[email protected]> Co-authored-by: rdubois-crypto <[email protected]>
get-smooth · Dec 15, 2023 · 1d242a6 · 1d242a6
1 parent 6736fff
commit 1d242a6
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 6 deletions.
diff --git a/src/ECDSA256r1.sol b/src/ECDSA256r1.sol
@@ -131,15 +131,17 @@ library ECDSA256r1 {
                                 T3 := mulmod(X, T2, p)
                                 // W=UV
                                 T1 := mulmod(T1, T2, p)
-                                y2 := addmod(X, zz, p)
-                                let TT1 := addmod(X, sub(p, zz), p)
-                                // X-ZZ)(X+ZZ)
-                                y2 := mulmod(y2, TT1, p)
-                                // M
+
+                                //(X-ZZ)(X+ZZ) -- xPlusZZ/xMinusZZ needed for stack management
+                                let xPlusZZ := addmod(X, sub(p, zz), p)
+                                let xMinusZZ := addmod(X, zz, p)
+                                y2 := mulmod(xMinusZZ, xPlusZZ, p)
+
+                                //M=3*(X-ZZ)(X+ZZ)
                                 T4 := mulmod(3, y2, p)
 
                                 // zzz3=W*zzz1
-                                zzz := mulmod(TT1, zzz, p)
+                                zzz := mulmod(T1, zzz, p)
                                 // zz3=V*ZZ1, V free
                                 zz := mulmod(T2, zz, p)
 

diff --git a/test/unit/ecdsa256r1.t.sol b/test/unit/ecdsa256r1.t.sol
@@ -139,6 +139,26 @@ contract Ecdsa256r1Test is PRBTest {
         uint256 _4P_res3 = implementation.mulmuladd(nQ[0], nQ[1], 2, 1);
 
         assertEq(_4P_res1, _4P_res3);
+
+        // edge case from nlordell
+        (uint256 niordellX, uint256 niordellY, uint256 niordellU, uint256 niordellV) = (
+            0xe2534a3532d08fbba02dde659ee62bd0031fe2db785596ef509302446b030852, //x
+            0x1f0ea8a4b39cc339e62011a02579d289b103693d0cf11ffaa3bd3dc0e7b12739, //y
+            0xd13800358b760290af0671ee67368e9702a7145d1b9a0024b0b61ffe7bce9214, //u
+            0x344e000d62dd80a42bc19c7b99cda3a5c0a9c51746e680092c2d87ff9ef3af6f //v
+        );
+        assertEq(
+            0xcfcfa95b195904fd97b548d9e3cd2e023e06b4f10a87c645c7d4f74a0e206bad,
+            implementation.mulmuladd(niordellX, niordellY, niordellU, niordellV)
+        );
+
+        // edge case for Shamir
+        (uint256 shamirK, uint256 shamirX,) = (
+            0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc63254f, //k
+            0x7CF27B188D034F7E8A52380304B51AC3C08969E277F21B35A60B48FC47669978, // x
+            0xF888AAEE24712FC0D6C26539608BCF244582521AC3167DD661FB4862DD878C2E // y
+        );
+        assertEq(shamirX, implementation.mulmuladd(0, 0, shamirK, 0));
     }
 
     function test_MulMullAddMultipleBy0Fail_ReportSkip(uint256 q0, uint256 q1) public {