diff --git a/Cargo.lock b/Cargo.lock index dc3348eab7..96c4ee5aa2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11190,6 +11190,7 @@ dependencies = [ "serde", "serde_with", "sov-accounts", + "sov-bank", "sov-modules-api", "sov-state", "sov-test-utils", diff --git a/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs b/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs index c5e4f57621..2f22b03fe2 100644 --- a/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs +++ b/crates/module-system/module-implementations/integration-tests/tests/stf_blueprint/operator/auth_eip712.rs @@ -1,4 +1,6 @@ -use sov_accounts::{Accounts, CallMessage as AccountsCallMessage}; +use sov_accounts::{ + derive_address_for_new_credential, Accounts, CallMessage as AccountsCallMessage, +}; use sov_address::{EthereumAddress, EvmCryptoSpec}; use sov_eip712_auth::{ Eip712Authenticator, Eip712AuthenticatorInput, Eip712AuthenticatorTrait, SchemaProvider, @@ -229,19 +231,35 @@ fn correct_signature_is_accepted() { #[test] fn test_multisig_signature_verification() { use sov_test_utils::AsUser; - let (mut runner, admin) = setup(); - // First, create and register a multisig + // Pre-generate the multisig so we can configure its (future) derived address + // as the ValueSetter admin at genesis. Without this the multisig-signed txs + // below would be rejected with "Only admin can change the value". let multisig_keys = [ TestPrivateKey::generate(), TestPrivateKey::generate(), TestPrivateKey::generate(), ]; - - // Create the multisig and register it let multisig = Multisig::new(2, multisig_keys.iter().map(|k| k.pub_key()).collect()); let multisig_credential_id = multisig.credential_id::<<::CryptoSpec as CryptoSpec>::Hasher>(); + + let genesis_config = + HighLevelOptimisticGenesisConfig::generate().add_accounts_with_default_balance(2); + let admin = genesis_config + .additional_accounts() + .first() + .unwrap() + .clone(); + let multisig_address = + derive_address_for_new_credential::(&multisig_credential_id, &admin.address()); + let module_config = sov_value_setter::ValueSetterConfig { + admin: multisig_address, + }; + let genesis = GenesisConfig::from_minimal_config(genesis_config.clone().into(), module_config); + let mut runner = TestRunner::new_with_genesis(genesis.into_genesis_params(), RT::default()); + + // Register the multisig credential: inserts `multisig_credential_id -> multisig_address`. runner.execute_transaction(TransactionTestCase { input: admin.create_plain_message::>( AccountsCallMessage::InsertCredentialId(multisig_credential_id), @@ -251,6 +269,23 @@ fn test_multisig_signature_verification() { }), }); + // The multisig now lives at its own, non-controlled address and has no balance — + // fund it from `admin` so subsequent multisig-signed transactions can reserve gas. + runner.execute_transaction(TransactionTestCase { + input: admin.create_plain_message::>( + sov_bank::CallMessage::Transfer { + to: multisig_address, + coins: sov_bank::Coins { + amount: sov_bank::Amount::new(1_000_000_000_000), + token_id: sov_bank::config_gas_token_id(), + }, + }, + ), + assert: Box::new(move |result, _state| { + assert!(result.tx_receipt.is_successful(), "Funding transfer failed"); + }), + }); + // Create a multisig transaction let utx = create_utx::(encode_message::<_, RT>()); let mut signatures = Vec::new(); diff --git a/crates/module-system/module-implementations/sov-accounts/Cargo.toml b/crates/module-system/module-implementations/sov-accounts/Cargo.toml index 2622868de9..55e71820a4 100644 --- a/crates/module-system/module-implementations/sov-accounts/Cargo.toml +++ b/crates/module-system/module-implementations/sov-accounts/Cargo.toml @@ -29,6 +29,7 @@ arbitrary = { workspace = true, optional = true } sov-accounts = { path = ".", features = ["native"] } tempfile = { workspace = true } strum = { workspace = true } +sov-bank = { workspace = true, features = ["native"] } sov-modules-api = { workspace = true, features = ["test-utils"] } sov-test-utils = { workspace = true } @@ -40,10 +41,12 @@ arbitrary = [ "sov-modules-api/arbitrary", "sov-state/arbitrary", "sov-test-utils/arbitrary", + "sov-bank/arbitrary", ] native = [ "sov-accounts/native", "sov-modules-api/native", "sov-state/native", + "sov-bank/native", ] test-utils = [] diff --git a/crates/module-system/module-implementations/sov-accounts/src/call.rs b/crates/module-system/module-implementations/sov-accounts/src/call.rs index 6b23a9ece5..6bb0770e42 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/call.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/call.rs @@ -1,11 +1,10 @@ -use anyhow::bail; -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, bail, Result}; use schemars::JsonSchema; use sov_modules_api::macros::{serialize, UniversalWallet}; use sov_modules_api::{Context, CredentialId, Spec, StateReader, TxState}; use sov_state::namespaces::User; -use crate::{Account, Accounts}; +use crate::{derive_address_for_new_credential, Account, Accounts}; /// Represents the available call messages for interacting with the sov-accounts module. #[derive(Debug, PartialEq, Eq, Clone, JsonSchema, UniversalWallet)] @@ -32,13 +31,12 @@ impl Accounts { bail!("Custom account mappings are disabled"); } + // Important invariant: credential ids must never be deleted once inserted. self.exit_if_credential_exists(&new_credential_id, state)?; - // Insert the new credential id -> account mapping - let account = Account { - addr: *context.sender(), - }; - self.accounts.set(&new_credential_id, &account, state)?; + let addr = derive_address_for_new_credential::(&new_credential_id, context.sender()); + self.accounts + .set(&new_credential_id, &Account { addr }, state)?; Ok(()) } @@ -53,7 +51,7 @@ impl Accounts { .get(new_credential_id, state) .map_err(|err| anyhow!("Error raised while getting account: {err:?}"))? .is_none(), - "New CredentialId already exists" + "New CredentialId already exists: {new_credential_id}" ); Ok(()) } diff --git a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs index 8d84fc04d4..fd3fa9217b 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/capabilities.rs @@ -5,8 +5,19 @@ use crate::{Account, Accounts}; impl Accounts { /// Resolve the sender's public key to an address. - /// If the sender is not registered, but a fallback address if provided, immediately registers - /// the credential to the fallback and then returns it. + /// + /// If the credential is already registered (by a prior `InsertCredentialId` + /// or prior auto-registration), returns the stored address. + /// + /// Otherwise immediately auto-registers the credential to `default_address` + /// (typically `Address::from(credential_id)`) and returns that. Note that + /// this auto-registration produces a **different** address than + /// [`CallMessage::InsertCredentialId`](crate::CallMessage::InsertCredentialId) + /// would — the latter derives `hash(credential_id || registering_sender)` + /// via [`derive_address_for_new_credential`](crate::derive_address_for_new_credential). + /// Callers that need a specific on-chain address for a credential (e.g. a + /// multisig) must pre-register via `InsertCredentialId` before the + /// credential's first tx. pub fn resolve_sender_address( &mut self, default_address: &S::Address, diff --git a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs index 5bf6eefaec..324020d912 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/genesis.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/genesis.rs @@ -25,7 +25,7 @@ pub struct AccountData
{ pub struct AccountConfig { /// Accounts to initialize the rollup. pub accounts: Vec>, - /// Enable custom `CredentailId` => `Account` mapping. + /// Enable custom `CredentialId` => `Account` mapping. #[serde(default = "default_true")] pub enable_custom_account_mappings: bool, } diff --git a/crates/module-system/module-implementations/sov-accounts/src/lib.rs b/crates/module-system/module-implementations/sov-accounts/src/lib.rs index 6bfe8bbda0..0b9dec26c3 100644 --- a/crates/module-system/module-implementations/sov-accounts/src/lib.rs +++ b/crates/module-system/module-implementations/sov-accounts/src/lib.rs @@ -12,11 +12,13 @@ mod query; pub use query::*; #[cfg(test)] mod tests; +mod utils; pub use call::CallMessage; use sov_modules_api::{ Context, CredentialId, DaSpec, GenesisState, Module, ModuleId, ModuleInfo, ModuleRestApi, Spec, StateMap, StateValue, TxState, }; +pub use utils::derive_address_for_new_credential; /// An account on the rollup. #[derive( diff --git a/crates/module-system/module-implementations/sov-accounts/src/utils.rs b/crates/module-system/module-implementations/sov-accounts/src/utils.rs new file mode 100644 index 0000000000..cf79c68274 --- /dev/null +++ b/crates/module-system/module-implementations/sov-accounts/src/utils.rs @@ -0,0 +1,27 @@ +use sov_modules_api::digest::Digest; +use sov_modules_api::{CredentialId, CryptoSpec, Spec}; + +/// Derives the address assigned to a newly inserted credential. +/// +/// The derivation mixes the transaction sender into the hash so that a +/// multisig registered by A does not alias A's own address — a later +/// compromise of A's single key must not also control the multisig. +/// +/// This derivation is only used by `InsertCredentialId`. A credential that +/// skips explicit registration and first appears as the signer of a tx is +/// auto-registered by `Accounts::resolve_sender_address` to +/// `Address::from(credential_id)` instead — a different address. Callers +/// that need a specific on-chain address for a credential (e.g. a multisig) +/// must pre-register it via `InsertCredentialId` before its first tx. +pub fn derive_address_for_new_credential( + new_credential_id: &CredentialId, + sender: &S::Address, +) -> S::Address { + let mut hasher = ::Hasher::new(); + hasher.update(new_credential_id.0 .0); + hasher.update(sender.as_ref()); + let hash: [u8; 32] = hasher.finalize().into(); + // Route through `CredentialId` so each Spec's existing `From` + // handles address-width reduction (28 / 32 / 20 bytes). + S::Address::from(CredentialId::from_bytes(hash)) +} diff --git a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs index c23bf2806a..aaf6eaefd9 100644 --- a/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs +++ b/crates/module-system/module-implementations/sov-accounts/tests/integration/main.rs @@ -1,4 +1,5 @@ -use sov_accounts::{Accounts, CallMessage, Response}; +use sov_accounts::{derive_address_for_new_credential, Accounts, CallMessage, Response}; +use sov_bank::{config_gas_token_id, Amount, Bank, Coins}; use sov_modules_api::transaction::{UnsignedTransaction, Version1}; use sov_modules_api::{ CryptoSpec, PrivateKey, PublicKey, RawTx, Runtime, SkippedTxContents, Spec, TxEffect, @@ -107,11 +108,11 @@ fn test_update_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. + // New credential maps to a freshly derived, non-controlled address. assert_eq!( accounts.get_account(new_credential, state), Response::AccountExists { - addr: user.address() + addr: derive_address_for_new_credential::(&new_credential, &user.address()), } ); // Account corresponding to the old credential still exists. @@ -132,23 +133,28 @@ fn test_update_account_fails() { let ( TestData { account_1, - account_2, + non_registered_account, .. }, mut runner, ) = setup(); + // `non_registered_account` has no `custom_credential_id`, so its signing + // credential matches the address that holds its balance — gas reservation + // can succeed and the call handler actually runs. runner.execute_transaction(TransactionTestCase { - input: account_1.create_plain_message::>(CallMessage::InsertCredentialId( - account_2.credential_id(), - )), - assert: Box::new(move |result, _state| { - if let TxEffect::Reverted(contents) = result.tx_receipt { - assert_eq!( - contents.reason.to_string(), - "New CredentialId already exists" + input: non_registered_account.create_plain_message::>( + CallMessage::InsertCredentialId(account_1.credential_id()), + ), + assert: Box::new(move |result, _state| match result.tx_receipt { + TxEffect::Reverted(contents) => { + let reason = contents.reason.to_string(); + assert!( + reason.contains("New CredentialId already exists"), + "Unexpected revert reason: {reason}", ); } + other => panic!("Expected reverted transaction, got {other:?}"), }), }); } @@ -174,6 +180,10 @@ fn test_setup_multisig_and_act() { let multisig = Multisig::new(2, multisig_keys.iter().map(|k| k.pub_key()).collect()); let multisig_credential_id = multisig.credential_id::<<::CryptoSpec as CryptoSpec>::Hasher>(); + let user_address = user.address(); + let user_credential_id = user.credential_id(); + let multisig_address = + derive_address_for_new_credential::(&multisig_credential_id, &user_address); runner.execute_transaction(TransactionTestCase { input: user.create_plain_message::>(CallMessage::InsertCredentialId( multisig_credential_id, @@ -183,22 +193,38 @@ fn test_setup_multisig_and_act() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. + // Multisig credential maps to a freshly derived, non-controlled address — + // it must not alias the creator's address, otherwise a compromise of the + // creator's single key would also control the multisig. assert_eq!( accounts.get_account(multisig_credential_id, state), Response::AccountExists { - addr: user.address() + addr: multisig_address, } ); // Account corresponding to the old credential still exists. assert_eq!( - accounts.get_account(user.credential_id(), state), - Response::AccountExists { - addr: user.address() - } + accounts.get_account(user_credential_id, state), + Response::AccountExists { addr: user_address } ); - assert_ne!(multisig_credential_id, user.credential_id()); + assert_ne!(multisig_credential_id, user_credential_id); + }), + }); + + // The multisig now lives at its own, non-controlled address and has no balance of + // its own — transfer from `user` so subsequent multisig-signed transactions can + // reserve gas. + runner.execute_transaction(TransactionTestCase { + input: user.create_plain_message::>(sov_bank::CallMessage::Transfer { + to: multisig_address, + coins: Coins { + amount: Amount::new(1_000_000_000_000), + token_id: config_gas_token_id(), + }, + }), + assert: Box::new(move |result, _state| { + assert!(result.tx_receipt.is_successful(), "Funding transfer failed"); }), }); @@ -405,11 +431,14 @@ fn test_register_new_account() { let accounts = Accounts::::default(); - // New account with the new public key and an old address is created. + // New credential maps to a freshly derived, non-controlled address. assert_eq!( accounts.get_account(new_credential, state), Response::AccountExists { - addr: non_registered_account.address() + addr: derive_address_for_new_credential::( + &new_credential, + &non_registered_account.address(), + ), } ); @@ -507,18 +536,25 @@ fn test_resolve_address_if_more_than_one_credential() { runner.query_visible_state(|state| { let mut accounts = Accounts::::default(); + // Each credential now resolves to its own derived address, not the sender's. assert_eq!( accounts .resolve_sender_address(&default_address_1, &credential_1, state) .unwrap(), - non_registered_account.address() + derive_address_for_new_credential::( + &credential_1, + &non_registered_account.address() + ), ); assert_eq!( accounts .resolve_sender_address(&default_address_2, &credential_2, state) .unwrap(), - non_registered_account.address() + derive_address_for_new_credential::( + &credential_2, + &non_registered_account.address() + ), ); }); } diff --git a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json index 5b4c04b15a..867858f505 100644 --- a/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json +++ b/crates/module-system/module-schemas/genesis-schemas/sov-accounts.json @@ -15,7 +15,7 @@ } }, "enable_custom_account_mappings": { - "description": "Enable custom `CredentailId` => `Account` mapping.", + "description": "Enable custom `CredentialId` => `Account` mapping.", "default": true, "type": "boolean" }