diff --git a/CHANGELOG.md b/CHANGELOG.md index 463e16ffda..6b8a45b695 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,7 @@ - [BREAKING] Fixed batch ID being serialized/deserialized and potentially not matching the serialized transaction headers ([#3061](https://github.com/0xMiden/protocol/pull/3061)). - Simplified the `ownable2step` ownership transitions ([#3170](https://github.com/0xMiden/protocol/pull/3170)). - Fixed `note_script_allowlist::assert_all_input_notes_allowed` and `tx_script_allowlist::assert_tx_script_allowed` to read the allowlist from the transaction's initial storage state via `active_account::get_initial_map_item` ([#3182](https://github.com/0xMiden/protocol/pull/3182)). +- Fixed `eth_address::to_account_id` to validate the decoded `AccountId` structural invariants, preventing a malformed bridge-in destination address from being routed into an unspendable P2ID/MINT output ([#3231](https://github.com/0xMiden/protocol/pull/3231)). ## v0.15.2 (2026-06-05) diff --git a/crates/miden-agglayer/asm/agglayer/common/eth_address.masm b/crates/miden-agglayer/asm/agglayer/common/eth_address.masm index b9cc4a9c33..cdec50cbb3 100644 --- a/crates/miden-agglayer/asm/agglayer/common/eth_address.masm +++ b/crates/miden-agglayer/asm/agglayer/common/eth_address.masm @@ -1,6 +1,7 @@ use agglayer::common::utils use miden::core::crypto::hashes::keccak256 use miden::core::word +use miden::protocol::account_id # TYPE ALIASES # ================================================================================================= @@ -42,9 +43,19 @@ const TWO_POW_32=4294967296 #! The packing is done via build_felt, which validates limbs are u32 and checks the packed value #! did not reduce mod p (i.e. the word fits in the field). #! +#! The decoded (suffix, prefix) is validated as a structurally valid AccountId, matching the Rust +#! `EthEmbeddedAccountId::try_from_eth_address` contract. This guarantees the id is safe to use as +#! an output target: an invalid id would otherwise be routed into an unspendable P2ID/MINT output. +#! #! Inputs: [limb0, limb1, limb2, limb3, limb4] #! Outputs: [suffix, prefix] #! +#! Panics if: +#! - the most-significant 4 bytes (limb0) are non-zero. +#! - any limb is not a valid u32, or a packed 8-byte word does not fit in the field. +#! - the decoded (suffix, prefix) is not a valid AccountId (unsupported version, suffix most +#! significant bit set, or suffix least significant byte non-zero). +#! #! Invocation: exec pub proc to_account_id # limb0 must be 0 (most-significant limb, on top) @@ -67,6 +78,13 @@ pub proc to_account_id swap # => [suffix, prefix] + + # Validate the decoded id is a structurally valid AccountId before it is used as an output target. + dup.1 dup.1 + # => [suffix, prefix, suffix, prefix] + + exec.account_id::validate + # => [suffix, prefix] end # HELPER PROCEDURES diff --git a/crates/miden-testing/tests/agglayer/eth_address.rs b/crates/miden-testing/tests/agglayer/eth_address.rs new file mode 100644 index 0000000000..eaa00365fa --- /dev/null +++ b/crates/miden-testing/tests/agglayer/eth_address.rs @@ -0,0 +1,98 @@ +//! Tests for `AccountId` validation in `eth_address::to_account_id`. +//! +//! The bridge-in claim decodes a deposit's `destinationAddress` into an `AccountId` and routes +//! bridged value to it via a P2ID/MINT output. If the decoded id is not a valid `AccountId`, that +//! output can never be consumed and the assets are locked. + +extern crate alloc; + +use alloc::format; +use alloc::string::String; +use alloc::vec::Vec; + +use miden_agglayer::errors::ERR_MSB_NONZERO; +use miden_agglayer::eth_types::{AddressConversionError, EthAddress, EthEmbeddedAccountId}; +use miden_protocol::Felt; +use miden_protocol::account::AccountId; +use miden_protocol::errors::MasmError; +use miden_protocol::testing::account_id::ACCOUNT_ID_REGULAR_PUBLIC_ACCOUNT_IMMUTABLE_CODE; + +use super::test_utils::assert_execution_fails_with; + +/// Message of `ERR_ACCOUNT_ID_SUFFIX_LEAST_SIGNIFICANT_BYTE_MUST_BE_ZERO` from the protocol +/// `account_id.masm`. It has no exported Rust constant, so we mirror the string (matched by code). +const ERR_SUFFIX_LSB_NONZERO: MasmError = + MasmError::from_static_str("least significant byte of the account ID suffix must be zero"); + +/// Builds a script that pushes the 5 address limbs (`limb0` on top) and runs `to_account_id`. +/// +/// `truncate_stack` lets a non-reverting run finish cleanly, so a missing revert surfaces as a +/// successful execution rather than a stack error. +fn to_account_id_script(addr: &EthAddress) -> String { + let limbs: Vec = addr.to_elements().iter().map(|f| f.as_canonical_u64()).collect(); + format!( + "use miden::core::sys +use agglayer::common::eth_address + +begin + push.{}.{}.{}.{}.{} + exec.eth_address::to_account_id + exec.sys::truncate_stack +end", + limbs[4], limbs[3], limbs[2], limbs[1], limbs[0] + ) +} + +/// Returns an embedded-form address that decodes into a structurally invalid `AccountId` (suffix +/// least-significant byte non-zero), plus its decoded `(suffix, prefix)`. Built from a valid +/// `AccountId` with only the suffix LSB flipped, so `account_id::validate` fails on that check. +fn crafted_invalid_embedded_address() -> (EthAddress, Felt, Felt) { + let valid_id = AccountId::try_from(ACCOUNT_ID_REGULAR_PUBLIC_ACCOUNT_IMMUTABLE_CODE).unwrap(); + let mut bytes = EthEmbeddedAccountId::from_account_id(valid_id).to_bytes(); + + // suffix = bytes[12..20] (big-endian), so byte 19 is its least-significant byte. A valid + // AccountId always has this byte zero; set it to make the suffix structurally invalid. + assert_eq!(bytes[19], 0, "a valid AccountId must have a zero suffix least-significant byte"); + bytes[19] = 1; + + let prefix_u64 = u64::from_be_bytes(bytes[4..12].try_into().unwrap()); + let suffix_u64 = u64::from_be_bytes(bytes[12..20].try_into().unwrap()); + let prefix = Felt::try_from(prefix_u64).unwrap(); + let suffix = Felt::try_from(suffix_u64).unwrap(); + + (EthAddress::new(bytes), suffix, prefix) +} + +/// Case A: a non-embedded destination (non-zero most-significant 4 bytes) hard-traps in +/// `to_account_id`, so the committed claim is deterministically unclaimable. Unchanged by the fix. +#[tokio::test] +async fn to_account_id_rejects_non_embedded_address() { + // A real-looking EVM address: the top 4 bytes are non-zero, so it is not an embedded AccountId. + let addr = EthAddress::from_hex("0xdeadbeefcafebabe0badf00d0011223344556677").unwrap(); + assert_execution_fails_with(&to_account_id_script(&addr), &ERR_MSB_NONZERO).await; +} + +/// Evidence that the crafted destination really is an invalid `AccountId`: the Rust reference +/// conversion rejects it, so the on-chain path accepting it is a genuine gap. +#[test] +fn crafted_invalid_account_id_is_rejected_by_rust() { + let (bad_addr, suffix, prefix) = crafted_invalid_embedded_address(); + + // The Rust embedded-address conversion (used off-chain to build CLAIM notes) rejects it... + assert_eq!( + EthEmbeddedAccountId::try_from(bad_addr), + Err(AddressConversionError::InvalidAccountId), + ); + + // ...precisely because the decoded (suffix, prefix) is not a structurally valid AccountId. + assert!(AccountId::try_from_elements(suffix, prefix).is_err()); +} + +/// Case B (the core bug): an embedded destination that decodes into a structurally invalid +/// `AccountId` must be rejected. Before the fix `to_account_id` accepts it (this test fails, +/// reproducing the bug); after the fix it reverts with the suffix least-significant-byte error. +#[tokio::test] +async fn to_account_id_rejects_structurally_invalid_account_id() { + let (bad_addr, _suffix, _prefix) = crafted_invalid_embedded_address(); + assert_execution_fails_with(&to_account_id_script(&bad_addr), &ERR_SUFFIX_LSB_NONZERO).await; +} diff --git a/crates/miden-testing/tests/agglayer/mod.rs b/crates/miden-testing/tests/agglayer/mod.rs index cf2362ca00..eeab6a29ca 100644 --- a/crates/miden-testing/tests/agglayer/mod.rs +++ b/crates/miden-testing/tests/agglayer/mod.rs @@ -2,6 +2,7 @@ pub mod asset_conversion; mod bridge_in; mod bridge_out; mod config_bridge; +mod eth_address; mod faucet_helpers; mod global_index; mod leaf_utils; diff --git a/crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs b/crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs index 17b42685df..55ec2db8cc 100644 --- a/crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs +++ b/crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs @@ -14,7 +14,6 @@ use miden_processor::{ Program, StackInputs, }; -use miden_protocol::Felt; use miden_protocol::account::AccountId; use miden_protocol::address::NetworkId; use miden_protocol::testing::account_id::{ @@ -25,6 +24,8 @@ use miden_protocol::testing::account_id::{ AccountIdBuilder, }; use miden_protocol::transaction::TransactionKernel; +use miden_protocol::{Felt, ProtocolLib}; +use miden_standards::StandardsLib; /// Execute a program with default host async fn execute_program_with_default_host( @@ -42,6 +43,12 @@ async fn execute_program_with_default_host( host.register_handler(event_name, handler)?; } + let protocol_lib = ProtocolLib::default(); + host.load_library(protocol_lib.mast_forest()).unwrap(); + + let standards_lib = StandardsLib::default(); + host.load_library(standards_lib.mast_forest()).unwrap(); + let asset_conversion_lib = agglayer_library(); host.load_library(asset_conversion_lib.mast_forest()).unwrap(); diff --git a/crates/miden-testing/tests/agglayer/test_utils.rs b/crates/miden-testing/tests/agglayer/test_utils.rs index 3c6490edc3..ade3f287bc 100644 --- a/crates/miden-testing/tests/agglayer/test_utils.rs +++ b/crates/miden-testing/tests/agglayer/test_utils.rs @@ -23,9 +23,11 @@ use miden_processor::{ Program, StackInputs, }; +use miden_protocol::ProtocolLib; use miden_protocol::errors::MasmError; use miden_protocol::transaction::TransactionKernel; use miden_protocol::utils::sync::LazyLock; +use miden_standards::StandardsLib; // EMBEDDED TEST VECTOR JSON FILES // ================================================================================================ @@ -63,6 +65,12 @@ pub async fn execute_program_with_default_host( host.register_handler(event_name, handler)?; } + let protocol_lib = ProtocolLib::default(); + host.load_library(protocol_lib.mast_forest()).unwrap(); + + let standards_lib = StandardsLib::default(); + host.load_library(standards_lib.mast_forest()).unwrap(); + let agglayer_lib = agglayer_library(); host.load_library(agglayer_lib.mast_forest()).unwrap();