Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove y from EthAccount signature. #940

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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

Expand Down
28 changes: 10 additions & 18 deletions src/account/utils/signature.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,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::secp256_trait;
use starknet::secp256k1::Secp256k1Point;
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved

#[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
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
/// for efficiency reasons. It is not recommended to use it other than for
/// validating account signatures over transaction hashes since otherwise
Expand All @@ -25,7 +30,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.
Expand All @@ -34,21 +39,8 @@ fn is_valid_eth_signature(
msg_hash: felt252, public_key: EthPublicKey, signature: Span<felt252>
) -> 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::<Secp256k1Point>(signature.r) {
return false;
}
if !is_signature_entry_valid::<Secp256k1Point>(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)
}
1 change: 0 additions & 1 deletion src/tests/account/test_dual_eth_account.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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;

//
Expand Down
7 changes: 3 additions & 4 deletions src/tests/account/test_eth_account.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -24,7 +25,6 @@ use poseidon::poseidon_hash_span;
use starknet::ContractAddress;
use starknet::SyscallResultTrait;
use starknet::account::Call;
use starknet::eth_signature::Signature;
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
use starknet::secp256k1::secp256k1_new_syscall;
use starknet::testing;

Expand All @@ -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.
Expand All @@ -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
}
}
}
Expand Down
Loading