pallet_revive: Change address derivation to use hashing (#7662)

Fixes #6723

Internal auditors recommended to not truncate Polkadot Addresses when
deriving Ethereum addresses from it. Reasoning is that they are raw
public keys where truncating could lead to collisions when weaknesses in
those curves are discovered in the future. Additionally, some pallets
generate account addresses in a way where only the suffix we were
truncating contains any entropy. The changes in this PR act as a safe
guard against those two points.

We change the `to_address` function to first hash the AccountId32 and
then use trailing 20 bytes as `AccountId20`. If the `AccountId32` ends
with 12x 0xEE we keep our current behaviour of just truncating those
trailing bytes.

This will allow us to still recover the original `AccountId20` because
those are constructed by just adding those 12 bytes. Please note that
generating an ed25519 key pair where the trailing 12 bytes are 0xEE is
theoretically possible as 96bits is not a huge search space. However,
this cannot be used as an attack vector. It will merely allow this
address to interact with `pallet_revive` without registering as the
fallback account is the same as the actual address. The ultimate vanity
address. In practice, this is not relevant since the 0xEE addresses are
not valid public keys for sr25519 which is used almost everywhere.

tl:dr: We keep truncating in case of an Ethereum address derived account
id. This is safe as those are already derived via keccak. In every other
case where we have to assume that the account id might be a public key.
Therefore we first hash and then take the trailing bytes.

No. We changed the name of the mapping. This means the runtime will not
try to read the old data. Ethereum keys are unaffected by this change.
We just advise people to re-register their AccountId32 in case they need
to use it as it is a very small circle of users (just 3 addresses
registered). This will not cause disturbance on Westend.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

prdoc/pr_7662.prdoc

@@ -0,0 +1,26 @@
+title: 'pallet_revive: Change address derivation to use hashing'
+doc:
+- audience: Runtime Dev
+  description: |-
+    ## Motivation
+
+    Internal auditors recommended to not truncate Polkadot Addresses when deriving Ethereum addresses from it. Reasoning is that they are raw public keys where truncating could lead to collisions when weaknesses in those curves are discovered in the future. Additionally,  some pallets generate account addresses in a way where only the suffix we were truncating contains any entropy. The changes in this PR act as a safe guard against those two points.
+
+    ## Changes made
+
+    We change the `to_address` function to first hash the AccountId32 and then use trailing 20 bytes as `AccountId20`. If the `AccountId32` ends with 12x 0xEE we keep our current behaviour of just truncating those trailing bytes.
+
+    ## Security Discussion
+
+    This will allow us to still recover the original `AccountId20` because those are constructed by just adding those 12 bytes. Please note that generating an ed25519 key pair where the trailing 12 bytes are 0xEE is theoretically possible as 96bits is not a huge search space. However, this cannot be used as an attack vector. It will merely allow this address to interact with `pallet_revive` without registering as the fallback account is the same as the actual address. The ultimate vanity address. In practice, this is not relevant since the 0xEE addresses are not valid public keys for sr25519 which is used almost everywhere.
+
+    tl:dr: We keep truncating in case of an Ethereum address derived account id. This is safe as those are already derived via keccak. In every other case where we have to assume that the account id might be a public key. Therefore we first hash and then take the trailing bytes.
+
+    ## Do we need a Migration for Westend
+
+    No. We changed the name of the mapping. This means the runtime will not try to read the old data. Ethereum keys are unaffected by this change. We just advise people to re-register their AccountId32 in case they need to use it as it is a very small circle of users (just 3 addresses registered). This will not cause disturbance on Westend.
+crates:
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-fixtures
+  bump: major

substrate/frame/revive/Cargo.toml

@@ -23,7 +23,6 @@ derive_more = { workspace = true }
 environmental = { workspace = true }
 ethabi = { workspace = true }
 ethereum-types = { workspace = true, features = ["codec", "rlp", "serialize"] }
-hex = { workspace = true }
 hex-literal = { workspace = true }
 impl-trait-for-tuples = { workspace = true }
 log = { workspace = true }
@@ -87,7 +86,6 @@ std = [
 	"frame-benchmarking?/std",
 	"frame-support/std",
 	"frame-system/std",
-	"hex/std",
 	"log/std",
 	"pallet-proxy/std",
 	"pallet-revive-fixtures?/std",

substrate/frame/revive/fixtures/contracts/terminate_and_send_to_eve.rs → substrate/frame/revive/fixtures/contracts/terminate_and_send_to_argument.rs

@@ -18,7 +18,7 @@
 #![no_std]
 #![no_main]
 
-extern crate common;
+use common::input;
 use uapi::{HostFn, HostFnImpl as api};
 
 #[no_mangle]
@@ -28,6 +28,6 @@ pub extern "C" fn deploy() {}
 #[no_mangle]
 #[polkavm_derive::polkavm_export]
 pub extern "C" fn call() {
-	let eve = [5u8; 20];
-	api::terminate(&eve);
+	input!(beneficiary: &[u8; 20],);
+	api::terminate(&beneficiary);
 }

substrate/frame/revive/src/address.rs

@@ -17,13 +17,13 @@
 
 //! Functions that deal contract addresses.
 
-use crate::{ensure, AddressSuffix, Config, Error, HoldReason};
+use crate::{ensure, Config, Error, HoldReason, OriginalAccount};
 use alloc::vec::Vec;
 use core::marker::PhantomData;
 use frame_support::traits::{fungible::MutateHold, tokens::Precision};
 use sp_core::{Get, H160};
 use sp_io::hashing::keccak_256;
-use sp_runtime::{AccountId32, DispatchResult, SaturatedConversion, Saturating};
+use sp_runtime::{AccountId32, DispatchResult, Saturating};
 
 /// Map between the native chain account id `T` and an Ethereum [`H160`].
 ///
@@ -40,7 +40,7 @@ use sp_runtime::{AccountId32, DispatchResult, SaturatedConversion, Saturating};
 ///
 /// We require the mapping to be reversible. Since we are potentially dealing with types of
 /// different sizes one direction of the mapping is necessarily lossy. This requires the mapping to
-/// make use of the [`AddressSuffix`] storage item to reverse the mapping.
+/// make use of the [`OriginalAccount`] storage item to reverse the mapping.
 pub trait AddressMapper<T: Config>: private::Sealed {
 	/// Convert an account id to an ethereum adress.
 	fn to_address(account_id: &T::AccountId) -> H160;
@@ -50,7 +50,7 @@ pub trait AddressMapper<T: Config>: private::Sealed {
 
 	/// Same as [`Self::to_account_id`] but always returns the fallback account.
 	///
-	/// This skips the query into [`AddressSuffix`] and always returns the stateless
+	/// This skips the query into [`OriginalAccount`] and always returns the stateless
 	/// fallback account. This is useful when we know for a fact that the `address`
 	/// in question is originally a `H160`. This is usually only the case when we
 	/// generated a new contract address.
@@ -83,11 +83,10 @@ mod private {
 
 /// The mapper to be used if the account id is `AccountId32`.
 ///
-/// It converts between addresses by either truncating the last 12 bytes or
-/// suffixing them. The suffix is queried from [`AddressSuffix`] and will fall
-/// back to all `0xEE` if no suffix was registered. This means contracts and
-/// plain wallets controlled by an `secp256k1` always have a `0xEE` suffixed
-/// account.
+/// It converts between addresses by either hash then truncate the last 12 bytes or
+/// suffixing them. To recover the original account id of a hashed and truncated account id we use
+/// [`OriginalAccount`] and will fall back to all `0xEE` if account was found. This means contracts
+/// and plain wallets controlled by an `secp256k1` always have a `0xEE` suffixed account.
 pub struct AccountId32Mapper<T>(PhantomData<T>);
 
 /// The mapper to be used if the account id is `H160`.
@@ -100,18 +99,21 @@ where
 	T: Config<AccountId = AccountId32>,
 {
 	fn to_address(account_id: &AccountId32) -> H160 {
-		H160::from_slice(&<AccountId32 as AsRef<[u8; 32]>>::as_ref(&account_id)[..20])
+		let account_bytes: &[u8; 32] = account_id.as_ref();
+		if Self::is_eth_derived(account_id) {
+			// this was originally an eth address
+			// we just strip the 0xEE suffix to get the original address
+			H160::from_slice(&account_bytes[..20])
+		} else {
+			// this is an (ed|sr)25510 derived address
+			// avoid truncating the public key by hashing it first
+			let account_hash = keccak_256(account_bytes);
+			H160::from_slice(&account_hash[12..])
+		}
 	}
 
 	fn to_account_id(address: &H160) -> AccountId32 {
-		if let Some(suffix) = <AddressSuffix<T>>::get(address) {
-			let mut account_id = Self::to_fallback_account_id(address);
-			let account_bytes: &mut [u8; 32] = account_id.as_mut();
-			account_bytes[20..].copy_from_slice(suffix.as_slice());
-			account_id
-		} else {
-			Self::to_fallback_account_id(address)
-		}
+		<OriginalAccount<T>>::get(address).unwrap_or_else(|| Self::to_fallback_account_id(address))
 	}
 
 	fn to_fallback_account_id(address: &H160) -> AccountId32 {
@@ -124,24 +126,19 @@ where
 	fn map(account_id: &T::AccountId) -> DispatchResult {
 		ensure!(!Self::is_mapped(account_id), <Error<T>>::AccountAlreadyMapped);
 
-		let account_bytes: &[u8; 32] = account_id.as_ref();
-
-		// each mapping entry stores one AccountId32 distributed between key and value
+		// each mapping entry stores the address (20 bytes) and the account id (32 bytes)
 		let deposit = T::DepositPerByte::get()
-			.saturating_mul(account_bytes.len().saturated_into())
+			.saturating_mul(52u32.into())
 			.saturating_add(T::DepositPerItem::get());
-
-		let suffix: [u8; 12] = account_bytes[20..]
-			.try_into()
-			.expect("Skipping 20 byte of a an 32 byte array will fit into 12 bytes; qed");
 		T::Currency::hold(&HoldReason::AddressMapping.into(), account_id, deposit)?;
-		<AddressSuffix<T>>::insert(Self::to_address(account_id), suffix);
+
+		<OriginalAccount<T>>::insert(Self::to_address(account_id), account_id);
 		Ok(())
 	}
 
 	fn unmap(account_id: &T::AccountId) -> DispatchResult {
 		// will do nothing if address is not mapped so no check required
-		<AddressSuffix<T>>::remove(Self::to_address(account_id));
+		<OriginalAccount<T>>::remove(Self::to_address(account_id));
 		T::Currency::release_all(
 			&HoldReason::AddressMapping.into(),
 			account_id,
@@ -151,9 +148,24 @@ where
 	}
 
 	fn is_mapped(account_id: &T::AccountId) -> bool {
+		Self::is_eth_derived(account_id) ||
+			<OriginalAccount<T>>::contains_key(Self::to_address(account_id))
+	}
+}
+
+impl<T> AccountId32Mapper<T>
+where
+	T: Config<AccountId = AccountId32>,
+{
+	/// Returns true if the passed account id is controlled by an eth key.
+	///
+	/// This is a stateless check that just compares the last 12 bytes. Please note that it is
+	/// theoretically possible to create an ed25519 keypair that passed this filter. However,
+	/// this can't be used for an attack. It also won't happen by accident since everbody is using
+	/// sr25519 where this is not a valid public key.
+	fn is_eth_derived(account_id: &T::AccountId) -> bool {
 		let account_bytes: &[u8; 32] = account_id.as_ref();
-		&account_bytes[20..] == &[0xEE; 12] ||
-			<AddressSuffix<T>>::contains_key(Self::to_address(account_id))
+		&account_bytes[20..] == &[0xEE; 12]
 	}
 }
 

substrate/frame/revive/src/evm/api/byte.rs

@@ -15,8 +15,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //! Define Byte wrapper types for encoding and decoding hex strings
+
 use super::hex_serde::HexCodec;
 use alloc::{vec, vec::Vec};
+use alloy_core::hex;
 use codec::{Decode, Encode};
 use core::{
 	fmt::{Debug, Display, Formatter, Result as FmtResult},

substrate/frame/revive/src/evm/api/hex_serde.rs

@@ -16,6 +16,7 @@
 // limitations under the License.
 
 use alloc::{format, string::String, vec::Vec};
+use alloy_core::hex;
 use serde::{Deserialize, Deserializer, Serializer};
 
 pub trait HexCodec: Sized {

substrate/frame/revive/src/evm/api/rlp_codec.rs

@@ -557,7 +557,7 @@ mod test {
 		];
 
 		for (tx, json) in txs {
-			let raw_tx = hex::decode(tx).unwrap();
+			let raw_tx = alloy_core::hex::decode(tx).unwrap();
 			let tx = TransactionSigned::decode(&raw_tx).unwrap();
 			assert_eq!(tx.signed_payload(), raw_tx);
 			let expected_tx = serde_json::from_str(json).unwrap();

substrate/frame/revive/src/evm/api/signature.rs

@@ -15,6 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //! Ethereum signature utilities
+
 use super::*;
 use sp_core::{H160, U256};
 use sp_io::{crypto::secp256k1_ecdsa_recover, hashing::keccak_256};
@@ -173,7 +174,7 @@ fn sign_and_recover_work() {
 	));
 
 	for tx in txs {
-		let raw_tx = hex::decode(tx).unwrap();
+		let raw_tx = alloy_core::hex::decode(tx).unwrap();
 		let tx = TransactionSigned::decode(&raw_tx).unwrap();
 
 		let address = tx.recover_eth_address();

substrate/frame/revive/src/lib.rs

@@ -75,7 +75,7 @@ use scale_info::TypeInfo;
 use sp_core::{H160, H256, U256};
 use sp_runtime::{
 	traits::{BadOrigin, Bounded, Convert, Dispatchable, Saturating, Zero},
-	DispatchError,
+	AccountId32, DispatchError,
 };
 
 pub use crate::{
@@ -504,7 +504,7 @@ pub mod pallet {
 		CodeUploadDepositReserve,
 		/// The Pallet has reserved it for storage deposit.
 		StorageDepositReserve,
-		/// Deposit for creating an address mapping in [`AddressSuffix`].
+		/// Deposit for creating an address mapping in [`OriginalAccount`].
 		AddressMapping,
 	}
 
@@ -539,11 +539,12 @@ pub mod pallet {
 
 	/// Map a Ethereum address to its original `AccountId32`.
 	///
-	/// Stores the last 12 byte for addresses that were originally an `AccountId32` instead
-	/// of an `H160`. Register your `AccountId32` using [`Pallet::map_account`] in order to
+	/// When deriving a `H160` from an `AccountId32` we use a hash function. In order to
+	/// reconstruct the original account we need to store the reverse mapping here.
+	/// Register your `AccountId32` using [`Pallet::map_account`] in order to
 	/// use it with this pallet.
 	#[pallet::storage]
-	pub(crate) type AddressSuffix<T: Config> = StorageMap<_, Identity, H160, [u8; 12]>;
+	pub(crate) type OriginalAccount<T: Config> = StorageMap<_, Identity, H160, AccountId32>;
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {

substrate/frame/revive/src/test_utils.rs

@@ -22,10 +22,12 @@
 
 pub mod builder;
 
+pub use sp_runtime::AccountId32;
+
 use crate::{BalanceOf, Config};
 use frame_support::weights::Weight;
+use hex_literal::hex;
 use sp_core::H160;
-pub use sp_runtime::AccountId32;
 
 const fn ee_suffix(mut account: [u8; 32]) -> AccountId32 {
 	let mut i = 20;
@@ -36,26 +38,41 @@ const fn ee_suffix(mut account: [u8; 32]) -> AccountId32 {
 	AccountId32::new(account)
 }
 
+const fn ee_extend(address: [u8; 20]) -> AccountId32 {
+	let mut account = [0xEEu8; 32];
+	let mut i = 0;
+	while i < 20 {
+		account[i] = address[i];
+		i += 1;
+	}
+	AccountId32::new(account)
+}
+
+// All those accounts ids end in `ee` which means they don't
+// need a stateful mapping and their address is derived
+// by truncation without a hash applied/
+
 pub const ALICE: AccountId32 = ee_suffix([1u8; 32]);
 pub const ALICE_ADDR: H160 = H160([1u8; 20]);
-pub const ALICE_FALLBACK: AccountId32 = ee_suffix([1u8; 32]);
+pub const ALICE_FALLBACK: AccountId32 = ALICE;
 
 pub const BOB: AccountId32 = ee_suffix([2u8; 32]);
 pub const BOB_ADDR: H160 = H160([2u8; 20]);
-pub const BOB_FALLBACK: AccountId32 = ee_suffix([2u8; 32]);
+pub const BOB_FALLBACK: AccountId32 = BOB;
 
 pub const CHARLIE: AccountId32 = ee_suffix([3u8; 32]);
 pub const CHARLIE_ADDR: H160 = H160([3u8; 20]);
-pub const CHARLIE_FALLBACK: AccountId32 = ee_suffix([3u8; 32]);
+pub const CHARLIE_FALLBACK: AccountId32 = CHARLIE;
 
 pub const DJANGO: AccountId32 = ee_suffix([4u8; 32]);
 pub const DJANGO_ADDR: H160 = H160([4u8; 20]);
-pub const DJANGO_FALLBACK: AccountId32 = ee_suffix([4u8; 32]);
+pub const DJANGO_FALLBACK: AccountId32 = DJANGO;
 
-/// Eve is a non ee account and hence needs a stateful mapping.
+/// Eve is a non ee account and hence needs a stateful mapping and its
+/// address is derived by hashing to avoid truncating public keys.
 pub const EVE: AccountId32 = AccountId32::new([5u8; 32]);
-pub const EVE_ADDR: H160 = H160([5u8; 20]);
-pub const EVE_FALLBACK: AccountId32 = ee_suffix([5u8; 32]);
+pub const EVE_ADDR: H160 = H160(hex!("e21eecd6e51cbcda5b0c5207ae87e605839e70ef"));
+pub const EVE_FALLBACK: AccountId32 = ee_extend(EVE_ADDR.0);
 
 pub const GAS_LIMIT: Weight = Weight::from_parts(100_000_000_000, 3 * 1024 * 1024);