Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
83 changes: 71 additions & 12 deletions crates/evm/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use alloy_evm::{
},
eth::{EthBlockExecutor, EthTxResult},
};
use alloy_sol_types::SolEvent as _;
use alloy_sol_types::{SolCall as _, SolEvent as _};
use reth_evm::block::StateDB;
use reth_revm::{Inspector, context::result::ResultAndState};
use tempo_evm::{TempoBlockExecutionCtx, TempoReceiptBuilder};
Expand All @@ -22,9 +22,7 @@ use tempo_revm::evm::TempoContext;
use tempo_zone_contracts::IZoneOutbox;
use zone_chainspec::ZoneChainSpec;
use zone_l1::state::L1StateProvider;
use zone_precompiles::{
ADVANCE_TEMPO_SELECTOR, L1StorageReader, is_finalize_withdrawal_batch_calldata,
};
use zone_precompiles::{ADVANCE_TEMPO_SELECTOR, L1StorageReader};
use zone_primitives::constants::{ZONE_INBOX_ADDRESS, ZONE_OUTBOX_ADDRESS};

use crate::{L1OverlayDB, ZoneEvm};
Expand Down Expand Up @@ -106,7 +104,8 @@ impl ZoneTransactionKind {
}

if tx.calls().any(|(kind, input)| {
kind.to() == Some(&ZONE_OUTBOX_ADDRESS) && is_finalize_withdrawal_batch_calldata(input)
kind.to() == Some(&ZONE_OUTBOX_ADDRESS)
&& input.starts_with(&IZoneOutbox::finalizeWithdrawalBatchCall::SELECTOR)
}) {
return Self::FinalizeWithdrawalBatch;
}
Expand Down Expand Up @@ -285,7 +284,7 @@ mod tests {
use reth_chainspec::EthChainSpec as _;
use reth_primitives_traits::Recovered;
use revm::database::{CacheDB, EmptyDB};
use tempo_chainspec::spec::DEV;
use tempo_chainspec::{hardfork::TempoHardfork, spec::DEV};
use tempo_evm::TempoBlockExecutionCtx;
use tempo_precompiles::{
DEFAULT_FEE_TOKEN, TIP_FEE_MANAGER_ADDRESS,
Expand Down Expand Up @@ -461,14 +460,74 @@ mod tests {
ZONE_OUTBOX_ADDRESS,
Bytes::copy_from_slice(&IZoneOutbox::finalizeWithdrawalBatchCall::SELECTOR),
);
let error = ZoneBlockPhase::Executing
.validate_transaction(&malformed_finalize)
.unwrap_err();
assert_eq!(
error.to_string(),
"system transactions after advanceTempo must call \
ZoneOutbox.finalizeWithdrawalBatch"
ZoneBlockPhase::Executing
.validate_transaction(&malformed_finalize)
.unwrap(),
ZoneBlockPhase::WithdrawalsFinalized
);

let mut trailing_calldata = IZoneOutbox::finalizeWithdrawalBatchCall {
count: U256::ZERO,
blockNumber: 1,
encryptedSenders: vec![],
}
.abi_encode();
trailing_calldata.extend_from_slice(&[0; 32]);
let trailing_finalize = system_tx(ZONE_OUTBOX_ADDRESS, trailing_calldata.into());
assert_eq!(
ZoneBlockPhase::Executing
.validate_transaction(&trailing_finalize)
.unwrap(),
ZoneBlockPhase::WithdrawalsFinalized
);
}

#[test]
fn malformed_t11_finalization_does_not_advance_block_phase() {
let mut zone_genesis = DEV.genesis().clone();
zone_genesis.config.chain_id = zone_chain_id(DEV.chain().id(), 2).unwrap();
let chain_spec = std::sync::Arc::new(ZoneChainSpec::from_genesis(zone_genesis).unwrap());
let factory =
ZoneEvmFactory::new(chain_spec.clone(), MockL1Reader::default(), Address::ZERO);
let mut env = EvmEnv::default();
env.cfg_env.spec = TempoHardfork::T11;
let evm = factory.create_evm(CacheDB::new(EmptyDB::default()), env);
let ctx = TempoBlockExecutionCtx {
inner: EthBlockExecutionCtx {
parent_hash: B256::ZERO,
parent_beacon_block_root: None,
ommers: &[],
withdrawals: None,
extra_data: Bytes::new(),
tx_count_hint: Some(1),
slot_number: None,
},
general_gas_limit: 0,
shared_gas_limit: 0,
validator_set: None,
consensus_context: None,
subblock_fee_recipients: Default::default(),
};
let mut executor = ZoneBlockExecutor::new(evm, ctx, &chain_spec);
executor.phase = ZoneBlockPhase::Executing;

let tx = Recovered::new_unchecked(
system_tx(
ZONE_OUTBOX_ADDRESS,
Bytes::copy_from_slice(&IZoneOutbox::finalizeWithdrawalBatchCall::SELECTOR),
),
TEMPO_SYSTEM_TX_SENDER,
);
let error = executor.execute_transaction_without_commit(tx).unwrap_err();

assert!(
error
.to_string()
.contains("system transaction execution failed"),
"unexpected error: {error}"
);
assert_eq!(executor.phase, ZoneBlockPhase::Executing);
}

#[test]
Expand Down
12 changes: 9 additions & 3 deletions crates/node/src/replication.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use alloy_primitives::B256;
use alloy_provider::DynProvider;
use alloy_rlp::Decodable as _;
use alloy_rpc_types_engine::ForkchoiceState;
use alloy_sol_types::SolCall as _;
use alloy_sol_types::{SolCall as _, abi::AbiDecoderConfig};
use futures::{StreamExt as _, stream::BoxStream};
use reth_chain_state::PersistedBlockSubscriptions;
use reth_node_api::{ConsensusEngineHandle, PayloadTypes as _};
Expand All @@ -18,6 +18,7 @@ use std::{
time::Duration,
};
use tempo_alloy::TempoNetwork;
use tempo_precompiles::dispatch::ABI_DECODER_MEMORY_LIMIT;
use tempo_primitives::{Block, TempoHeader, TempoTxEnvelope};
use tokio::sync::{mpsc, oneshot};
use tokio_util::sync;
Expand Down Expand Up @@ -1375,8 +1376,13 @@ fn decode_advance_tempo(
if signed.tx().to != ZONE_INBOX_ADDRESS.into() {
eyre::bail!("first Tempo system transaction is not sent to IZoneInbox")
}
let call = IZoneInbox::advanceTempoCall::abi_decode(signed.tx().input.as_ref())
.map_err(|err| eyre::eyre!("first transaction does not decode as advanceTempo: {err}"))?;
let call = IZoneInbox::advanceTempoCall::abi_decode_with_config(
signed.tx().input.as_ref(),
AbiDecoderConfig::new()
.memory_limit(ABI_DECODER_MEMORY_LIMIT)
.strict(true),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚨 [POTENTIAL-VULNERABILITY] Fork-unaware strict advanceTempo decoding can make followers reject valid pre-T11 blocks

This unconditional strict decoder runs before engine.new_payload, but pre-T11 ZoneInbox execution uses fork-aware non-strict decoding and the executor classifies advanceTempo by selector prefix. A leader can add otherwise-ignored ABI trailing data that execution accepts while followers reject during replication import, stalling backfill.

Recommended Fix:
Use the same fork-aware decoder config as ZoneInbox dispatch for the block's active spec, or keep only the memory limit pre-T11. Add a regression test that replication and execution accept the same advanceTempo calldata set across forks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

leader is normal ABI encoding

Comment thread
0xalpharush marked this conversation as resolved.
)
.map_err(|err| eyre::eyre!("first transaction does not decode as advanceTempo: {err}"))?;

// 3. the system tx is valid.
let mut header_rlp = call.header.as_ref();
Expand Down
43 changes: 41 additions & 2 deletions crates/precompiles/src/account_keychain.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Zone read-privacy rules for the upstream Tempo AccountKeychain precompile.

use alloy_primitives::Address;
use alloy_sol_types::SolInterface;
use alloy_sol_types::{SolCall, SolInterface};
use tempo_contracts::precompiles::IAccountKeychain;
use tempo_precompiles::dispatch::abi_decoder_config_for_spec;

Expand All @@ -15,11 +15,36 @@ use crate::{
#[derive(Clone)]
pub(crate) struct AccountKeychainRules;

const UNRESTRICTED_SELECTORS: &[[u8; 4]] = &[
IAccountKeychain::authorizeKey_0Call::SELECTOR,
IAccountKeychain::authorizeKey_1Call::SELECTOR,
IAccountKeychain::authorizeKey_2Call::SELECTOR,
IAccountKeychain::authorizeAdminKeyCall::SELECTOR,
IAccountKeychain::burnKeyAuthorizationWitnessCall::SELECTOR,
IAccountKeychain::revokeKeyCall::SELECTOR,
IAccountKeychain::updateSpendingLimitCall::SELECTOR,
IAccountKeychain::setAllowedCallsCall::SELECTOR,
IAccountKeychain::removeAllowedCallsCall::SELECTOR,
IAccountKeychain::getTransactionKeyCall::SELECTOR,
];

impl CallRules for AccountKeychainRules {
fn admit(&self, data: &[u8], caller: Address) -> CallCheck {
let spec = StorageCtx::default().spec();

// These calls have no Zone-specific privacy policy. Defer directly to the upstream
// dispatcher for selector scheduling and ABI decoding.
if data.get(..4).is_some_and(|selector| {
UNRESTRICTED_SELECTORS
.iter()
.any(|allowed| selector == allowed.as_slice())
}) {
return CallCheck::Continue;
}

let Ok(call) = IAccountKeychain::IAccountKeychainCalls::abi_decode_with_config(
data,
abi_decoder_config_for_spec(StorageCtx::default().spec()),
abi_decoder_config_for_spec(spec),
) else {
// Preserve the upstream error and gas behavior for malformed or unknown calldata.
return CallCheck::Continue;
Expand Down Expand Up @@ -229,4 +254,18 @@ mod tests {
CallCheck::Continue
));
}

#[test]
fn malformed_unrestricted_calls_remain_deferred_to_upstream() {
let rules = AccountKeychainRules;

for data in UNRESTRICTED_SELECTORS {
for spec in [TempoHardfork::T10, TempoHardfork::T11] {
assert!(matches!(
admit_at(&rules, data, Address::ZERO, spec),
CallCheck::Continue
));
}
}
}
}
24 changes: 11 additions & 13 deletions crates/precompiles/src/inbox/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ use alloc::vec::Vec;

use alloy_evm::precompiles::DynPrecompile;
use alloy_primitives::{Address, B256, U256};
use alloy_sol_types::{SolCall, SolType, SolValue};
use alloy_sol_types::{SolCall, SolValue, abi::AbiDecoderConfig};
use tempo_precompiles::{
PATH_USD_ADDRESS,
dispatch::ABI_DECODER_MEMORY_LIMIT,
error::TempoPrecompileError,
storage::{Handler, Mapping, Slot, StorageCtx},
tip20::{ISSUER_ROLE, ITIP20, TIP20Error, TIP20Token},
Expand Down Expand Up @@ -378,27 +379,24 @@ impl TryFrom<QueuedDeposit> for DecodedQueuedDeposit {
type Error = ZonePrecompileError;

fn try_from(queued: QueuedDeposit) -> Result<Self, Self::Error> {
let config = AbiDecoderConfig::new()
.memory_limit(ABI_DECODER_MEMORY_LIMIT)
.strict(true);

match queued.depositType {
DepositType::WithdrawalBounceBack => {
decode_canonical(&queued.depositData).map(Self::WithdrawalBounceBack)
WithdrawalBounceBackDeposit::abi_decode_with_config(&queued.depositData, config)
.map(Self::WithdrawalBounceBack)
}
DepositType::Deposit => {
Deposit::abi_decode_with_config(&queued.depositData, config).map(Self::Deposit)
}
DepositType::Deposit => decode_canonical(&queued.depositData).map(Self::Deposit),
_ => return Err(ZonePrecompileError::MalformedCalldata),
}
.map_err(|_| ZonePrecompileError::MalformedCalldata)
}
}

fn decode_canonical<T>(encoded: &[u8]) -> alloy_sol_types::Result<T>
where
T: SolValue + From<<T::SolType as SolType>::RustType>,
{
let value = T::abi_decode(encoded)?;
(value.abi_encode().as_slice() == encoded)
.then_some(value)
.ok_or(alloy_sol_types::Error::ReserMismatch)
}

fn decode_deposits(deposits: Vec<QueuedDeposit>) -> ZoneResult<Vec<DecodedQueuedDeposit>> {
deposits.into_iter().map(TryInto::try_into).collect()
}
Expand Down
2 changes: 1 addition & 1 deletion crates/precompiles/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub mod ztip20;
pub use aes_gcm::AesGcmDecrypt;
pub use chaum_pedersen::ChaumPedersenVerify;
pub use inbox::{ADVANCE_TEMPO_SELECTOR, ZoneInbox};
pub use outbox::{ZoneOutbox, is_finalize_withdrawal_batch_calldata};
pub use outbox::ZoneOutbox;
pub use storage::{L1State, L1StateError, L1StorageReader};
pub use tempo_contracts::precompiles::TIP403_REGISTRY_ADDRESS;
pub use tempo_state::TempoState;
Expand Down
9 changes: 0 additions & 9 deletions crates/precompiles/src/outbox/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ mod tests;
use alloc::vec::Vec;

use alloy_primitives::{Address, B256, Bytes, U256};
use alloy_sol_types::SolCall;
use tempo_precompiles::{
Result as TempoResult,
error::TempoPrecompileError,
Expand All @@ -32,14 +31,6 @@ use crate::{
pub const MAX_CALLBACK_DATA_SIZE: usize = 1024;
const WITHDRAWAL_BASE_GAS: u64 = 50_000;

/// Returns whether `calldata` is a canonical `finalizeWithdrawalBatch` call.
pub fn is_finalize_withdrawal_batch_calldata(calldata: &[u8]) -> bool {
let Ok(call) = IZoneOutbox::finalizeWithdrawalBatchCall::abi_decode(calldata) else {
return false;
};
call.abi_encode() == calldata
}

#[contract(addr = ZONE_OUTBOX_ADDRESS)]
pub struct ZoneOutbox {
tempo_gas_rate: u128,
Expand Down
Loading