From f2e1f6139296ec1a4169753061adbbd7069d3678 Mon Sep 17 00:00:00 2001 From: qd-qd Date: Wed, 5 Jul 2023 11:08:13 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=A5=20Revised=20all=20verify=20functio?= =?UTF-8?q?n=20signatures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Transitioned rs parameter from memory layout to stack layout across all implementations - Shifted Q parameter from memory layout to stack layout in basic implementation This pivotal change, though breaking, was driven by the necessity to reduce interaction costs with the verify function, the library's sole user-facing feature. --- src/ECDSA256PrecomputeInternal.sol | 13 ++++--- src/ECDSA256r1.sol | 20 +++++----- src/ECDSA256r1Precompute.sol | 13 ++++--- test/ecdsa256r1.t.sol | 54 ++++++++++++++------------- test/ecdsa256r1Precomput.t.sol | 60 +++++++++++++++--------------- 5 files changed, 84 insertions(+), 76 deletions(-) diff --git a/src/ECDSA256PrecomputeInternal.sol b/src/ECDSA256PrecomputeInternal.sol index 856d903..4423696 100644 --- a/src/ECDSA256PrecomputeInternal.sol +++ b/src/ECDSA256PrecomputeInternal.sol @@ -143,22 +143,23 @@ library ECDSA256r1PrecomputeInternal { /// @notice Verifies an ECDSA signature using a precomputed table of multiples of P and Q stored in the bytecode of /// the contrat that uses this library /// @param message The message that was signed. - /// @param rs uint256[2] The r and s values of the ECDSA signature. + /// @param r uint256 The r value of the ECDSA signature. + /// @param s uint256 The s value of the ECDSA signature. /// @param precomputedOffset The **offset** where the precomputed points starts in the bytecode /// @return True if the signature is valid, false otherwise. /// @dev Note the required interactions with the precompled contract can revert the transaction - function verify(bytes32 message, uint256[2] calldata rs, uint256 precomputedOffset) external returns (bool) { + function verify(bytes32 message, uint256 r, uint256 s, uint256 precomputedOffset) external returns (bool) { // check the validity of the signature - if (rs[0] == 0 || rs[0] >= n || rs[1] == 0) { + if (r == 0 || r >= n || s == 0) { return false; } // preform the Shamir's trick in order to calculate the x coordinate of the point - uint256 sInv = rs[1].nModInv(); - uint256 X = mulmuladd(mulmod(uint256(message), sInv, n), mulmod(rs[0], sInv, n), precomputedOffset); + uint256 sInv = s.nModInv(); + uint256 X = mulmuladd(mulmod(uint256(message), sInv, n), mulmod(r, sInv, n), precomputedOffset); assembly { - X := addmod(X, sub(n, calldataload(rs)), n) + X := addmod(X, sub(n, r), n) } return X == 0; diff --git a/src/ECDSA256r1.sol b/src/ECDSA256r1.sol index 8d61a17..447c8d8 100644 --- a/src/ECDSA256r1.sol +++ b/src/ECDSA256r1.sol @@ -192,30 +192,32 @@ library ECDSA256r1 { /// @notice Verifies an ECDSA signature on the secp256r1 curve given the message, signature, and public key. /// This function is the only one exposed by the library /// @param message The original message that was signed - /// @param rs uint256[2] The r and s values of the ECDSA signature. - /// @param Q The public key used for the signature, in the format [Qx, Qy] + /// @param r uint256 The r value of the ECDSA signature. + /// @param s uint256 The s value of the ECDSA signature. + /// @param qx The x value of the public key used for the signature + /// @param qy The y value of the public key used for the signature /// @return bool True if the signature is valid, false otherwise /// @dev Note the required interactions with the precompled contract can revert the transaction - function verify(bytes32 message, uint256[2] calldata rs, uint256[2] calldata Q) external returns (bool) { + function verify(bytes32 message, uint256 r, uint256 s, uint256 qx, uint256 qy) external returns (bool) { // check the validity of the signature - if (rs[0] == 0 || rs[0] >= n || rs[1] == 0 || rs[1] >= n) { + if (r == 0 || r >= n || s == 0 || s >= n) { return false; } // check the public key is on the curve - if (!ECDSA.affIsOnCurve(Q[0], Q[1])) { + if (!ECDSA.affIsOnCurve(qx, qy)) { return false; } // calculate the scalars used for the multiplication of the point - uint256 sInv = rs[1].nModInv(); + uint256 sInv = s.nModInv(); uint256 scalar_u = mulmod(uint256(message), sInv, n); - uint256 scalar_v = mulmod(rs[0], sInv, n); + uint256 scalar_v = mulmod(r, sInv, n); - uint256 x1 = mulmuladd(Q[0], Q[1], scalar_u, scalar_v); + uint256 x1 = mulmuladd(qx, qy, scalar_u, scalar_v); assembly { - x1 := addmod(x1, sub(n, calldataload(rs)), n) + x1 := addmod(x1, sub(n, r), n) } return x1 == 0; diff --git a/src/ECDSA256r1Precompute.sol b/src/ECDSA256r1Precompute.sol index 414d12c..11e8624 100644 --- a/src/ECDSA256r1Precompute.sol +++ b/src/ECDSA256r1Precompute.sol @@ -181,24 +181,25 @@ library ECDSA256r1Precompute { /// @notice Verifies an ECDSA signature using a precomputed table of multiples of P and Q stored in an external /// contract /// @param message The message to verify - /// @param rs tuple [r, s] representing the ECDSA signature + /// @param r uint256 The r value of the ECDSA signature. + /// @param s uint256 The s value of the ECDSA signature. /// @param precomputedTable The address of the external contract containing the precomputations for Shamir's trick. /// It is expected the contract store the precomputations as its bytecode. The contract is not supposed /// to be functional. /// @return True if the signature is valid, false otherwise. /// @dev Note the required interactions with the precompled contract can revert the transaction - function verify(bytes32 message, uint256[2] calldata rs, address precomputedTable) external returns (bool) { + function verify(bytes32 message, uint256 r, uint256 s, address precomputedTable) external returns (bool) { // check the validity of the signature - if (rs[0] == 0 || rs[0] >= n || rs[1] == 0 || rs[1] >= n) { + if (r == 0 || r >= n || s == 0 || s >= n) { return false; } // perform the Shamir's trick in order to calculate the x coordinate of the point - uint256 sInv = rs[1].nModInv(); - uint256 X = mulmuladd(mulmod(uint256(message), sInv, n), mulmod(rs[0], sInv, n), precomputedTable); + uint256 sInv = s.nModInv(); + uint256 X = mulmuladd(mulmod(uint256(message), sInv, n), mulmod(r, sInv, n), precomputedTable); assembly { - X := addmod(X, sub(n, calldataload(rs)), n) + X := addmod(X, sub(n, r), n) } return X == 0; diff --git a/test/ecdsa256r1.t.sol b/test/ecdsa256r1.t.sol index 380bc12..60e8201 100644 --- a/test/ecdsa256r1.t.sol +++ b/test/ecdsa256r1.t.sol @@ -6,14 +6,18 @@ import { ECDSA, p, gx, gy, n } from "../src/utils/ECDSA.sol"; import { ECDSA256r1 } from "../src/ECDSA256r1.sol"; struct TestVectors { - uint256[2] pubKey; + // public key + uint256 qx; + uint256 qy; + // number of tests uint256 numtests; + // JSON string containing the test vectors string fixtures; } contract ImplementationECDSA256r1 { - function verify(bytes32 message, uint256[2] memory rs, uint256[2] memory pubKey) external returns (bool) { - return ECDSA256r1.verify(message, rs, pubKey); + function verify(bytes32 message, uint256 r, uint256 s, uint256 qx, uint256 qy) external returns (bool) { + return ECDSA256r1.verify(message, r, s, qx, qy); } function mulmuladd(uint256 q0, uint256 q1, uint256 n0, uint256 n1) external returns (uint256) { @@ -50,31 +54,29 @@ contract Ecdsa256r1Test is PRBTest { /// @return testvectors The test vectors function _loadFixtures(bool flag) internal returns (TestVectors memory testvectors) { // all the content of the JSON file - string memory fixtures = + testvectors.fixtures = flag == true ? vm.readFile(VALID_VECTOR_FILE_PATH) : vm.readFile(INVALID_VECTOR_FILE_PATH); - uint256[2] memory pubKey; - pubKey[0] = vm.parseJsonUint(fixtures, ".keyx"); - pubKey[1] = vm.parseJsonUint(fixtures, ".keyy"); - uint256 numtests = vm.parseJsonUint(fixtures, ".NumberOfTests"); - - return TestVectors(pubKey, numtests, fixtures); + testvectors.qx = vm.parseJsonUint(testvectors.fixtures, ".keyx"); + testvectors.qy = vm.parseJsonUint(testvectors.fixtures, ".keyy"); + testvectors.numtests = vm.parseJsonUint(testvectors.fixtures, ".NumberOfTests"); } /// @notice get the test vector n from the JSON string /// @param fixtures The JSON string containing the test vectors /// @param id The test vector number to get - /// @return rs The signature (r, s) of the test vector + /// @return r uint256 The r value of the ECDSA signature. + /// @return s uint256 The s value of the ECDSA signature. /// @return message The message of the test vector function _getTestVector( string memory fixtures, string memory id ) internal - returns (uint256[2] memory rs, bytes32 message) + returns (uint256 r, uint256 s, bytes32 message) { - rs[0] = vm.parseJsonUint(fixtures, string.concat(".sigx_", id)); - rs[1] = vm.parseJsonUint(fixtures, string.concat(".sigy_", id)); + r = vm.parseJsonUint(fixtures, string.concat(".sigx_", id)); + s = vm.parseJsonUint(fixtures, string.concat(".sigy_", id)); message = vm.parseJsonBytes32(fixtures, string.concat(".msg_", id)); } @@ -86,9 +88,9 @@ contract Ecdsa256r1Test is PRBTest { for (uint256 i = 1; i <= testVectors.numtests; i++) { // get the test vector (message, signature) - (uint256[2] memory rs, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); + (uint256 r, uint256 s, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); // run the verification function with the test vector - bool isStandardValid = implementation.verify(message, rs, testVectors.pubKey); + bool isStandardValid = implementation.verify(message, r, s, testVectors.qx, testVectors.qy); // ensure the result is the expected one assertEq(isStandardValid, flag); } @@ -136,43 +138,43 @@ contract Ecdsa256r1Test is PRBTest { // test invalid vectors, all assert shall be false function test_VerifySignatureValidity() public { // expect to fail because rs[0] == 0 - bool isValid = implementation.verify(bytes32("hello"), [uint256(0), uint256(1)], [uint256(1), uint256(1)]); + bool isValid = implementation.verify(bytes32("hello"), uint256(0), uint256(1), uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because rs[1] == 0 - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(0)], [uint256(1), uint256(1)]); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(0), uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because rs[0] > n - isValid = implementation.verify(bytes32("hello"), [n + 1, uint256(1)], [uint256(1), uint256(1)]); + isValid = implementation.verify(bytes32("hello"), n + 1, uint256(1), uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because rs[0] == n - isValid = implementation.verify(bytes32("hello"), [n, uint256(1)], [uint256(1), uint256(1)]); + isValid = implementation.verify(bytes32("hello"), n, uint256(1), uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because rs[1] > n - isValid = implementation.verify(bytes32("hello"), [uint256(1), n + 1], [uint256(1), uint256(1)]); + isValid = implementation.verify(bytes32("hello"), uint256(1), n + 1, uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because rs[1] == n - isValid = implementation.verify(bytes32("hello"), [uint256(1), n], [uint256(1), uint256(1)]); + isValid = implementation.verify(bytes32("hello"), uint256(1), n, uint256(1), uint256(1)); assertFalse(isValid); // expect to fail because q[0] == 0 (affine coordinates not on the curve) - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(1)], [0, uint256(1)]); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(1), 0, uint256(1)); assertFalse(isValid); // expect to fail because q[1] == 0 (affine coordinates not on the curve) - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(1)], [uint256(1), 0]); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(1), uint256(1), 0); assertFalse(isValid); // expect to fail because q[0] == p (affine coordinates not on the curve) - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(1)], [p, uint256(1)]); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(1), p, uint256(1)); assertFalse(isValid); // expect to fail because q[1] == p (affine coordinates not on the curve) - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(1)], [uint256(1), p]); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(1), uint256(1), p); assertFalse(isValid); } } diff --git a/test/ecdsa256r1Precomput.t.sol b/test/ecdsa256r1Precomput.t.sol index 45dcfa4..2d9ec58 100644 --- a/test/ecdsa256r1Precomput.t.sol +++ b/test/ecdsa256r1Precomput.t.sol @@ -7,14 +7,18 @@ import { ECDSA, Curve, n } from "../src/utils/ECDSA.sol"; import { ECDSA256r1Precompute } from "../src/ECDSA256r1Precompute.sol"; struct TestVectors { - uint256[2] pubKey; + // public key + uint256 qx; + uint256 qy; + // number of tests uint256 numtests; + // JSON string containing the test vectors string fixtures; } contract ImplementationECDSA256r1Precompute { - function verify(bytes32 message, uint256[2] calldata rs, address precomputedTable) external returns (bool) { - return ECDSA256r1Precompute.verify(message, rs, precomputedTable); + function verify(bytes32 message, uint256 r, uint256 s, address precomputedTable) external returns (bool) { + return ECDSA256r1Precompute.verify(message, r, s, precomputedTable); } function mulmuladd(uint256 scalar_u, uint256 scalar_v, address precomputedTable) external returns (uint256) { @@ -64,31 +68,29 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { /// @return testvectors The test vectors function _loadFixtures(bool flag) internal returns (TestVectors memory testvectors) { // all the content of the JSON file - string memory fixtures = + testvectors.fixtures = flag == true ? vm.readFile(VALID_VECTOR_FILE_PATH) : vm.readFile(INVALID_VECTOR_FILE_PATH); - uint256[2] memory pubKey; - pubKey[0] = vm.parseJsonUint(fixtures, ".keyx"); - pubKey[1] = vm.parseJsonUint(fixtures, ".keyy"); - uint256 numtests = vm.parseJsonUint(fixtures, ".NumberOfTests"); - - return TestVectors(pubKey, numtests, fixtures); + testvectors.qx = vm.parseJsonUint(testvectors.fixtures, ".keyx"); + testvectors.qy = vm.parseJsonUint(testvectors.fixtures, ".keyy"); + testvectors.numtests = vm.parseJsonUint(testvectors.fixtures, ".NumberOfTests"); } /// @notice get the test vector n from the JSON string /// @param fixtures The JSON string containing the test vectors /// @param id The test vector number to get - /// @return rs The signature (r, s) of the test vector + /// @return r uint256 The r value of the ECDSA signature. + /// @return s uint256 The s value of the ECDSA signature. /// @return message The message of the test vector function _getTestVector( string memory fixtures, string memory id ) internal - returns (uint256[2] memory rs, bytes32 message) + returns (uint256 r, uint256 s, bytes32 message) { - rs[0] = vm.parseJsonUint(fixtures, string.concat(".sigx_", id)); - rs[1] = vm.parseJsonUint(fixtures, string.concat(".sigy_", id)); + r = vm.parseJsonUint(fixtures, string.concat(".sigx_", id)); + s = vm.parseJsonUint(fixtures, string.concat(".sigy_", id)); message = vm.parseJsonBytes32(fixtures, string.concat(".msg_", id)); } @@ -112,7 +114,7 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { /// @dev Uses `_precomputeShamirTable` function to generate the precomputed table modifier _preparePrecomputeTable() { // generate the precomputed table - bytes memory precompute = _precomputeShamirTable(validVectors.pubKey[0], validVectors.pubKey[1]); + bytes memory precompute = _precomputeShamirTable(validVectors.qx, validVectors.qy); // set the precomputed points as the bytecode of the target contract vm.etch(precomputeAddress, precompute); @@ -155,9 +157,9 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { for (uint256 i = 1; i <= testVectors.numtests; i++) { // get the test vector (message, signature) - (uint256[2] memory rs, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); + (uint256 r, uint256 s, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); // run the verification function with the test vector - bool isValid = implementation.verify(message, rs, precomputeAddress); + bool isValid = implementation.verify(message, r, s, precomputeAddress); // ensure the result is the expected one assertTrue(isValid); } @@ -171,9 +173,9 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { for (uint256 i = 1; i <= testVectors.numtests; i++) { // get the test vector (message, signature) - (uint256[2] memory rs, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); + (uint256 r, uint256 s, bytes32 message) = _getTestVector(testVectors.fixtures, vm.toString(i)); // run the verification function with the test vector - bool isValid = implementation.verify(message, rs, precomputeAddress); + bool isValid = implementation.verify(message, r, s, precomputeAddress); // ensure the result is the expected one assertFalse(isValid); } @@ -183,27 +185,27 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { /// @dev Ensures that incorrect signatures are not valid function test_VerifyIncorrectSignatureFail() external _preparePrecomputeTable { // expect to fail because rs[0] == 0 - bool isValid = implementation.verify(bytes32("hello"), [uint256(0), uint256(1)], precomputeAddress); + bool isValid = implementation.verify(bytes32("hello"), uint256(0), uint256(1), precomputeAddress); assertFalse(isValid); // expect to fail because rs[1] == 0 - isValid = implementation.verify(bytes32("hello"), [uint256(1), uint256(0)], precomputeAddress); + isValid = implementation.verify(bytes32("hello"), uint256(1), uint256(0), precomputeAddress); assertFalse(isValid); // expect to fail because rs[0] > n - isValid = implementation.verify(bytes32("hello"), [n + 1, uint256(1)], precomputeAddress); + isValid = implementation.verify(bytes32("hello"), n + 1, uint256(1), precomputeAddress); assertFalse(isValid); // expect to fail because rs[0] == n - isValid = implementation.verify(bytes32("hello"), [n, uint256(1)], precomputeAddress); + isValid = implementation.verify(bytes32("hello"), n, uint256(1), precomputeAddress); assertFalse(isValid); // expect to fail because rs[1] > n - isValid = implementation.verify(bytes32("hello"), [uint256(1), n + 1], precomputeAddress); + isValid = implementation.verify(bytes32("hello"), uint256(1), n + 1, precomputeAddress); assertFalse(isValid); // expect to fail because rs[1] == n - isValid = implementation.verify(bytes32("hello"), [uint256(1), n], precomputeAddress); + isValid = implementation.verify(bytes32("hello"), uint256(1), n, precomputeAddress); assertFalse(isValid); } @@ -211,11 +213,11 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { /// @dev Ensures that incorrect addresses fail verification (0x00 + precompile addresses + some extra empty ones) function test_VerifyIncorectAddressesFail() external _preparePrecomputeTable { // get a valid test vector (message, signature) - (uint256[2] memory rs, bytes32 message) = _getTestVector(validVectors.fixtures, "1"); + (uint256 r, uint256 s, bytes32 message) = _getTestVector(validVectors.fixtures, "1"); for (uint160 addr = 0; addr <= 20; addr++) { // run the verification function with the correct test vector BUT an invalid address - bool isValid = implementation.verify(message, rs, address(addr)); + bool isValid = implementation.verify(message, r, s, address(addr)); assertFalse(isValid); } } @@ -229,7 +231,7 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { TestVectors memory testVectors = invalidVectors; // get the test vector (message, signature) - (uint256[2] memory rs, bytes32 message) = _getTestVector(testVectors.fixtures, "1"); + (uint256 r,, bytes32 message) = _getTestVector(testVectors.fixtures, "1"); // minimal fixtures uint256 sInv = @@ -239,7 +241,7 @@ contract Ecdsa256r1PrecomputTest is StdUtils, PRBTest { // run the verification function with the test vector and the fixtures uint256 result = - implementation.mulmuladd(mulmod(uint256(message), sInv, n), mulmod(rs[0], sInv, n), precomputeAddress); + implementation.mulmuladd(mulmod(uint256(message), sInv, n), mulmod(r, sInv, n), precomputeAddress); assertEq(result, expected); }