From 7016cbc33d6b482863bfe1e4734d39a9f2466ff5 Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Sat, 18 May 2024 19:56:20 +0200 Subject: [PATCH] Validate new public key in EthAccount (#990) * feat: add logic and tests * feat: add documentation * feat: update CHANGELOG * Update src/account/eth_account.cairo Co-authored-by: Andrew Fleming * Update docs/modules/ROOT/pages/api/account.adoc Co-authored-by: Andrew Fleming * feat: update test --------- Co-authored-by: Andrew Fleming --- CHANGELOG.md | 1 + docs/modules/ROOT/pages/api/account.adoc | 40 ++++++++- src/account/dual_eth_account.cairo | 11 ++- src/account/eth_account.cairo | 61 ++++++++++++-- src/account/interface.cairo | 8 +- src/presets/interfaces/eth_account.cairo | 4 +- src/tests/account/test_dual_eth_account.cairo | 74 +++++++++++++++-- src/tests/account/test_eth_account.cairo | 83 +++++++++++++++---- src/tests/mocks/eth_account_mocks.cairo | 8 +- src/tests/presets/test_eth_account.cairo | 50 ++++++++--- 10 files changed, 281 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16b145ecc..3f967c394 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ERC721Component implementations now require an ERC721HooksTrait implementation in scope (#978) - ERC1155Component implementations now require an ERC1155HooksTrait implementation in scope (#982) - AccountComponent, preset, and dispatcher now require a `signature` param in the public-key-setter functions (#989) +- EthAccountComponent, preset, and dispatcher now require a `signature` param in the public-key-setter functions (#990) ## 0.12.0 (2024-04-21) diff --git a/docs/modules/ROOT/pages/api/account.adoc b/docs/modules/ROOT/pages/api/account.adoc index 66d0c686d..a3a39e3b6 100644 --- a/docs/modules/ROOT/pages/api/account.adoc +++ b/docs/modules/ROOT/pages/api/account.adoc @@ -369,7 +369,7 @@ NOTE: The `EthPublicKey` type is an alias for `starknet::secp256k1::Secp256k1Poi .PublicKeyImpl * xref:#EthAccountComponent-get_public_key[`++get_public_key(self)++`] -* xref:#EthAccountComponent-set_public_key[`++set_public_key(self, new_public_key)++`] +* xref:#EthAccountComponent-set_public_key[`++set_public_key(self, new_public_key, signature)++`] [.sub-index#EthAccountComponent-Embeddable-Impls-SRC6CamelOnlyImpl] .SRC6CamelOnlyImpl @@ -380,7 +380,7 @@ NOTE: The `EthPublicKey` type is an alias for `starknet::secp256k1::Secp256k1Poi .PublicKeyCamelImpl * xref:#EthAccountComponent-getPublicKey[`++getPublicKey(self)++`] -* xref:#EthAccountComponent-setPublicKey[`++setPublicKey(self, newPublicKey)++`] +* xref:#EthAccountComponent-setPublicKey[`++setPublicKey(self, newPublicKey, signature)++`] .SRC5Impl * xref:api/introspection.adoc#ISRC5-supports_interface[`supports_interface(self, interface_id: felt252)`] @@ -393,6 +393,7 @@ NOTE: The `EthPublicKey` type is an alias for `starknet::secp256k1::Secp256k1Poi * xref:#EthAccountComponent-initializer[`++initializer(self, public_key)++`] * xref:#EthAccountComponent-assert_only_self[`++assert_only_self(self)++`] +* xref:#EthAccountComponent-assert_valid_new_owner[`++assert_valid_new_owner(self, current_owner, new_owner, signature)++`] * xref:#EthAccountComponent-validate_transaction[`++validate_transaction(self)++`] * xref:#EthAccountComponent-_set_public_key[`++_set_public_key(self, new_public_key)++`] * xref:#EthAccountComponent-_is_valid_signature[`++_is_valid_signature(self, hash, signature)++`] @@ -451,12 +452,30 @@ Returns the current public key of the account. [.contract-item] [[EthAccountComponent-set_public_key]] -==== `[.contract-item-name]#++set_public_key++#++(ref self: ContractState, new_public_key: EthPublicKey)++` [.item-kind]#external# +==== `[.contract-item-name]#++set_public_key++#++(ref self: ContractState, new_public_key: EthPublicKey, signature: Span)++` [.item-kind]#external# Sets a new public key for the account. Only accessible by the account calling itself through `\\__execute__`. +Requirements: + +- The caller must be the contract itself. +- The signature must be valid for the new owner. + Emits both an {OwnerRemoved} and an {OwnerAdded} event. +[NOTE] +==== +The message to be signed is computed in Cairo as follows: +```javascript +let message_hash = PoseidonTrait::new() + .update_with('StarkNet Message') + .update_with('accept_ownership') + .update_with(get_contract_address()) + .update_with(current_owner.get_coordinates().unwrap_syscall()) + .finalize(); +``` +==== + [.contract-item] [[EthAccountComponent-isValidSignature]] ==== `[.contract-item-name]#++isValidSignature++#++(self: @ContractState, hash: felt252, signature: Array) → felt252++` [.item-kind]#external# @@ -471,7 +490,7 @@ See xref:EthAccountComponent-get_public_key[get_public_key]. [.contract-item] [[EthAccountComponent-setPublicKey]] -==== `[.contract-item-name]#++setPublicKey++#++(ref self: ContractState, newPublicKey: EthPublicKey)++` [.item-kind]#external# +==== `[.contract-item-name]#++setPublicKey++#++(ref self: ContractState, newPublicKey: EthPublicKey, signature: Span)++` [.item-kind]#external# See xref:EthAccountComponent-set_public_key[set_public_key]. @@ -492,6 +511,19 @@ Emits an {OwnerAdded} event. Validates that the caller is the account itself. Otherwise it reverts. +[.contract-item] +[[EthAccountComponent-assert_valid_new_owner]] +==== `[.contract-item-name]#++assert_valid_new_owner++#++(self: @ComponentState, current_owner: EthPublicKey, new_owner: EthPublicKey, signature: Span)++` [.item-kind]#internal# + +Validates that `new_owner` accepted the ownership of the contract through a signature. + +Requirements: + +- The signature must be valid for the `new_owner`. + +WARNING: This function assumes that `current_owner` is the current owner of the contract, and +does not validate this assumption. + [.contract-item] [[EthAccountComponent-validate_transaction]] ==== `[.contract-item-name]#++validate_transaction++#++(self: @ComponentState)++ → felt252` [.item-kind]#internal# diff --git a/src/account/dual_eth_account.cairo b/src/account/dual_eth_account.cairo index fec1d1cc1..36e8c8e7a 100644 --- a/src/account/dual_eth_account.cairo +++ b/src/account/dual_eth_account.cairo @@ -17,7 +17,9 @@ struct DualCaseEthAccount { } trait DualCaseEthAccountABI { - fn set_public_key(self: @DualCaseEthAccount, new_public_key: EthPublicKey); + fn set_public_key( + self: @DualCaseEthAccount, new_public_key: EthPublicKey, signature: Span + ); fn get_public_key(self: @DualCaseEthAccount) -> EthPublicKey; fn is_valid_signature( self: @DualCaseEthAccount, hash: felt252, signature: Array @@ -26,9 +28,12 @@ trait DualCaseEthAccountABI { } impl DualCaseEthAccountImpl of DualCaseEthAccountABI { - fn set_public_key(self: @DualCaseEthAccount, new_public_key: EthPublicKey) { + fn set_public_key( + self: @DualCaseEthAccount, new_public_key: EthPublicKey, signature: Span + ) { let mut args = array![]; - new_public_key.serialize(ref args); + args.append_serde(new_public_key); + args.append_serde(signature); try_selector_with_fallback( *self.contract_address, selectors::set_public_key, selectors::setPublicKey, args.span() diff --git a/src/account/eth_account.cairo b/src/account/eth_account.cairo index 9146fc311..a2ff222a8 100644 --- a/src/account/eth_account.cairo +++ b/src/account/eth_account.cairo @@ -6,6 +6,7 @@ /// The EthAccount component enables contracts to behave as accounts signing with Ethereum keys. #[starknet::component] mod EthAccountComponent { + use core::hash::{HashStateExTrait, HashStateTrait}; use core::starknet::secp256_trait::Secp256PointTrait; use openzeppelin::account::interface::EthPublicKey; use openzeppelin::account::interface; @@ -15,7 +16,7 @@ mod EthAccountComponent { use openzeppelin::introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait; use openzeppelin::introspection::src5::SRC5Component::SRC5; use openzeppelin::introspection::src5::SRC5Component; - use poseidon::poseidon_hash_span; + use poseidon::{PoseidonTrait, poseidon_hash_span}; use starknet::SyscallResultTrait; use starknet::account::Call; use starknet::get_caller_address; @@ -160,14 +161,21 @@ mod EthAccountComponent { /// Requirements: /// /// - The caller must be the contract itself. + /// - The signature must be valid for the new owner. /// /// Emits an `OwnerRemoved` event. - fn set_public_key(ref self: ComponentState, new_public_key: EthPublicKey) { + fn set_public_key( + ref self: ComponentState, + new_public_key: EthPublicKey, + signature: Span + ) { self.assert_only_self(); let current_public_key: EthPublicKey = self.EthAccount_public_key.read(); let removed_owner_guid = _get_guid_from_public_key(current_public_key); + self.assert_valid_new_owner(current_public_key, new_public_key, signature); + self.emit(OwnerRemoved { removed_owner_guid }); self._set_public_key(new_public_key); } @@ -200,8 +208,12 @@ mod EthAccountComponent { self.EthAccount_public_key.read() } - fn setPublicKey(ref self: ComponentState, newPublicKey: EthPublicKey) { - PublicKey::set_public_key(ref self, newPublicKey); + fn setPublicKey( + ref self: ComponentState, + newPublicKey: EthPublicKey, + signature: Span + ) { + PublicKey::set_public_key(ref self, newPublicKey, signature); } } @@ -227,6 +239,31 @@ mod EthAccountComponent { assert(self == caller, Errors::UNAUTHORIZED); } + /// Validates that `new_owner` accepted the ownership of the contract. + /// + /// WARNING: This function assumes that `current_owner` is the current owner of the contract, and + /// does not validate this assumption. + /// + /// Requirements: + /// + /// - The signature must be valid for the `new_owner`. + fn assert_valid_new_owner( + self: @ComponentState, + current_owner: EthPublicKey, + new_owner: EthPublicKey, + signature: Span + ) { + let message_hash = PoseidonTrait::new() + .update_with('StarkNet Message') + .update_with('accept_ownership') + .update_with(get_contract_address()) + .update_with(current_owner.get_coordinates().unwrap_syscall()) + .finalize(); + + let is_valid = is_valid_eth_signature(message_hash, new_owner, signature); + assert(is_valid, Errors::INVALID_SIGNATURE); + } + /// Validates the signature for the current transaction. /// Returns the short string `VALID` if valid, otherwise it reverts. fn validate_transaction(self: @ComponentState) -> felt252 { @@ -315,8 +352,12 @@ mod EthAccountComponent { PublicKey::get_public_key(self) } - fn set_public_key(ref self: ComponentState, new_public_key: EthPublicKey) { - PublicKey::set_public_key(ref self, new_public_key); + fn set_public_key( + ref self: ComponentState, + new_public_key: EthPublicKey, + signature: Span + ) { + PublicKey::set_public_key(ref self, new_public_key, signature); } // IPublicKeyCamel @@ -324,8 +365,12 @@ mod EthAccountComponent { PublicKeyCamel::getPublicKey(self) } - fn setPublicKey(ref self: ComponentState, newPublicKey: EthPublicKey) { - PublicKeyCamel::setPublicKey(ref self, newPublicKey); + fn setPublicKey( + ref self: ComponentState, + newPublicKey: EthPublicKey, + signature: Span + ) { + PublicKeyCamel::setPublicKey(ref self, newPublicKey, signature); } // ISRC5 diff --git a/src/account/interface.cairo b/src/account/interface.cairo index 7ba187422..0c12a510f 100644 --- a/src/account/interface.cairo +++ b/src/account/interface.cairo @@ -97,14 +97,14 @@ trait IEthDeployable { #[starknet::interface] trait IEthPublicKey { fn get_public_key(self: @TState) -> EthPublicKey; - fn set_public_key(ref self: TState, new_public_key: EthPublicKey); + fn set_public_key(ref self: TState, new_public_key: EthPublicKey, signature: Span); } #[starknet::interface] trait IEthPublicKeyCamel { fn getPublicKey(self: @TState) -> EthPublicKey; - fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey); + fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span); } // @@ -131,12 +131,12 @@ trait EthAccountABI { // IEthPublicKey fn get_public_key(self: @TState) -> EthPublicKey; - fn set_public_key(ref self: TState, new_public_key: EthPublicKey); + fn set_public_key(ref self: TState, new_public_key: EthPublicKey, signature: Span); // ISRC6CamelOnly fn isValidSignature(self: @TState, hash: felt252, signature: Array) -> felt252; // IEthPublicKeyCamel fn getPublicKey(self: @TState) -> EthPublicKey; - fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey); + fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span); } diff --git a/src/presets/interfaces/eth_account.cairo b/src/presets/interfaces/eth_account.cairo index e9f5f95da..27fb9dfef 100644 --- a/src/presets/interfaces/eth_account.cairo +++ b/src/presets/interfaces/eth_account.cairo @@ -23,14 +23,14 @@ trait EthAccountUpgradeableABI { // IEthPublicKey fn get_public_key(self: @TState) -> EthPublicKey; - fn set_public_key(ref self: TState, new_public_key: EthPublicKey); + fn set_public_key(ref self: TState, new_public_key: EthPublicKey, signature: Span); // ISRC6CamelOnly fn isValidSignature(self: @TState, hash: felt252, signature: Array) -> felt252; // IEthPublicKeyCamel fn getPublicKey(self: @TState) -> EthPublicKey; - fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey); + fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span); // IUpgradeable fn upgrade(ref self: TState, new_class_hash: ClassHash); diff --git a/src/tests/account/test_dual_eth_account.cairo b/src/tests/account/test_dual_eth_account.cairo index 8f9e2bf3c..c187edf1f 100644 --- a/src/tests/account/test_dual_eth_account.cairo +++ b/src/tests/account/test_dual_eth_account.cairo @@ -3,13 +3,15 @@ use openzeppelin::account::interface::{EthAccountABIDispatcherTrait, EthAccountA use openzeppelin::account::utils::secp256k1::{ DebugSecp256k1Point, Secp256k1PointPartialEq, Secp256k1PointSerde }; +use openzeppelin::account::utils::signature::EthSignature; use openzeppelin::introspection::interface::ISRC5_ID; +use openzeppelin::tests::account::test_eth_account::NEW_ETH_PUBKEY; use openzeppelin::tests::account::test_eth_account::SIGNED_TX_DATA; use openzeppelin::tests::mocks::eth_account_mocks::{ CamelEthAccountPanicMock, CamelEthAccountMock, SnakeEthAccountMock, SnakeEthAccountPanicMock }; use openzeppelin::tests::mocks::non_implementing_mock::NonImplementingMock; -use openzeppelin::tests::utils::constants::{ETH_PUBKEY, NEW_ETH_PUBKEY}; +use openzeppelin::tests::utils::constants::ETH_PUBKEY; use openzeppelin::tests::utils; use openzeppelin::utils::serde::SerializedAppend; use starknet::testing; @@ -66,7 +68,7 @@ fn test_dual_set_public_key() { testing::set_contract_address(snake_dispatcher.contract_address); let new_public_key = NEW_ETH_PUBKEY(); - snake_dispatcher.set_public_key(new_public_key); + snake_dispatcher.set_public_key(new_public_key, get_accept_ownership_signature_snake()); assert_eq!(target.get_public_key(), new_public_key); } @@ -74,14 +76,14 @@ fn test_dual_set_public_key() { #[should_panic(expected: ('ENTRYPOINT_NOT_FOUND',))] fn test_dual_no_set_public_key() { let dispatcher = setup_non_account(); - dispatcher.set_public_key(NEW_ETH_PUBKEY()); + dispatcher.set_public_key(NEW_ETH_PUBKEY(), array![].span()); } #[test] #[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] fn test_dual_set_public_key_exists_and_panics() { let (dispatcher, _) = setup_account_panic(); - dispatcher.set_public_key(NEW_ETH_PUBKEY()); + dispatcher.set_public_key(NEW_ETH_PUBKEY(), array![].span()); } #[test] @@ -114,7 +116,7 @@ fn test_dual_is_valid_signature() { data.signature.serialize(ref serialized_signature); testing::set_contract_address(snake_dispatcher.contract_address); - target.set_public_key(data.public_key); + target.set_public_key(data.public_key, get_accept_ownership_signature_snake()); let is_valid = snake_dispatcher.is_valid_signature(hash, serialized_signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -171,7 +173,7 @@ fn test_dual_setPublicKey() { testing::set_contract_address(camel_dispatcher.contract_address); - camel_dispatcher.set_public_key(new_public_key); + camel_dispatcher.set_public_key(new_public_key, get_accept_ownership_signature_camel()); assert_eq!(target.getPublicKey(), new_public_key); } @@ -179,7 +181,7 @@ fn test_dual_setPublicKey() { #[should_panic(expected: ("Some error", 'ENTRYPOINT_FAILED',))] fn test_dual_setPublicKey_exists_and_panics() { let (_, dispatcher) = setup_account_panic(); - dispatcher.set_public_key(NEW_ETH_PUBKEY()); + dispatcher.set_public_key(NEW_ETH_PUBKEY(), array![].span()); } #[test] @@ -205,7 +207,7 @@ fn test_dual_isValidSignature() { data.signature.serialize(ref serialized_signature); testing::set_contract_address(camel_dispatcher.contract_address); - target.setPublicKey(data.public_key); + target.setPublicKey(data.public_key, get_accept_ownership_signature_camel()); let is_valid = camel_dispatcher.is_valid_signature(hash, serialized_signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -220,3 +222,59 @@ fn test_dual_isValidSignature_exists_and_panics() { let (_, dispatcher) = setup_account_panic(); dispatcher.is_valid_signature(hash, signature); } + +// +// Helpers +// + +fn get_accept_ownership_signature_snake() -> Span { + let mut output = array![]; + + // 0x0601d3dfc94e01d267d2e879ba8063b4341dc231939110bcb9e1211b9bbbaf19 = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(snake_dispatcher.contract_address) + // .update_with(ETH_PUBKEY().get_coordinates().unwrap_syscall()) + // .finalize(); + + // This signature was computed using ethers js sdk from the following values: + // - private_key: 0x45397ee6ca34cb49060f1c303c6cb7ee2d6123e617601ef3e31ccf7bf5bef1f9 + // - public_key: + // r: 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e + // s: 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 + // - msg_hash: 0x0601d3dfc94e01d267d2e879ba8063b4341dc231939110bcb9e1211b9bbbaf19 + EthSignature { + r: 0x2ee21d761c9dec6bc855f427ea83b9746b84d3ed7f38cf41e65e9c2d846e9f6c, + s: 0x586ceb49429f27352cd8237775b28c57002b27f9f1d5418707ac8b88c4794847, + } + .serialize(ref output); + + output.span() +} + +fn get_accept_ownership_signature_camel() -> Span { + let mut output = array![]; + + // 0x07cb1b9d0e9d4c29d2ac505057496b5f3674218c804b17b625f0880c93540dde = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(camel_dispatcher.contract_address) + // .update_with(ETH_PUBKEY().get_coordinates().unwrap_syscall()) + // .finalize(); + + // This signature was computed using ethers js sdk from the following values: + // - private_key: 0x45397ee6ca34cb49060f1c303c6cb7ee2d6123e617601ef3e31ccf7bf5bef1f9 + // - public_key: + // r: 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e + // s: 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 + // - msg_hash: 0x07cb1b9d0e9d4c29d2ac505057496b5f3674218c804b17b625f0880c93540dde + EthSignature { + r: 0xfa72fe7817eaf98fc104183f7f64956196285f7e09f4eecb47c1dcf352a23e13, + s: 0x4213427c846e9a9be01c91d556f8981f3450918926a5716e865763c9d41bc14b, + } + .serialize(ref output); + + output.span() +} diff --git a/src/tests/account/test_eth_account.cairo b/src/tests/account/test_eth_account.cairo index 62035076f..f7cbe3bee 100644 --- a/src/tests/account/test_eth_account.cairo +++ b/src/tests/account/test_eth_account.cairo @@ -14,7 +14,7 @@ use openzeppelin::introspection::interface::{ISRC5, ISRC5_ID}; use openzeppelin::tests::mocks::erc20_mocks::DualCaseERC20Mock; use openzeppelin::tests::mocks::eth_account_mocks::DualCaseEthAccountMock; use openzeppelin::tests::utils::constants::{ - ETH_PUBKEY, NEW_ETH_PUBKEY, NAME, SYMBOL, SALT, ZERO, OTHER, RECIPIENT, CALLER, QUERY_VERSION, + ETH_PUBKEY, NAME, SYMBOL, SALT, ZERO, OTHER, RECIPIENT, CALLER, QUERY_VERSION, MIN_TRANSACTION_VERSION }; use openzeppelin::tests::utils; @@ -22,11 +22,12 @@ use openzeppelin::token::erc20::interface::{IERC20DispatcherTrait, IERC20Dispatc use openzeppelin::utils::selectors; use openzeppelin::utils::serde::SerializedAppend; use poseidon::poseidon_hash_span; -use starknet::ContractAddress; use starknet::SyscallResultTrait; use starknet::account::Call; +use starknet::secp256k1::secp256k1_get_point_from_x_syscall; use starknet::secp256k1::secp256k1_new_syscall; use starknet::testing; +use starknet::{contract_address_const, ContractAddress}; #[derive(Drop)] struct SignedTransactionData { @@ -40,12 +41,7 @@ struct SignedTransactionData { fn SIGNED_TX_DATA() -> SignedTransactionData { SignedTransactionData { private_key: 0x45397ee6ca34cb49060f1c303c6cb7ee2d6123e617601ef3e31ccf7bf5bef1f9, - public_key: secp256k1_new_syscall( - 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e, - 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 - ) - .unwrap() - .unwrap(), + public_key: NEW_ETH_PUBKEY(), transaction_hash: 0x008f882c63d0396d216d57529fe29ad5e70b6cd51b47bd2458b0a4ccb2ba0957, signature: EthSignature { r: 0x82bb3efc0554ec181405468f273b0dbf935cca47182b22da78967d0770f7dcc3, @@ -58,12 +54,21 @@ fn SIGNED_TX_DATA() -> SignedTransactionData { // Constants // +fn NEW_ETH_PUBKEY() -> EthPublicKey { + secp256k1_new_syscall( + 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e, + 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 + ) + .unwrap() + .unwrap() +} + fn CLASS_HASH() -> felt252 { DualCaseEthAccountMock::TEST_CLASS_HASH } fn ACCOUNT_ADDRESS() -> ContractAddress { - Zeroable::zero() + contract_address_const::<0x111111>() } // @@ -418,12 +423,11 @@ fn test_cannot_get_without_initialize() { #[should_panic(expected: ('Secp256k1Point: Invalid point.',))] fn test_cannot_set_without_initialize() { let mut state = COMPONENT_STATE(); - let new_public_key = NEW_ETH_PUBKEY(); testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(ACCOUNT_ADDRESS()); - state.set_public_key(new_public_key); + state.set_public_key(NEW_ETH_PUBKEY(), array![].span()); } #[test] @@ -443,7 +447,7 @@ fn test_public_key_setter_and_getter() { assert_eq!(current, public_key); // Set key - state.set_public_key(new_public_key); + state.set_public_key(new_public_key, get_accept_ownership_signature()); assert_event_owner_removed(ACCOUNT_ADDRESS(), current); assert_only_event_owner_added(ACCOUNT_ADDRESS(), new_public_key); @@ -459,7 +463,7 @@ fn test_public_key_setter_different_account() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(CALLER()); - state.set_public_key(NEW_ETH_PUBKEY()); + state.set_public_key(NEW_ETH_PUBKEY(), get_accept_ownership_signature()); } // @@ -481,7 +485,7 @@ fn test_public_key_setter_and_getter_camel() { let current = state.getPublicKey(); assert_eq!(current, public_key); - state.setPublicKey(new_public_key); + state.setPublicKey(new_public_key, get_accept_ownership_signature()); assert_event_owner_removed(ACCOUNT_ADDRESS(), public_key); assert_only_event_owner_added(ACCOUNT_ADDRESS(), new_public_key); @@ -497,7 +501,7 @@ fn test_public_key_setter_different_account_camel() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(CALLER()); - state.setPublicKey(NEW_ETH_PUBKEY()); + state.setPublicKey(NEW_ETH_PUBKEY(), get_accept_ownership_signature()); } // @@ -542,6 +546,29 @@ fn test_assert_only_self_false() { state.assert_only_self(); } +#[test] +fn test_assert_valid_new_owner() { + let mut state = setup(); + + testing::set_contract_address(ACCOUNT_ADDRESS()); + state.assert_valid_new_owner(ETH_PUBKEY(), NEW_ETH_PUBKEY(), get_accept_ownership_signature()); +} + +#[test] +#[should_panic(expected: ('EthAccount: invalid signature',))] +fn test_assert_valid_new_owner_invalid_signature() { + let mut state = setup(); + + testing::set_contract_address(ACCOUNT_ADDRESS()); + let mut bad_signature = array![]; + EthSignature { + r: 0xe2c02fbaa03809019ce6501cb5e57fc4a1e96e09dd8becfde8508ceddb53330b, + s: 0x6811f854c0f5793a0086f53e4a23c3773fd8afee401b1c4ef148a1554eede5e1, + } + .serialize(ref bad_signature); + state.assert_valid_new_owner(ETH_PUBKEY(), NEW_ETH_PUBKEY(), bad_signature.span()); +} + #[test] fn test__is_valid_signature() { let mut state = COMPONENT_STATE(); @@ -583,6 +610,32 @@ fn test__set_public_key() { // Helpers // +fn get_accept_ownership_signature() -> Span { + let mut output = array![]; + + // 0x5b23679494e4634c66808d93eeef8301f5fd806b095e5e98b45ee97432a0d8d = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(ACCOUNT_ADDRESS()) + // .update_with(ETH_PUBKEY().get_coordinates().unwrap_syscall()) + // .finalize(); + + // This signature was computed using ethers js sdk from the following values: + // - private_key: 0x45397ee6ca34cb49060f1c303c6cb7ee2d6123e617601ef3e31ccf7bf5bef1f9 + // - public_key: + // r: 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e + // s: 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 + // - msg_hash: 0x5b23679494e4634c66808d93eeef8301f5fd806b095e5e98b45ee97432a0d8d + EthSignature { + r: 0x161de897c0232716792d7b580a577212a6573dbb60c0d0449fa673b95b22d942, + s: 0x7c7b279857889e20fb4c002fd2d1c112c9f30fa4c411f7cb32f55ab0af991a73, + } + .serialize(ref output); + + output.span() +} + fn assert_event_owner_added(contract: ContractAddress, public_key: EthPublicKey) { let event = utils::pop_log::(contract).unwrap(); let new_owner_guid = get_guid_from_public_key(public_key); diff --git a/src/tests/mocks/eth_account_mocks.cairo b/src/tests/mocks/eth_account_mocks.cairo index 91724eb41..93750e06c 100644 --- a/src/tests/mocks/eth_account_mocks.cairo +++ b/src/tests/mocks/eth_account_mocks.cairo @@ -162,7 +162,9 @@ mod SnakeEthAccountPanicMock { #[generate_trait] impl ExternalImpl of ExternalTrait { #[external(v0)] - fn set_public_key(ref self: ContractState, new_public_key: EthPublicKey) { + fn set_public_key( + ref self: ContractState, new_public_key: EthPublicKey, signature: Span + ) { panic!("Some error"); } @@ -202,7 +204,9 @@ mod CamelEthAccountPanicMock { #[generate_trait] impl ExternalImpl of ExternalTrait { #[external(v0)] - fn setPublicKey(ref self: ContractState, newPublicKey: EthPublicKey) { + fn setPublicKey( + ref self: ContractState, newPublicKey: EthPublicKey, signature: Span + ) { panic!("Some error"); } diff --git a/src/tests/presets/test_eth_account.cairo b/src/tests/presets/test_eth_account.cairo index 736af2114..46523fccf 100644 --- a/src/tests/presets/test_eth_account.cairo +++ b/src/tests/presets/test_eth_account.cairo @@ -2,11 +2,13 @@ use openzeppelin::account::interface::ISRC6_ID; use openzeppelin::account::utils::secp256k1::{ DebugSecp256k1Point, Secp256k1PointSerde, Secp256k1PointPartialEq }; +use openzeppelin::account::utils::signature::EthSignature; use openzeppelin::introspection::interface::ISRC5_ID; use openzeppelin::presets::EthAccountUpgradeable; use openzeppelin::presets::interfaces::{ EthAccountUpgradeableABIDispatcher, EthAccountUpgradeableABIDispatcherTrait }; +use openzeppelin::tests::account::test_eth_account::NEW_ETH_PUBKEY; use openzeppelin::tests::account::test_eth_account::{ assert_only_event_owner_added, assert_event_owner_removed }; @@ -17,8 +19,7 @@ use openzeppelin::tests::account::test_secp256k1::get_points; use openzeppelin::tests::mocks::eth_account_mocks::SnakeEthAccountMock; use openzeppelin::tests::upgrades::test_upgradeable::assert_only_event_upgraded; use openzeppelin::tests::utils::constants::{ - CLASS_HASH_ZERO, ETH_PUBKEY, NEW_ETH_PUBKEY, SALT, ZERO, RECIPIENT, QUERY_VERSION, - MIN_TRANSACTION_VERSION + CLASS_HASH_ZERO, ETH_PUBKEY, SALT, ZERO, RECIPIENT, QUERY_VERSION, MIN_TRANSACTION_VERSION }; use openzeppelin::tests::utils; use openzeppelin::token::erc20::interface::IERC20DispatcherTrait; @@ -104,13 +105,13 @@ fn test_constructor() { // #[test] -fn test_public_key_setter_and_getter() { +fn test_public_key_setter_and_getter_2() { let dispatcher = setup_dispatcher(); let new_public_key = NEW_ETH_PUBKEY(); testing::set_contract_address(dispatcher.contract_address); - dispatcher.set_public_key(new_public_key); + dispatcher.set_public_key(new_public_key, get_accept_ownership_signature()); assert_eq!(dispatcher.get_public_key(), new_public_key); assert_event_owner_removed(dispatcher.contract_address, ETH_PUBKEY()); @@ -124,7 +125,7 @@ fn test_public_key_setter_and_getter_camel() { testing::set_contract_address(dispatcher.contract_address); - dispatcher.setPublicKey(new_public_key); + dispatcher.setPublicKey(new_public_key, get_accept_ownership_signature()); assert_eq!(dispatcher.getPublicKey(), new_public_key); assert_event_owner_removed(dispatcher.contract_address, ETH_PUBKEY()); @@ -135,14 +136,14 @@ fn test_public_key_setter_and_getter_camel() { #[should_panic(expected: ('EthAccount: unauthorized', 'ENTRYPOINT_FAILED'))] fn test_set_public_key_different_account() { let dispatcher = setup_dispatcher(); - dispatcher.set_public_key(NEW_ETH_PUBKEY()); + dispatcher.set_public_key(NEW_ETH_PUBKEY(), array![].span()); } #[test] #[should_panic(expected: ('EthAccount: unauthorized', 'ENTRYPOINT_FAILED'))] fn test_setPublicKey_different_account() { let dispatcher = setup_dispatcher(); - dispatcher.setPublicKey(NEW_ETH_PUBKEY()); + dispatcher.setPublicKey(NEW_ETH_PUBKEY(), array![].span()); } // @@ -158,7 +159,7 @@ fn is_valid_sig_dispatcher() -> (EthAccountUpgradeableABIDispatcher, felt252, Ar data.signature.serialize(ref serialized_signature); testing::set_contract_address(dispatcher.contract_address); - dispatcher.set_public_key(data.public_key); + dispatcher.set_public_key(data.public_key, get_accept_ownership_signature()); (dispatcher, hash, serialized_signature) } @@ -430,7 +431,6 @@ fn test_account_called_from_contract() { account.__execute__(calls); } - // // upgrade // @@ -483,12 +483,10 @@ fn test_state_persists_after_upgrade() { set_contract_and_caller(v1.contract_address); let dispatcher = EthAccountUpgradeableABIDispatcher { contract_address: v1.contract_address }; - let (point, _) = get_points(); - - dispatcher.set_public_key(point); + dispatcher.set_public_key(NEW_ETH_PUBKEY(), get_accept_ownership_signature()); let camel_public_key = dispatcher.getPublicKey(); - assert_eq!(camel_public_key, point); + assert_eq!(camel_public_key, NEW_ETH_PUBKEY()); v1.upgrade(v2_class_hash); let snake_public_key = dispatcher.get_public_key(); @@ -504,3 +502,29 @@ fn set_contract_and_caller(address: ContractAddress) { testing::set_contract_address(address); testing::set_caller_address(address); } + +fn get_accept_ownership_signature() -> Span { + let mut output = array![]; + + // 0x0438342f44d2d0cd5f5037fc965ca4765cdfc9d6b039c8ffefd1f94804d3b6ed = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(dispatcher.contract_address) + // .update_with(ETH_PUBKEY().get_coordinates().unwrap_syscall()) + // .finalize(); + + // This signature was computed using ethers js sdk from the following values: + // - private_key: 0x45397ee6ca34cb49060f1c303c6cb7ee2d6123e617601ef3e31ccf7bf5bef1f9 + // - public_key: + // r: 0x829307f82a1883c2414503ba85fc85037f22c6fc6f80910801f6b01a4131da1e + // s: 0x2a23f7bddf3715d11767b1247eccc68c89e11b926e2615268db6ad1af8d8da96 + // - msg_hash: 0x0438342f44d2d0cd5f5037fc965ca4765cdfc9d6b039c8ffefd1f94804d3b6ed + EthSignature { + r: 0x512c2acbb64be5ac67d5d143898d915919cc6d6806a26f0686d5e92e101c6271, + s: 0x420deca8404b7680530a4c9178a7fcd2c3e5a2f98fa4f8ada84ef90fea0d98ca, + } + .serialize(ref output); + + output.span() +}