From ea05584ff61b3229626198061919dcf7a902fc89 Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Thu, 18 Apr 2024 11:41:42 +0200 Subject: [PATCH] Remove y from EthAccount signature. (#940) * feat: remove y from signature * feat: update CHANGELOG * feat: apply update review * refactor: Signature to EthSignature * fix: tests * feat: apply update reviews --- CHANGELOG.md | 2 ++ src/account/utils/signature.cairo | 32 +++++++------------ src/tests/account/test_dual_eth_account.cairo | 1 - src/tests/account/test_eth_account.cairo | 7 ++-- 4 files changed, 16 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1b367890..5a5d25d3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `IERC721::name`, `IERC721::symbol`, and `IERC721Metadata::token_uri` return ByteArrays instead of felts (#857) - `InternalTrait::initializer` accepts an additional `base_uri` ByteArray parameter (#857) - IERC721Metadata SRC5 interface ID. This is changed because of the ByteArray integration (#857) +- EthAccount + - Expected signature format changed from `(r, s, y)` to `(r, s)` (#940) ### Removed diff --git a/src/account/utils/signature.cairo b/src/account/utils/signature.cairo index 747d91e8a..3c3372add 100644 --- a/src/account/utils/signature.cairo +++ b/src/account/utils/signature.cairo @@ -3,12 +3,15 @@ use ecdsa::check_ecdsa_signature; use openzeppelin::account::interface::EthPublicKey; -use openzeppelin::account::utils::secp256k1::Secp256k1PointPartialEq; -use starknet::eth_signature::Signature; -use starknet::secp256_trait::{is_signature_entry_valid, recover_public_key}; -use starknet::secp256k1::Secp256k1Point; +use starknet::secp256_trait; -/// This function assumes the `s` component of the signature to be positive +#[derive(Copy, Drop, Serde)] +pub struct EthSignature { + pub r: u256, + pub s: u256, +} + +/// This function assumes the `s` component of the signature to be positive /// for efficiency reasons. It is not recommended to use it other than for /// validating account signatures over transaction hashes since otherwise /// it's not protected against signature malleability. @@ -25,7 +28,7 @@ fn is_valid_stark_signature( } } -/// This function assumes the `s` component of the signature to be positive +/// This function assumes the `s` component of the signature to be positive /// for efficiency reasons. It is not recommended to use it other than for /// validating account signatures over transaction hashes since otherwise /// it's not protected against signature malleability. @@ -34,21 +37,8 @@ fn is_valid_eth_signature( msg_hash: felt252, public_key: EthPublicKey, signature: Span ) -> bool { let mut signature = signature; - let signature: Signature = Serde::deserialize(ref signature) + let signature: EthSignature = Serde::deserialize(ref signature) .expect('Signature: Invalid format.'); - // Signature out of range - if !is_signature_entry_valid::(signature.r) { - return false; - } - if !is_signature_entry_valid::(signature.s) { - return false; - } - - let public_key_point: Secp256k1Point = recover_public_key(msg_hash.into(), signature).unwrap(); - if public_key_point != public_key { - // Invalid signature - return false; - } - true + secp256_trait::is_valid_signature(msg_hash.into(), signature.r, signature.s, public_key) } diff --git a/src/tests/account/test_dual_eth_account.cairo b/src/tests/account/test_dual_eth_account.cairo index 72bb1bb5c..8f9e2bf3c 100644 --- a/src/tests/account/test_dual_eth_account.cairo +++ b/src/tests/account/test_dual_eth_account.cairo @@ -12,7 +12,6 @@ use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; use openzeppelin::tests::utils::constants::{ETH_PUBKEY, NEW_ETH_PUBKEY}; use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend; -use starknet::eth_signature::Signature; use starknet::testing; // diff --git a/src/tests/account/test_eth_account.cairo b/src/tests/account/test_eth_account.cairo index 309d03259..62035076f 100644 --- a/src/tests/account/test_eth_account.cairo +++ b/src/tests/account/test_eth_account.cairo @@ -9,6 +9,7 @@ use openzeppelin::account::interface::{ISRC6, ISRC6_ID}; use openzeppelin::account::utils::secp256k1::{ DebugSecp256k1Point, Secp256k1PointPartialEq, Secp256k1PointSerde }; +use openzeppelin::account::utils::signature::EthSignature; use openzeppelin::introspection::interface::{ISRC5, ISRC5_ID}; use openzeppelin::tests::mocks::erc20_mocks::DualCaseERC20Mock; use openzeppelin::tests::mocks::eth_account_mocks::DualCaseEthAccountMock; @@ -24,7 +25,6 @@ use poseidon::poseidon_hash_span; use starknet::ContractAddress; use starknet::SyscallResultTrait; use starknet::account::Call; -use starknet::eth_signature::Signature; use starknet::secp256k1::secp256k1_new_syscall; use starknet::testing; @@ -33,7 +33,7 @@ struct SignedTransactionData { private_key: u256, public_key: EthPublicKey, transaction_hash: felt252, - signature: Signature + signature: EthSignature } /// This signature was computed using ethers.js. @@ -47,10 +47,9 @@ fn SIGNED_TX_DATA() -> SignedTransactionData { .unwrap() .unwrap(), transaction_hash: 0x008f882c63d0396d216d57529fe29ad5e70b6cd51b47bd2458b0a4ccb2ba0957, - signature: Signature { + signature: EthSignature { r: 0x82bb3efc0554ec181405468f273b0dbf935cca47182b22da78967d0770f7dcc3, s: 0x6719fef30c11c74add873e4da0e1234deb69eae6a6bd4daa44b816dc199f3e86, - y_parity: true } } }