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.

Sign up for GitHub

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 7 commits into from
Apr 18, 2024
Merged

Remove y from EthAccount signature. #940

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecc4083
feat: remove y from signature
ericnordelo Mar 7, 2024
59bd5ab
feat: update CHANGELOG
ericnordelo Mar 7, 2024
1fc7758
feat: apply update review
ericnordelo Mar 8, 2024
98932ca
refactor: Signature to EthSignature
ericnordelo Mar 11, 2024
1daa5c1
fix: tests
ericnordelo Mar 11, 2024
a71a5ff
Merge branch 'main' of github.com:OpenZeppelin/cairo-contracts into f…
ericnordelo Mar 19, 2024
9cc2634
feat: apply update reviews
ericnordelo Mar 19, 2024
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
View file
Open in desktop
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
26 changes: 9 additions & 17 deletions src/account/utils/signature.cairo
View file
Open in desktop
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 Signature {
pub r: u256,
pub s: u256,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the new struct necessary? We can just keep the corelib eth_signature and not use the y parity or do you think this adds something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Struct is used for Serde (we can't ignore y in the other one for deserialization).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm I see. In that case, the Signature name appears a little vague with both stark and eth signature validation fns in the same module. What do you think about changing the struct name to EthSignature and documenting the reason? IMO we should at least document it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to change to EthSignature, but I feel that's enough, not sure we should document the reason. The only difference between the signatures is the types (felt252 vs u256). Do you have something specific in mind for the documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant documenting the difference of the OZ signature::EthSignature and starknet::eth_signature::Signature just to make it clear why we're defining a new eth sig struct as opposed to using corelib's. I was thinking that above the struct we could comment something like: "Omits the y parity because it's not required for validation." If a user wonders which eth sig type to use, this might be helpful. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that comment adds much, since people don't need to be aware of corelib's Signature, or why is it using y. We just defined a struct representing an EthSignature, without the parity because for this use case is not required.


/// 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 @@ -37,18 +42,5 @@ fn is_valid_eth_signature(
let signature: Signature = 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)
}
3 changes: 1 addition & 2 deletions src/tests/account/test_eth_account.cairo
View file
Open in desktop
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::Signature;
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 @@ -50,7 +50,6 @@ fn SIGNED_TX_DATA() -> SignedTransactionData {
signature: Signature {
r: 0x82bb3efc0554ec181405468f273b0dbf935cca47182b22da78967d0770f7dcc3,
s: 0x6719fef30c11c74add873e4da0e1234deb69eae6a6bd4daa44b816dc199f3e86,
y_parity: true
}
}
}
Expand Down
Loading