Validate new public key in EthAccount (OpenZeppelin#990)

* feat: add logic and tests * feat: add documentation * feat: update CHANGELOG * Update src/account/eth_account.cairo Co-authored-by: Andrew Fleming <[email protected]> * Update docs/modules/ROOT/pages/api/account.adoc Co-authored-by: Andrew Fleming <[email protected]> * feat: update test --------- Co-authored-by: Andrew Fleming <[email protected]>
andrew-fleming · May 18, 2024 · 7016cbc · 7016cbc
1 parent 852abcf
commit 7016cbc
Show file tree

Hide file tree

Showing 10 changed files with 281 additions and 59 deletions.
diff --git 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
@@ -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<felt252>)++` [.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>) → 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<felt252>)++` [.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<felt252>)++` [.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
@@ -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<felt252>
+    );
     fn get_public_key(self: @DualCaseEthAccount) -> EthPublicKey;
     fn is_valid_signature(
         self: @DualCaseEthAccount, hash: felt252, signature: Array<felt252>
@@ -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<felt252>
+    ) {
         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
@@ -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<TContractState>, new_public_key: EthPublicKey) {
+        fn set_public_key(
+            ref self: ComponentState<TContractState>,
+            new_public_key: EthPublicKey,
+            signature: Span<felt252>
+        ) {
             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<TContractState>, newPublicKey: EthPublicKey) {
-            PublicKey::set_public_key(ref self, newPublicKey);
+        fn setPublicKey(
+            ref self: ComponentState<TContractState>,
+            newPublicKey: EthPublicKey,
+            signature: Span<felt252>
+        ) {
+            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<TContractState>,
+            current_owner: EthPublicKey,
+            new_owner: EthPublicKey,
+            signature: Span<felt252>
+        ) {
+            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<TContractState>) -> felt252 {
@@ -315,17 +352,25 @@ mod EthAccountComponent {
             PublicKey::get_public_key(self)
         }
 
-        fn set_public_key(ref self: ComponentState<TContractState>, new_public_key: EthPublicKey) {
-            PublicKey::set_public_key(ref self, new_public_key);
+        fn set_public_key(
+            ref self: ComponentState<TContractState>,
+            new_public_key: EthPublicKey,
+            signature: Span<felt252>
+        ) {
+            PublicKey::set_public_key(ref self, new_public_key, signature);
         }
 
         // IPublicKeyCamel
         fn getPublicKey(self: @ComponentState<TContractState>) -> EthPublicKey {
             PublicKeyCamel::getPublicKey(self)
         }
 
-        fn setPublicKey(ref self: ComponentState<TContractState>, newPublicKey: EthPublicKey) {
-            PublicKeyCamel::setPublicKey(ref self, newPublicKey);
+        fn setPublicKey(
+            ref self: ComponentState<TContractState>,
+            newPublicKey: EthPublicKey,
+            signature: Span<felt252>
+        ) {
+            PublicKeyCamel::setPublicKey(ref self, newPublicKey, signature);
         }
 
         // ISRC5

diff --git a/src/account/interface.cairo b/src/account/interface.cairo
@@ -97,14 +97,14 @@ trait IEthDeployable<TState> {
 #[starknet::interface]
 trait IEthPublicKey<TState> {
     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<felt252>);
 }
 
 
 #[starknet::interface]
 trait IEthPublicKeyCamel<TState> {
     fn getPublicKey(self: @TState) -> EthPublicKey;
-    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey);
+    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span<felt252>);
 }
 
 //
@@ -131,12 +131,12 @@ trait EthAccountABI<TState> {
 
     // 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<felt252>);
 
     // ISRC6CamelOnly
     fn isValidSignature(self: @TState, hash: felt252, signature: Array<felt252>) -> felt252;
 
     // IEthPublicKeyCamel
     fn getPublicKey(self: @TState) -> EthPublicKey;
-    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey);
+    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span<felt252>);
 }
diff --git a/src/presets/interfaces/eth_account.cairo b/src/presets/interfaces/eth_account.cairo
@@ -23,14 +23,14 @@ trait EthAccountUpgradeableABI<TState> {
 
     // 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<felt252>);
 
     // ISRC6CamelOnly
     fn isValidSignature(self: @TState, hash: felt252, signature: Array<felt252>) -> felt252;
 
     // IEthPublicKeyCamel
     fn getPublicKey(self: @TState) -> EthPublicKey;
-    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey);
+    fn setPublicKey(ref self: TState, newPublicKey: EthPublicKey, signature: Span<felt252>);
 
     // IUpgradeable
     fn upgrade(ref self: TState, new_class_hash: ClassHash);