Skip to content

pallet_revive: Change address derivation to use hashing #7662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
athei merged 5 commits into from
Feb 28, 2025
Merged

pallet_revive: Change address derivation to use hashing #7662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d841fd
Change address derivation to use hashing
athei Feb 20, 2025
0dcac9c
Update from athei running command 'prdoc --audience runtime_dev --bum…
github-actions[bot] Feb 21, 2025
8a38892
Fix docs
athei Feb 21, 2025
ad1290d
Merge branch 'master' into at/amapping
athei Feb 28, 2025
5e66f86
Use keccak
athei Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions prdoc/pr_7662.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
2 changes: 0 additions & 2 deletions substrate/frame/revive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Member

@bkchr bkchr Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use array-bytes as used in the rest of the code base?

Copy link
Member Author

@athei athei Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both seem to be widely used in the code base. Should we transition away from hex-literal?

alexander:~/Developer/polkadot-sdk% cargo tree -i hex-literal --depth 1 | wc -l
      22
alexander:~/Developer/polkadot-sdk% cargo tree -i array-bytes --depth 1 | wc -l
      34

Copy link
Member

@bkchr bkchr Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we tried to push for array-bytes, but all the other are coming back over time :P

Copy link
Member Author

@athei athei Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked into array-bytes. It is not a replacement since it does the conversion at runtime. But we often need it at compile time as literal. Or do you mean the hex crate? I removed the direct dependency but we are still indirectly depending on it via ethereum-types. We will move away eventually to the newer alloy crates.

impl-trait-for-tuples = { workspace = true }
log = { workspace = true }
Expand Down Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions ...es/contracts/terminate_and_send_to_eve.rs → ...ntracts/terminate_and_send_to_argument.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#![no_std]
#![no_main]

extern crate common;
use common::input;
use uapi::{HostFn, HostFnImpl as api};

#[no_mangle]
Expand All @@ -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);
Comment on lines -31 to +32
Copy link
Member Author

@athei athei Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code made assumptions about how the address is derived. So it had to be changed.

}
72 changes: 42 additions & 30 deletions substrate/frame/revive/src/address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`].
///
Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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`.
Expand All @@ -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 {
Expand All @@ -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,
Expand All @@ -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]
}
}

Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/revive/src/evm/api/byte.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
1 change: 1 addition & 0 deletions substrate/frame/revive/src/evm/api/hex_serde.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/revive/src/evm/api/rlp_codec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/revive/src/evm/api/signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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();
Expand Down
11 changes: 6 additions & 5 deletions substrate/frame/revive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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,
}

Expand Down Expand Up @@ -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> {
Expand Down
33 changes: 25 additions & 8 deletions substrate/frame/revive/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand Down
Loading
Loading