diff --git a/Cargo.lock b/Cargo.lock
index a110a6d0f4d718d45f27f47ea4ae0585416b10d6..817004c537e46de746212ef091710fc785e0f5cb 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -12740,7 +12740,6 @@ dependencies = [
"frame-benchmarking",
"frame-support",
"frame-system",
- "hex",
"hex-literal",
"impl-trait-for-tuples",
"log",
diff --git a/prdoc/pr_7662.prdoc b/prdoc/pr_7662.prdoc
new file mode 100644
index 0000000000000000000000000000000000000000..050ef3aa88e4df0a0a27f18c133cc8c123918389
--- /dev/null
+++ b/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
diff --git a/substrate/frame/revive/Cargo.toml b/substrate/frame/revive/Cargo.toml
index a10f8935fb60d1d4df303cc129958a28d4c765e1..b3d8feb890f37abacb9fab2c93679d5b104292d7 100644
--- a/substrate/frame/revive/Cargo.toml
+++ b/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",
diff --git a/substrate/frame/revive/fixtures/contracts/terminate_and_send_to_eve.rs b/substrate/frame/revive/fixtures/contracts/terminate_and_send_to_argument.rs
similarity index 91%
rename from substrate/frame/revive/fixtures/contracts/terminate_and_send_to_eve.rs
rename to substrate/frame/revive/fixtures/contracts/terminate_and_send_to_argument.rs
index c078f9d46c1d71bfe079051bd34a2d01eb270328..d34736de30a650075841c3e7e943fdfa293dee6a 100644
--- a/substrate/frame/revive/fixtures/contracts/terminate_and_send_to_eve.rs
+++ b/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);
}
diff --git a/substrate/frame/revive/src/address.rs b/substrate/frame/revive/src/address.rs
index 45b5bf822dc91dd94691c00425d6c3ef3f826a2f..dc7ccd3fb5a4992bc09fde514914c91ecb697b83 100644
--- a/substrate/frame/revive/src/address.rs
+++ b/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]
}
}
diff --git a/substrate/frame/revive/src/evm/api/byte.rs b/substrate/frame/revive/src/evm/api/byte.rs
index f11966d0072cf64d674bd2925862720192c43924..b25ff2da27b1413c225c1a9c8a5ce4e5fd2aaabf 100644
--- a/substrate/frame/revive/src/evm/api/byte.rs
+++ b/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},
diff --git a/substrate/frame/revive/src/evm/api/hex_serde.rs b/substrate/frame/revive/src/evm/api/hex_serde.rs
index ba07b36fa4be6e5cba82bba719c3893cd32aafc5..250a38496a63f4c260faa8bd8f1bf8fb0f21b132 100644
--- a/substrate/frame/revive/src/evm/api/hex_serde.rs
+++ b/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 {
diff --git a/substrate/frame/revive/src/evm/api/rlp_codec.rs b/substrate/frame/revive/src/evm/api/rlp_codec.rs
index 9b61cd042ec5433655c1002bcab62b6d67bd2fd7..5e9b9915352af681470918449536cc540899ecb6 100644
--- a/substrate/frame/revive/src/evm/api/rlp_codec.rs
+++ b/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();
diff --git a/substrate/frame/revive/src/evm/api/signature.rs b/substrate/frame/revive/src/evm/api/signature.rs
index 9f39b92b461eabbed376f31ea8cc3259363368a7..de4ac428ffe4e0ded74b796cebf306936b862e3f 100644
--- a/substrate/frame/revive/src/evm/api/signature.rs
+++ b/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();
diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs
index 62633a8f30b80408db5f202d46b94371a25cf483..c7af353de8cdd7c609186f38dd20dc767e5f6866 100644
--- a/substrate/frame/revive/src/lib.rs
+++ b/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> {
diff --git a/substrate/frame/revive/src/test_utils.rs b/substrate/frame/revive/src/test_utils.rs
index acd9a4cda38a808f55aa37bc858f45ecd0484674..0f5eef7a30e2d47dba95958d50d2679532a3c8ee 100644
--- a/substrate/frame/revive/src/test_utils.rs
+++ b/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);
diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs
index a20b6d0c630566b942728cfef9dc011a7b594d86..29f1b73e82c64cc02e96763b677237b15e48b778 100644
--- a/substrate/frame/revive/src/tests.rs
+++ b/substrate/frame/revive/src/tests.rs
@@ -4213,7 +4213,7 @@ fn origin_must_be_mapped() {
#[test]
fn mapped_address_works() {
- let (code, _) = compile_module("terminate_and_send_to_eve").unwrap();
+ let (code, _) = compile_module("terminate_and_send_to_argument").unwrap();
ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
<Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
@@ -4222,7 +4222,7 @@ fn mapped_address_works() {
let Contract { addr, .. } =
builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();
assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 0);
- builder::bare_call(addr).build_and_unwrap_result();
+ builder::bare_call(addr).data(EVE_ADDR.encode()).build_and_unwrap_result();
assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 100);
// after mapping it will be sent to the real eve account
@@ -4231,12 +4231,43 @@ fn mapped_address_works() {
// need some balance to pay for the map deposit
<Test as Config>::Currency::set_balance(&EVE, 1_000);
<Pallet<Test>>::map_account(RuntimeOrigin::signed(EVE)).unwrap();
- builder::bare_call(addr).build_and_unwrap_result();
+ builder::bare_call(addr).data(EVE_ADDR.encode()).build_and_unwrap_result();
assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 100);
assert_eq!(<Test as Config>::Currency::total_balance(&EVE), 1_100);
});
}
+#[test]
+fn recovery_works() {
+ let (code, _) = compile_module("terminate_and_send_to_argument").unwrap();
+
+ ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
+ <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+ // eve puts her AccountId20 as argument to terminate but forgot to register
+ // her AccountId32 first so now the funds are trapped in her fallback account
+ let Contract { addr, .. } =
+ builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE), 0);
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 0);
+ builder::bare_call(addr).data(EVE_ADDR.encode()).build_and_unwrap_result();
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 100);
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE), 0);
+
+ let call = RuntimeCall::Balances(pallet_balances::Call::transfer_all {
+ dest: EVE,
+ keep_alive: false,
+ });
+
+ // she now uses the recovery function to move all funds from the fallback
+ // account to her real account
+ <Pallet<Test>>::dispatch_as_fallback_account(RuntimeOrigin::signed(EVE), Box::new(call))
+ .unwrap();
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE_FALLBACK), 0);
+ assert_eq!(<Test as Config>::Currency::total_balance(&EVE), 100);
+ });
+}
+
#[test]
fn skip_transfer_works() {
let (code_caller, _) = compile_module("call").unwrap();