Add is_valid_secp256_signature support (#1189)

* feat: add function to validate secp256r1 signatures * feat: add logic and CHANGELOG entries * docs: add entry for get_secp256r1_keys_from * feat: add PR number to CHANGELOG entries * Update packages/account/src/utils/secp256_point.cairo Co-authored-by: immrsd <[email protected]> * Update packages/account/src/tests/test_signature.cairo Co-authored-by: immrsd <[email protected]> * Update packages/account/src/tests/test_eth_account.cairo Co-authored-by: immrsd <[email protected]> * Update packages/account/src/tests/test_eth_account.cairo Co-authored-by: immrsd <[email protected]> * refactor: function --------- Co-authored-by: immrsd <[email protected]>
OpenZeppelin · Oct 29, 2024 · 52551a6 · 52551a6
1 parent 0a63799
commit 52551a6
Show file tree

Hide file tree

Showing 15 changed files with 235 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,13 +11,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- `is_valid_p256_signature` utility function to `openzeppelin_account::utils::signature` (#1189)
+- `Secp256r1KeyPair` type and helpers to `openzeppelin_testing::signing` (#1189)
 - Embeddable impls for ERC2981 component (#1173)
   - `ERC2981Info` with read functions for discovering the component's state
   - `ERC2981AdminOwnable` providing admin functions for a token that implements Ownable component
   - `ERC2981AdminAccessControl` providing admin functions for a token that implements AccessControl component
 
 ### Changed (Breaking)
 
+- Refactor `openzeppelin_account::utils::secp256k1` module to `openzeppelin_account::utils::secp256_point` (#1189)
+  - `Secp256k1PointStorePacking` replaced by a generic `Secp256PointStorePacking`
+  - `Secp256k1PointPartialEq` replaced by a generic `Secp256PointPartialEq`
+  - `DebugSecp256k1Point` replaced by a generic `DebugSecp256Point`
 - Apply underscore pattern to the internal functions of `ERC2981Component` to prevent collisions
 with new external functions (#1173)
 

diff --git a/docs/modules/ROOT/pages/api/testing.adoc b/docs/modules/ROOT/pages/api/testing.adoc
@@ -2,6 +2,7 @@
 
 :stark: https://docs.starknet.io/architecture-and-concepts/cryptography/stark-curve/[Stark]
 :secp256k1: https://github.com/starkware-libs/cairo/blob/main/corelib/src/starknet/secp256k1.cairo[Secp256k1]
+:secp256r1: https://www.nervos.org/knowledge-base/what_is_secp256r1[Secp256r1]
 
 This crate provides various helper functions for declaring, deploying,
 and testing smart contracts using the `snforge` toolchain from Starknet Foundry.
@@ -267,6 +268,7 @@ A module offering utility functions for easier management of key pairs and signa
 .Functions
 * xref:#testing-signing-get_stark_keys_from[`++get_stark_keys_from(private_key)++`]
 * xref:#testing-signing-get_secp256k1_keys_from[`++get_secp256k1_keys_from(private_key)++`]
+* xref:#testing-signing-get_secp256r1_keys_from[`++get_secp256r1_keys_from(private_key)++`]
 
 .Traits
 * xref:#testing-signing-SerializedSigning[`++SerializedSigning++`]
@@ -287,6 +289,12 @@ Builds a {stark} key pair from a private key represented by a `felt252` value.
 
 Builds a {secp256k1} key pair from a private key represented by a `u256` value.
 
+[.contract-item]
+[[testing-signing-get_secp256r1_keys_from]]
+==== `[.contract-item-name]#++get_secp256r1_keys_from++#++(private_key: u256) → Secp256r1KeyPair++` [.item-kind]#function#
+
+Builds a {secp256r1} key pair from a private key represented by a `u256` value.
+
 [#testing-signing-Traits]
 ==== Traits
 

diff --git a/packages/account/src/eth_account.cairo b/packages/account/src/eth_account.cairo
@@ -12,7 +12,7 @@ pub mod EthAccountComponent {
     use core::starknet::secp256_trait::Secp256PointTrait;
     use crate::interface::EthPublicKey;
     use crate::interface;
-    use crate::utils::secp256k1::Secp256k1PointStorePacking;
+    use crate::utils::secp256_point::Secp256PointStorePacking;
     use crate::utils::{MIN_TRANSACTION_VERSION, QUERY_OFFSET};
     use crate::utils::{execute_calls, is_valid_eth_signature};
     use openzeppelin_introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait;

diff --git a/packages/account/src/interface.cairo b/packages/account/src/interface.cairo
@@ -4,6 +4,7 @@
 use starknet::account::Call;
 
 pub type EthPublicKey = starknet::secp256k1::Secp256k1Point;
+pub type P256PublicKey = starknet::secp256r1::Secp256r1Point;
 
 pub const ISRC6_ID: felt252 = 0x2ceccef7f994940b3962a6c67e0ba4fcd37df7d131417c604f91e03caecc1cd;
 

diff --git a/packages/account/src/tests.cairo b/packages/account/src/tests.cairo
@@ -2,5 +2,5 @@ mod extensions;
 
 mod test_account;
 mod test_eth_account;
-mod test_secp256k1;
+mod test_secp256_point;
 mod test_signature;
diff --git a/packages/account/src/tests/test_eth_account.cairo b/packages/account/src/tests/test_eth_account.cairo
@@ -3,8 +3,8 @@ use crate::EthAccountComponent::{PublicKeyCamelImpl, PublicKeyImpl};
 use crate::EthAccountComponent;
 use crate::interface::{EthAccountABIDispatcherTrait, EthAccountABIDispatcher};
 use crate::interface::{ISRC6, ISRC6_ID};
-use crate::utils::secp256k1::{DebugSecp256k1Point, Secp256k1PointPartialEq};
-use crate::utils::signature::EthSignature;
+use crate::utils::secp256_point::{DebugSecp256Point, Secp256PointPartialEq};
+use crate::utils::signature::Secp256Signature;
 use openzeppelin_introspection::interface::{ISRC5, ISRC5_ID};
 use openzeppelin_test_common::eth_account::EthAccountSpyHelpers;
 use openzeppelin_test_common::eth_account::{
@@ -364,14 +364,14 @@ fn test_account_called_from_contract() {
 //
 
 #[test]
-#[should_panic(expected: ('Secp256k1Point: Invalid point.',))]
+#[should_panic(expected: 'Secp256Point: Invalid point.')]
 fn test_cannot_get_without_initialize() {
     let state = COMPONENT_STATE();
     state.get_public_key();
 }
 
 #[test]
-#[should_panic(expected: ('Secp256k1Point: Invalid point.',))]
+#[should_panic(expected: 'Secp256Point: Invalid point.')]
 fn test_cannot_set_without_initialize() {
     let key_pair = KEY_PAIR();
     let mut state = COMPONENT_STATE();
@@ -529,7 +529,7 @@ fn test_assert_valid_new_owner_invalid_signature() {
 
     start_cheat_caller_address(test_address(), test_address());
     let mut bad_signature = array![];
-    EthSignature { r: 'BAD'.into(), s: 'SIG'.into() }.serialize(ref bad_signature);
+    Secp256Signature { r: 'BAD'.into(), s: 'SIG'.into() }.serialize(ref bad_signature);
     let new_key_pair = KEY_PAIR_2();
 
     state

diff --git a/...es/account/src/tests/test_secp256k1.cairo → ...ccount/src/tests/test_secp256_point.cairo b/...es/account/src/tests/test_secp256k1.cairo → ...ccount/src/tests/test_secp256_point.cairo
@@ -1,5 +1,5 @@
-use crate::utils::secp256k1::{
-    DebugSecp256k1Point, Secp256k1PointPartialEq, Secp256k1PointStorePacking as StorePacking
+use crate::utils::secp256_point::{
+    DebugSecp256Point, Secp256PointPartialEq, Secp256PointStorePacking as StorePacking
 };
 use starknet::SyscallResultTrait;
 use starknet::secp256_trait::{Secp256Trait, Secp256PointTrait};
@@ -11,7 +11,6 @@ fn test_pack_big_secp256k1_points() {
     let curve_size = Secp256Trait::<Secp256k1Point>::get_curve_size();
 
     // Check point 1
-
     let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_1);
     let xhigh_and_parity: u256 = xhigh_and_parity.into();
 
@@ -24,7 +23,6 @@ fn test_pack_big_secp256k1_points() {
     assert_eq!(parity, true, "Parity should be odd");
 
     // Check point 2
-
     let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_2);
     let xhigh_and_parity: u256 = xhigh_and_parity.into();
 
@@ -42,21 +40,21 @@ fn test_unpack_big_secp256k1_points() {
     let (big_point_1, big_point_2) = get_points();
 
     // Check point 1
-
     let (expected_x, expected_y) = big_point_1.get_coordinates().unwrap_syscall();
 
     let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_1);
-    let (x, y) = StorePacking::unpack((xlow, xhigh_and_parity)).get_coordinates().unwrap_syscall();
+    let point: Secp256k1Point = StorePacking::unpack((xlow, xhigh_and_parity));
+    let (x, y) = point.get_coordinates().unwrap_syscall();
 
     assert_eq!(x, expected_x);
     assert_eq!(y, expected_y);
 
     // Check point 2
-
     let (expected_x, _) = big_point_2.get_coordinates().unwrap_syscall();
 
     let (xlow, xhigh_and_parity) = StorePacking::pack(big_point_2);
-    let (x, _) = StorePacking::unpack((xlow, xhigh_and_parity)).get_coordinates().unwrap_syscall();
+    let point: Secp256k1Point = StorePacking::unpack((xlow, xhigh_and_parity));
+    let (x, _) = point.get_coordinates().unwrap_syscall();
 
     assert_eq!(x, expected_x);
     assert_eq!(y, expected_y);

diff --git a/packages/account/src/tests/test_signature.cairo b/packages/account/src/tests/test_signature.cairo
@@ -1,9 +1,16 @@
-use crate::utils::signature::{is_valid_stark_signature, is_valid_eth_signature};
+use crate::interface::P256PublicKey;
+use crate::utils::signature::{
+    is_valid_stark_signature, is_valid_eth_signature, is_valid_p256_signature
+};
+use openzeppelin_account::utils::signature::Secp256Signature;
 use openzeppelin_test_common::account::SIGNED_TX_DATA as stark_signature_data;
 use openzeppelin_test_common::eth_account::SIGNED_TX_DATA as eth_signature_data;
-use openzeppelin_testing::constants::{stark, secp256k1};
+use openzeppelin_testing::constants::{TRANSACTION_HASH, stark, secp256k1, secp256r1};
+use openzeppelin_testing::signing::{Secp256r1KeyPair, Secp256r1SerializedSigning};
+use snforge_std::signature::secp256r1_curve::Secp256r1CurveSignerImpl;
 use starknet::secp256_trait::Secp256Trait;
 use starknet::secp256k1::Secp256k1Point;
+use starknet::secp256r1::Secp256r1Point;
 
 //
 // is_valid_stark_signature
@@ -86,7 +93,7 @@ fn test_is_valid_eth_signature_invalid_format_sig() {
 }
 
 #[test]
-fn test_signature_r_out_of_range() {
+fn test_eth_signature_r_out_of_range() {
     let key_pair = secp256k1::KEY_PAIR();
     let data = eth_signature_data(key_pair);
     let mut bad_signature = data.signature;
@@ -106,7 +113,7 @@ fn test_signature_r_out_of_range() {
 }
 
 #[test]
-fn test_signature_s_out_of_range() {
+fn test_eth_signature_s_out_of_range() {
     let key_pair = secp256k1::KEY_PAIR();
     let data = eth_signature_data(key_pair);
     let mut bad_signature = data.signature;
@@ -124,3 +131,108 @@ fn test_signature_s_out_of_range() {
     );
     assert!(is_invalid);
 }
+
+//
+// is_valid_p256_signature
+//
+
+#[derive(Drop)]
+pub struct SignedTransactionData {
+    pub private_key: u256,
+    pub public_key: P256PublicKey,
+    pub tx_hash: felt252,
+    pub signature: Secp256Signature
+}
+
+fn p256_signature_data(key_pair: Secp256r1KeyPair) -> SignedTransactionData {
+    let tx_hash = TRANSACTION_HASH;
+    let (r, s) = key_pair.sign(tx_hash.into()).unwrap();
+    SignedTransactionData {
+        private_key: key_pair.secret_key,
+        public_key: key_pair.public_key,
+        tx_hash,
+        signature: Secp256Signature { r, s }
+    }
+}
+
+#[test]
+fn test_is_valid_p256_signature_good_sig() {
+    let key_pair = secp256r1::KEY_PAIR();
+    let data = p256_signature_data(key_pair);
+
+    let mut serialized_good_signature = array![];
+    data.signature.serialize(ref serialized_good_signature);
+
+    let is_valid = is_valid_p256_signature(
+        data.tx_hash, key_pair.public_key, serialized_good_signature.span()
+    );
+    assert!(is_valid);
+}
+
+#[test]
+fn test_is_valid_p256_signature_bad_sig() {
+    let key_pair = secp256r1::KEY_PAIR();
+    let data = p256_signature_data(key_pair);
+    let mut bad_signature = data.signature;
+
+    bad_signature.r += 1;
+
+    let mut serialized_bad_signature = array![];
+
+    bad_signature.serialize(ref serialized_bad_signature);
+
+    let is_invalid = !is_valid_p256_signature(
+        data.tx_hash, key_pair.public_key, serialized_bad_signature.span()
+    );
+    assert!(is_invalid);
+}
+
+#[test]
+#[should_panic(expected: 'Signature: Invalid format.')]
+fn test_is_valid_p256_signature_invalid_format_sig() {
+    let key_pair = secp256r1::KEY_PAIR();
+    let data = p256_signature_data(key_pair);
+    let mut serialized_bad_signature = array![0x1];
+
+    is_valid_p256_signature(data.tx_hash, key_pair.public_key, serialized_bad_signature.span());
+}
+
+#[test]
+fn test_p256_signature_r_out_of_range() {
+    let key_pair = secp256r1::KEY_PAIR();
+    let data = p256_signature_data(key_pair);
+    let mut bad_signature = data.signature;
+
+    let curve_size = Secp256Trait::<Secp256r1Point>::get_curve_size();
+
+    bad_signature.r = curve_size + 1;
+
+    let mut serialized_bad_signature = array![];
+
+    bad_signature.serialize(ref serialized_bad_signature);
+
+    let is_invalid = !is_valid_p256_signature(
+        data.tx_hash, key_pair.public_key, serialized_bad_signature.span()
+    );
+    assert!(is_invalid);
+}
+
+#[test]
+fn test_p256_signature_s_out_of_range() {
+    let key_pair = secp256r1::KEY_PAIR();
+    let data = p256_signature_data(key_pair);
+    let mut bad_signature = data.signature;
+
+    let curve_size = Secp256Trait::<Secp256r1Point>::get_curve_size();
+
+    bad_signature.s = curve_size + 1;
+
+    let mut serialized_bad_signature = array![];
+
+    bad_signature.serialize(ref serialized_bad_signature);
+
+    let is_invalid = !is_valid_p256_signature(
+        data.tx_hash, key_pair.public_key, serialized_bad_signature.span()
+    );
+    assert!(is_invalid);
+}
diff --git a/packages/account/src/utils.cairo b/packages/account/src/utils.cairo
@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: MIT
 // OpenZeppelin Contracts for Cairo v0.18.0 (account/utils.cairo)
 
-pub mod secp256k1;
+pub mod secp256_point;
 pub mod signature;
 
-pub use signature::{is_valid_stark_signature, is_valid_eth_signature};
+pub use signature::{is_valid_stark_signature, is_valid_eth_signature, is_valid_p256_signature};
 
 use starknet::SyscallResultTrait;
 use starknet::account::Call;