From 852abcf954dd62efd419e733019ce8e2a89f908b Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Fri, 17 May 2024 00:34:35 +0200 Subject: [PATCH] Validate new public key in Account (#989) * feat: add logic and tests * feat: update CHANGELOG * feat: use ExTrait * feat: update doc example * Update src/tests/account/test_account.cairo Co-authored-by: Andrew Fleming * Update src/tests/account/test_account.cairo Co-authored-by: Andrew Fleming * Update docs/modules/ROOT/pages/api/account.adoc Co-authored-by: Andrew Fleming * Update docs/modules/ROOT/pages/api/account.adoc Co-authored-by: Andrew Fleming * Update docs/modules/ROOT/pages/api/account.adoc Co-authored-by: Andrew Fleming * Update src/account/account.cairo Co-authored-by: Andrew Fleming * Update src/account/account.cairo Co-authored-by: Andrew Fleming * feat: move NEW_PUBKEY to constants * feat: format files --------- Co-authored-by: Andrew Fleming --- CHANGELOG.md | 5 +- docs/modules/ROOT/pages/api/account.adoc | 41 ++++++++++-- src/account/account.cairo | 66 +++++++++++++++++--- src/account/dual_account.cairo | 7 ++- src/account/interface.cairo | 8 +-- src/presets/interfaces/account.cairo | 4 +- src/tests/account/test_account.cairo | 76 ++++++++++++++++++----- src/tests/account/test_dual_account.cairo | 61 +++++++++++++++--- src/tests/mocks/account_mocks.cairo | 6 +- src/tests/presets/test_account.cairo | 34 +++++++--- src/tests/utils/constants.cairo | 2 +- 11 files changed, 251 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d402f671f..16b145ecc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed (Breaking) - ERC721Component internal implementation to support transfer, mint, and burn flows going through an `_update` function (#978) -- ERC721Component implementations now require an ERC721HooksTrait implementation in scope. -- ERC1155Component implementations now require an ERC1155HooksTrait implementation in scope. +- 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) ## 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 75f33e700..66d0c686d 100644 --- a/docs/modules/ROOT/pages/api/account.adoc +++ b/docs/modules/ROOT/pages/api/account.adoc @@ -117,7 +117,7 @@ NOTE: {src5-component-required-note} .PublicKeyImpl * xref:#AccountComponent-get_public_key[`++get_public_key(self)++`] -* xref:#AccountComponent-set_public_key[`++set_public_key(self, new_public_key)++`] +* xref:#AccountComponent-set_public_key[`++set_public_key(self, new_public_key, signature)++`] [.sub-index#AccountComponent-Embeddable-Impls-SRC6CamelOnlyImpl] .SRC6CamelOnlyImpl @@ -128,7 +128,7 @@ NOTE: {src5-component-required-note} .PublicKeyCamelImpl * xref:#AccountComponent-getPublicKey[`++getPublicKey(self)++`] -* xref:#AccountComponent-setPublicKey[`++setPublicKey(self, newPublicKey)++`] +* xref:#AccountComponent-setPublicKey[`++setPublicKey(self, newPublicKey, signature)++`] .SRC5Impl * xref:api/introspection.adoc#ISRC5-supports_interface[`supports_interface(self, interface_id: felt252)`] @@ -141,6 +141,7 @@ NOTE: {src5-component-required-note} * xref:#AccountComponent-initializer[`++initializer(self, public_key)++`] * xref:#AccountComponent-assert_only_self[`++assert_only_self(self)++`] +* xref:#AccountComponent-assert_valid_new_owner[`++assert_valid_new_owner(self, current_owner, new_owner, signature)++`] * xref:#AccountComponent-validate_transaction[`++validate_transaction(self)++`] * xref:#AccountComponent-_set_public_key[`++_set_public_key(self, new_public_key)++`] * xref:#AccountComponent-_is_valid_signature[`++_is_valid_signature(self, hash, signature)++`] @@ -199,12 +200,31 @@ Returns the current public key of the account. [.contract-item] [[AccountComponent-set_public_key]] -==== `[.contract-item-name]#++set_public_key++#++(ref self: ContractState, new_public_key: felt252)++` [.item-kind]#external# +==== `[.contract-item-name]#++set_public_key++#++(ref self: ContractState, new_public_key: felt252, 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) + .finalize(); +``` +==== + [.contract-item] [[AccountComponent-isValidSignature]] ==== `[.contract-item-name]#++isValidSignature++#++(self: @ContractState, hash: felt252, signature: Array) → felt252++` [.item-kind]#external# @@ -219,7 +239,7 @@ See xref:AccountComponent-get_public_key[get_public_key]. [.contract-item] [[AccountComponent-setPublicKey]] -==== `[.contract-item-name]#++setPublicKey++#++(ref self: ContractState, newPublicKey: felt252)++` [.item-kind]#external# +==== `[.contract-item-name]#++setPublicKey++#++(ref self: ContractState, newPublicKey: felt252, signature: Span)++` [.item-kind]#external# See xref:AccountComponent-set_public_key[set_public_key]. @@ -240,6 +260,19 @@ Emits an {OwnerAdded} event. Validates that the caller is the account itself. Otherwise it reverts. +[.contract-item] +[[AccountComponent-assert_valid_new_owner]] +==== `[.contract-item-name]#++assert_valid_new_owner++#++(self: @ComponentState, current_owner: felt252, new_owner: felt252, signature: Span)++` [.item-kind]#internal# + +Validates that `new_owner` accepted the ownership of the contract through a signature. + +Requirements: + +- `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] [[AccountComponent-validate_transaction]] ==== `[.contract-item-name]#++validate_transaction++#++(self: @ComponentState)++ → felt252` [.item-kind]#internal# diff --git a/src/account/account.cairo b/src/account/account.cairo index b962a1055..26a6c6bab 100644 --- a/src/account/account.cairo +++ b/src/account/account.cairo @@ -6,12 +6,14 @@ /// The Account component enables contracts to behave as accounts. #[starknet::component] mod AccountComponent { + use core::hash::{HashStateExTrait, HashStateTrait}; use openzeppelin::account::interface; use openzeppelin::account::utils::{MIN_TRANSACTION_VERSION, QUERY_VERSION, QUERY_OFFSET}; use openzeppelin::account::utils::{execute_calls, is_valid_stark_signature}; use openzeppelin::introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait; use openzeppelin::introspection::src5::SRC5Component::SRC5; use openzeppelin::introspection::src5::SRC5Component; + use poseidon::PoseidonTrait; use starknet::account::Call; use starknet::get_caller_address; use starknet::get_contract_address; @@ -155,11 +157,20 @@ mod AccountComponent { /// 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: felt252) { + /// Emits both an `OwnerRemoved` and an `OwnerAdded` event. + fn set_public_key( + ref self: ComponentState, + new_public_key: felt252, + signature: Span + ) { self.assert_only_self(); - self.emit(OwnerRemoved { removed_owner_guid: self.Account_public_key.read() }); + + let current_owner = self.Account_public_key.read(); + self.assert_valid_new_owner(current_owner, new_public_key, signature); + + self.emit(OwnerRemoved { removed_owner_guid: current_owner }); self._set_public_key(new_public_key); } } @@ -191,8 +202,12 @@ mod AccountComponent { self.Account_public_key.read() } - fn setPublicKey(ref self: ComponentState, newPublicKey: felt252) { - PublicKey::set_public_key(ref self, newPublicKey); + fn setPublicKey( + ref self: ComponentState, + newPublicKey: felt252, + signature: Span + ) { + PublicKey::set_public_key(ref self, newPublicKey, signature); } } @@ -218,6 +233,31 @@ mod AccountComponent { 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: felt252, + new_owner: felt252, + signature: Span + ) { + let message_hash = PoseidonTrait::new() + .update_with('StarkNet Message') + .update_with('accept_ownership') + .update_with(get_contract_address()) + .update_with(current_owner) + .finalize(); + + let is_valid = is_valid_stark_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 { @@ -300,8 +340,12 @@ mod AccountComponent { PublicKey::get_public_key(self) } - fn set_public_key(ref self: ComponentState, new_public_key: felt252) { - PublicKey::set_public_key(ref self, new_public_key); + fn set_public_key( + ref self: ComponentState, + new_public_key: felt252, + signature: Span + ) { + PublicKey::set_public_key(ref self, new_public_key, signature); } // IPublicKeyCamel @@ -309,8 +353,12 @@ mod AccountComponent { PublicKeyCamel::getPublicKey(self) } - fn setPublicKey(ref self: ComponentState, newPublicKey: felt252) { - PublicKeyCamel::setPublicKey(ref self, newPublicKey); + fn setPublicKey( + ref self: ComponentState, + newPublicKey: felt252, + signature: Span + ) { + PublicKeyCamel::setPublicKey(ref self, newPublicKey, signature); } // ISRC5 diff --git a/src/account/dual_account.cairo b/src/account/dual_account.cairo index 024d1582c..da691e6ce 100644 --- a/src/account/dual_account.cairo +++ b/src/account/dual_account.cairo @@ -15,7 +15,7 @@ struct DualCaseAccount { } trait DualCaseAccountABI { - fn set_public_key(self: @DualCaseAccount, new_public_key: felt252); + fn set_public_key(self: @DualCaseAccount, new_public_key: felt252, signature: Span); fn get_public_key(self: @DualCaseAccount) -> felt252; fn is_valid_signature( self: @DualCaseAccount, hash: felt252, signature: Array @@ -24,8 +24,9 @@ trait DualCaseAccountABI { } impl DualCaseAccountImpl of DualCaseAccountABI { - fn set_public_key(self: @DualCaseAccount, new_public_key: felt252) { - let args = array![new_public_key]; + fn set_public_key(self: @DualCaseAccount, new_public_key: felt252, signature: Span) { + let mut args = array![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/interface.cairo b/src/account/interface.cairo index aa46dc163..7ba187422 100644 --- a/src/account/interface.cairo +++ b/src/account/interface.cairo @@ -35,7 +35,7 @@ trait IDeployable { #[starknet::interface] trait IPublicKey { fn get_public_key(self: @TState) -> felt252; - fn set_public_key(ref self: TState, new_public_key: felt252); + fn set_public_key(ref self: TState, new_public_key: felt252, signature: Span); } #[starknet::interface] @@ -46,7 +46,7 @@ trait ISRC6CamelOnly { #[starknet::interface] trait IPublicKeyCamel { fn getPublicKey(self: @TState) -> felt252; - fn setPublicKey(ref self: TState, newPublicKey: felt252); + fn setPublicKey(ref self: TState, newPublicKey: felt252, signature: Span); } // @@ -73,14 +73,14 @@ trait AccountABI { // IPublicKey fn get_public_key(self: @TState) -> felt252; - fn set_public_key(ref self: TState, new_public_key: felt252); + fn set_public_key(ref self: TState, new_public_key: felt252, signature: Span); // ISRC6CamelOnly fn isValidSignature(self: @TState, hash: felt252, signature: Array) -> felt252; // IPublicKeyCamel fn getPublicKey(self: @TState) -> felt252; - fn setPublicKey(ref self: TState, newPublicKey: felt252); + fn setPublicKey(ref self: TState, newPublicKey: felt252, signature: Span); } // diff --git a/src/presets/interfaces/account.cairo b/src/presets/interfaces/account.cairo index 3bda0eb19..7cada575a 100644 --- a/src/presets/interfaces/account.cairo +++ b/src/presets/interfaces/account.cairo @@ -21,14 +21,14 @@ trait AccountUpgradeableABI { // IPublicKey fn get_public_key(self: @TState) -> felt252; - fn set_public_key(ref self: TState, new_public_key: felt252); + fn set_public_key(ref self: TState, new_public_key: felt252, signature: Span); // ISRC6CamelOnly fn isValidSignature(self: @TState, hash: felt252, signature: Array) -> felt252; // IPublicKeyCamel fn getPublicKey(self: @TState) -> felt252; - fn setPublicKey(ref self: TState, newPublicKey: felt252); + fn setPublicKey(ref self: TState, newPublicKey: felt252, signature: Span); // IUpgradeable fn upgrade(ref self: TState, new_class_hash: ClassHash); diff --git a/src/tests/account/test_account.cairo b/src/tests/account/test_account.cairo index ec149353c..e9dcac9a6 100644 --- a/src/tests/account/test_account.cairo +++ b/src/tests/account/test_account.cairo @@ -32,10 +32,10 @@ struct SignedTransactionData { fn SIGNED_TX_DATA() -> SignedTransactionData { SignedTransactionData { private_key: 1234, - public_key: 0x1f3c942d7f492a37608cde0d77b884a5aa9e11d2919225968557370ddb5a5aa, + public_key: NEW_PUBKEY, transaction_hash: 0x601d3d2e265c10ff645e1554c435e72ce6721f0ba5fc96f0c650bfc6231191a, - r: 0x6c8be1fb0fb5c730fbd7abaecbed9d980376ff2e660dfcd157e158d2b026891, - s: 0x76b4669998eb933f44a59eace12b41328ab975ceafddf92602b21eb23e22e35 + r: 0x6bc22689efcaeacb9459577138aff9f0af5b77ee7894cdc8efabaf760f6cf6e, + s: 0x295989881583b9325436851934334faa9d639a2094cd1e2f8691c8a71cd4cdf } } @@ -114,7 +114,7 @@ fn test_is_valid_signature() { let mut good_signature = array![data.r, data.s]; let mut bad_signature = array![0x987, 0x564]; - state.set_public_key(data.public_key); + state._set_public_key(data.public_key); let is_valid = state.is_valid_signature(hash, good_signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -132,7 +132,7 @@ fn test_isValidSignature() { let mut good_signature = array![data.r, data.s]; let mut bad_signature = array![0x987, 0x564]; - state.set_public_key(data.public_key); + state._set_public_key(data.public_key); let is_valid = state.isValidSignature(hash, good_signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -398,14 +398,15 @@ fn test_public_key_setter_and_getter() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(ACCOUNT_ADDRESS()); - // Check default + state._set_public_key(PUBKEY); + utils::drop_event(ACCOUNT_ADDRESS()); let public_key = state.get_public_key(); - assert!(public_key.is_zero()); + assert_eq!(public_key, PUBKEY); // Set key - state.set_public_key(NEW_PUBKEY); + state.set_public_key(NEW_PUBKEY, get_accept_ownership_signature()); - assert_event_owner_removed(ACCOUNT_ADDRESS(), 0); + assert_event_owner_removed(ACCOUNT_ADDRESS(), PUBKEY); assert_only_event_owner_added(ACCOUNT_ADDRESS(), NEW_PUBKEY); let public_key = state.get_public_key(); @@ -420,7 +421,7 @@ fn test_public_key_setter_different_account() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(caller); - state.set_public_key(NEW_PUBKEY); + state.set_public_key(NEW_PUBKEY, array![].span()); } // @@ -433,14 +434,15 @@ fn test_public_key_setter_and_getter_camel() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(ACCOUNT_ADDRESS()); - // Check default + state._set_public_key(PUBKEY); + utils::drop_event(ACCOUNT_ADDRESS()); let public_key = state.getPublicKey(); - assert!(public_key.is_zero()); + assert_eq!(public_key, PUBKEY); // Set key - state.setPublicKey(NEW_PUBKEY); + state.setPublicKey(NEW_PUBKEY, get_accept_ownership_signature()); - assert_event_owner_removed(ACCOUNT_ADDRESS(), 0); + assert_event_owner_removed(ACCOUNT_ADDRESS(), PUBKEY); assert_only_event_owner_added(ACCOUNT_ADDRESS(), NEW_PUBKEY); let public_key = state.getPublicKey(); @@ -455,7 +457,7 @@ fn test_public_key_setter_different_account_camel() { testing::set_contract_address(ACCOUNT_ADDRESS()); testing::set_caller_address(caller); - state.setPublicKey(NEW_PUBKEY); + state.setPublicKey(NEW_PUBKEY, array![].span()); } // @@ -500,6 +502,28 @@ fn test_assert_only_self_false() { state.assert_only_self(); } +#[test] +fn test_assert_valid_new_owner() { + let state = setup(); + + testing::set_contract_address(ACCOUNT_ADDRESS()); + state.assert_valid_new_owner(PUBKEY, NEW_PUBKEY, get_accept_ownership_signature()); +} + + +#[test] +#[should_panic(expected: ('Account: invalid signature',))] +fn test_assert_valid_new_owner_invalid_signature() { + let state = setup(); + + testing::set_contract_address(ACCOUNT_ADDRESS()); + let bad_signature = array![ + 0x2ce8fbcf8a793ee5b2a57254dc96863b696698943e3bc7845285f3851336318, + 0x6009f5720649ff1ceb0aba44f85bec3572c81aecb9d2dada7c0cc70b791debe + ]; + state.assert_valid_new_owner(PUBKEY, NEW_PUBKEY, bad_signature.span()); +} + #[test] fn test__is_valid_signature() { let mut state = COMPONENT_STATE(); @@ -510,7 +534,7 @@ fn test__is_valid_signature() { let mut bad_signature = array![0x987, 0x564]; let mut invalid_length_signature = array![0x987]; - state.set_public_key(data.public_key); + state._set_public_key(data.public_key); let is_valid = state._is_valid_signature(hash, good_signature.span()); assert!(is_valid); @@ -537,6 +561,26 @@ fn test__set_public_key() { // Helpers // +fn get_accept_ownership_signature() -> Span { + // 0x7b3d2ce38c132a36e692f6e809b518276d091513af3baf0e94ce2abceee3632 = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(ACCOUNT_ADDRESS()) + // .update_with(PUBKEY) + // .finalize(); + + // This signature was computed using starknet js sdk from the following values: + // - private_key: '1234' + // - public_key: 0x26da8d11938b76025862be14fdb8b28438827f73e75e86f7bfa38b196951fa7 + // - msg_hash: 0x7b3d2ce38c132a36e692f6e809b518276d091513af3baf0e94ce2abceee3632 + array![ + 0x1ce8fbcf8a793ee5b2a57254dc96863b696698943e3bc7845285f3851336318, + 0x6009f5720649ff1ceb0aba44f85bec3572c81aecb9d2dada7c0cc70b791debe + ] + .span() +} + fn assert_event_owner_removed(contract: ContractAddress, removed_owner_guid: felt252) { let event = utils::pop_log::(contract).unwrap(); let expected = AccountComponent::Event::OwnerRemoved(OwnerRemoved { removed_owner_guid }); diff --git a/src/tests/account/test_dual_account.cairo b/src/tests/account/test_dual_account.cairo index a86e407aa..a8706120d 100644 --- a/src/tests/account/test_dual_account.cairo +++ b/src/tests/account/test_dual_account.cairo @@ -57,7 +57,7 @@ fn test_dual_set_public_key() { testing::set_contract_address(snake_dispatcher.contract_address); - snake_dispatcher.set_public_key(NEW_PUBKEY); + snake_dispatcher.set_public_key(NEW_PUBKEY, get_accept_ownership_signature_snake()); let public_key = target.get_public_key(); assert_eq!(public_key, NEW_PUBKEY); @@ -67,14 +67,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_PUBKEY); + dispatcher.set_public_key(NEW_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_PUBKEY); + dispatcher.set_public_key(NEW_PUBKEY, array![].span()); } #[test] @@ -107,7 +107,7 @@ fn test_dual_is_valid_signature() { let mut signature = array![data.r, data.s]; 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, signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -163,8 +163,7 @@ fn test_dual_setPublicKey() { let (camel_dispatcher, target) = setup_camel(); testing::set_contract_address(camel_dispatcher.contract_address); - - camel_dispatcher.set_public_key(NEW_PUBKEY); + camel_dispatcher.set_public_key(NEW_PUBKEY, get_accept_ownership_signature_camel()); let public_key = target.getPublicKey(); assert_eq!(public_key, NEW_PUBKEY); @@ -174,7 +173,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_PUBKEY); + dispatcher.set_public_key(NEW_PUBKEY, array![].span()); } #[test] @@ -197,10 +196,10 @@ fn test_dual_isValidSignature() { let data = SIGNED_TX_DATA(); let hash = data.transaction_hash; - let mut signature = array![data.r, data.s]; + let signature = array![data.r, data.s]; 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, signature); assert_eq!(is_valid, starknet::VALIDATED); @@ -215,3 +214,47 @@ 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 { + // 0x68d0d8341890d736df4ad1f1d73fc0ea21240f5a7f0877376c11cbd5aedaa87 = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(snake_dispatcher.contract_address) + // .update_with(PUBKEY) + // .finalize(); + + // This signature was computed using starknet js sdk from the following values: + // - private_key: '1234' + // - public_key: 0x26da8d11938b76025862be14fdb8b28438827f73e75e86f7bfa38b196951fa7 + // - msg_hash: 0x68d0d8341890d736df4ad1f1d73fc0ea21240f5a7f0877376c11cbd5aedaa87 + array![ + 0x3735f9488006188a1bcb44954c6a42ec9772407b5d74fb8ef289f4dc7e19546, + 0x729228e2d61aa713ccb5eaa5d1d542f5020a8dad9720e70733667189bf73c6d + ] + .span() +} + +fn get_accept_ownership_signature_camel() -> Span { + // 0x7574ff949c0537b235ec5ab787f1c1989ebd875a8e083a1038ee21eb2b44a43 = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(camel_dispatcher.contract_address) + // .update_with(PUBKEY) + // .finalize(); + + // This signature was computed using starknet js sdk from the following values: + // - private_key: '1234' + // - public_key: 0x26da8d11938b76025862be14fdb8b28438827f73e75e86f7bfa38b196951fa7 + // - msg_hash: 0x7574ff949c0537b235ec5ab787f1c1989ebd875a8e083a1038ee21eb2b44a43 + array![ + 0x5a776880cf7154726f9d4afaeee9055ba4ab1a4e9d7f19b6cda4529ed7dc78e, + 0x1e1f24c532a23606f2b996b0b5e38fe14dc2ea9028f1849752fdef534a536d5 + ] + .span() +} diff --git a/src/tests/mocks/account_mocks.cairo b/src/tests/mocks/account_mocks.cairo index 8a3dae2af..321e43f70 100644 --- a/src/tests/mocks/account_mocks.cairo +++ b/src/tests/mocks/account_mocks.cairo @@ -159,7 +159,9 @@ mod SnakeAccountPanicMock { #[generate_trait] impl ExternalImpl of ExternalTrait { #[external(v0)] - fn set_public_key(ref self: ContractState, new_public_key: felt252) { + fn set_public_key( + ref self: ContractState, new_public_key: felt252, signature: Span + ) { panic!("Some error"); } @@ -194,7 +196,7 @@ mod CamelAccountPanicMock { #[generate_trait] impl ExternalImpl of ExternalTrait { #[external(v0)] - fn setPublicKey(ref self: ContractState, newPublicKey: felt252) { + fn setPublicKey(ref self: ContractState, newPublicKey: felt252, signature: Span) { panic!("Some error"); } diff --git a/src/tests/presets/test_account.cairo b/src/tests/presets/test_account.cairo index 596e8bc70..c137a4a6d 100644 --- a/src/tests/presets/test_account.cairo +++ b/src/tests/presets/test_account.cairo @@ -95,7 +95,7 @@ fn test_public_key_setter_and_getter() { testing::set_contract_address(dispatcher.contract_address); - dispatcher.set_public_key(NEW_PUBKEY); + dispatcher.set_public_key(NEW_PUBKEY, get_accept_ownership_signature()); let public_key = dispatcher.get_public_key(); assert_eq!(public_key, NEW_PUBKEY); @@ -109,7 +109,7 @@ fn test_public_key_setter_and_getter_camel() { testing::set_contract_address(dispatcher.contract_address); - dispatcher.setPublicKey(NEW_PUBKEY); + dispatcher.setPublicKey(NEW_PUBKEY, get_accept_ownership_signature()); let public_key = dispatcher.getPublicKey(); assert_eq!(public_key, NEW_PUBKEY); @@ -121,14 +121,14 @@ fn test_public_key_setter_and_getter_camel() { #[should_panic(expected: ('Account: unauthorized', 'ENTRYPOINT_FAILED'))] fn test_set_public_key_different_account() { let dispatcher = setup_dispatcher(); - dispatcher.set_public_key(NEW_PUBKEY); + dispatcher.set_public_key(NEW_PUBKEY, get_accept_ownership_signature()); } #[test] #[should_panic(expected: ('Account: unauthorized', 'ENTRYPOINT_FAILED'))] fn test_setPublicKey_different_account() { let dispatcher = setup_dispatcher(); - dispatcher.setPublicKey(NEW_PUBKEY); + dispatcher.setPublicKey(NEW_PUBKEY, get_accept_ownership_signature()); } // @@ -143,7 +143,7 @@ fn is_valid_sig_dispatcher() -> (AccountUpgradeableABIDispatcher, felt252, Array let mut signature = array![data.r, data.s]; 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, signature) } @@ -496,10 +496,10 @@ fn test_state_persists_after_upgrade() { set_contract_and_caller(v1.contract_address); let dispatcher = AccountUpgradeableABIDispatcher { contract_address: v1.contract_address }; - dispatcher.set_public_key(PUBKEY); + dispatcher.set_public_key(NEW_PUBKEY, get_accept_ownership_signature()); let camel_public_key = dispatcher.getPublicKey(); - assert_eq!(camel_public_key, PUBKEY); + assert_eq!(camel_public_key, NEW_PUBKEY); v1.upgrade(v2_class_hash); let snake_public_key = dispatcher.get_public_key(); @@ -515,3 +515,23 @@ fn set_contract_and_caller(address: ContractAddress) { testing::set_contract_address(address); testing::set_caller_address(address); } + +fn get_accept_ownership_signature() -> Span { + // 0x1d0f29f91d4d8242ae5646be871a7e64717eac611aed9ec15b423cb965817fb = + // PoseidonTrait::new() + // .update_with('StarkNet Message') + // .update_with('accept_ownership') + // .update_with(dispatcher.contract_address) + // .update_with(PUBKEY) + // .finalize(); + + // This signature was computed using starknet js sdk from the following values: + // - private_key: '1234' + // - public_key: 0x26da8d11938b76025862be14fdb8b28438827f73e75e86f7bfa38b196951fa7 + // - msg_hash: 0x1d0f29f91d4d8242ae5646be871a7e64717eac611aed9ec15b423cb965817fb + array![ + 0x5fcf4473fa8304093722b4999e53042db1a16ac4e51669203fe32c241b8ac4c, + 0x2350a661e77f26c304d9a896271977522410d015437dfea190148f224c6c30f + ] + .span() +} diff --git a/src/tests/utils/constants.cairo b/src/tests/utils/constants.cairo index 023d5acf1..cbb92ed19 100644 --- a/src/tests/utils/constants.cairo +++ b/src/tests/utils/constants.cairo @@ -16,7 +16,7 @@ const TOKEN_ID_2: u256 = 121; const TOKEN_VALUE: u256 = 42; const TOKEN_VALUE_2: u256 = 142; const PUBKEY: felt252 = 'PUBKEY'; -const NEW_PUBKEY: felt252 = 'NEW_PUBKEY'; +const NEW_PUBKEY: felt252 = 0x26da8d11938b76025862be14fdb8b28438827f73e75e86f7bfa38b196951fa7; const DAPP_NAME: felt252 = 'DAPP_NAME'; const DAPP_VERSION: felt252 = 'DAPP_VERSION'; const SALT: felt252 = 'SALT';