Skip to content

Commit 5ee0794

Browse files
antoniolocascio0xVolosnikov
antoniolocascio
and
0xVolosnikov
authored
fix!: low and info audittens fixes (#524)
## What ❔ Fixes for the latest review from Audittens <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted. --------- Co-authored-by: Vladislav Volosnikov <Volosnikov.apmath@gmail.com>
1 parent d2184d0 commit 5ee0794

File tree

28 files changed

+315
-183
lines changed

28 files changed

+315
-183
lines changed

‎basic_bootloader/src/bootloader/block_flow/zk/post_tx_op/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ pub fn read_multichain_root<
215215
&MESSAGE_ROOT_ADDRESS,
216216
&root_slot,
217217
)
218-
.expect("must read MessageRoot shared tree height")
218+
.expect("must read MessageRoot multichain root")
219219
}
220220

221221
///

‎basic_bootloader/src/bootloader/block_flow/zk/tx_loop.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ where
184184
block_data
185185
.transaction_hashes_accumulator
186186
.add_tx_hash(&tx_processing_result.tx_hash);
187-
if tx_processing_result.is_l1_tx {
187+
if tx_processing_result.is_priority_tx {
188188
block_data
189189
.enforced_transaction_hashes_accumulator
190190
.add_tx_hash(&tx_processing_result.tx_hash);

‎basic_bootloader/src/bootloader/transaction_flow/mod.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,14 +64,14 @@ pub enum ExecutionResult<'a, IOTypes: SystemIOTypesConfig> {
6464
}
6565

6666
impl<'a, IOTypes: SystemIOTypesConfig> ExecutionResult<'a, IOTypes> {
67-
pub fn reverted(self) -> Self {
67+
pub fn to_reverted(self) -> Self {
6868
match self {
6969
Self::Success {
70-
output: ExecutionOutput::Call(r),
70+
output: ExecutionOutput::Call(_),
7171
}
7272
| Self::Success {
73-
output: ExecutionOutput::Create(r, _),
74-
} => Self::Revert { output: r },
73+
output: ExecutionOutput::Create(_, _),
74+
} => Self::Revert { output: &[] },
7575
a => a,
7676
}
7777
}

‎basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ pub struct ZkTransactionFlowOnlyEOA<S: EthereumLikeTypes> {
5252
pub struct ZkTxResult<'a> {
5353
pub result: ExecutionResult<'a, EthereumIOTypesConfig>,
5454
pub tx_hash: Bytes32,
55-
pub is_l1_tx: bool,
55+
pub is_priority_tx: bool,
5656
pub is_upgrade_tx: bool,
5757
pub is_service_tx: bool,
5858
pub gas_refunded: u64,
@@ -136,7 +136,7 @@ impl<S: EthereumLikeTypes> core::fmt::Debug for TxContextForPreAndPostProcessing
136136
.field("native_per_gas", &self.native_per_gas)
137137
.field("tx_gas_limit", &self.tx_gas_limit)
138138
.field("gas_used", &self.gas_used)
139-
.field("gas_refunded", &self.gas_used)
139+
.field("gas_refunded", &self.gas_refunded)
140140
.field("validation_pubdata", &self.validation_pubdata)
141141
.field("total_pubdata", &self.total_pubdata)
142142
.field("native_used", &self.native_used)
@@ -478,10 +478,10 @@ where
478478
// go to the operator. Base fees are effectively "burned" (not transferred anywhere).
479479
let gas_price_for_operator = if cfg!(feature = "burn_base_fee") {
480480
let base_fee = system.get_eip1559_basefee();
481-
context
482-
.gas_price
483-
.checked_sub(base_fee)
484-
.ok_or(internal_error!("Gas_price - base_fee underflow"))?
481+
// We use saturating arithmetic to allow the caller of this method to
482+
// allow gas_price < base_fee. This can be used, for example, for
483+
// transaction simulation
484+
context.gas_price.saturating_sub(base_fee)
485485
} else {
486486
context.gas_price
487487
};
@@ -564,7 +564,7 @@ where
564564
ZkTxResult {
565565
result,
566566
tx_hash: context.tx_hash,
567-
is_l1_tx: false,
567+
is_priority_tx: false,
568568
is_upgrade_tx: false,
569569
is_service_tx: transaction.is_service(),
570570
gas_used: context.gas_used,
@@ -862,10 +862,10 @@ where
862862
Some(context.validation_pubdata),
863863
)?;
864864
if !has_enough {
865-
execution_result = execution_result.reverted();
865+
execution_result = execution_result.to_reverted();
866866
system_log!(system, "Not enough gas for pubdata after execution\n");
867867
Ok((
868-
execution_result.reverted(),
868+
execution_result.to_reverted(),
869869
CachedPubdataInfo {
870870
pubdata_used,
871871
to_charge_for_pubdata,

‎basic_bootloader/src/bootloader/transaction_flow/zk/process_l1_transaction.rs‎

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::bootloader::config::BasicBootloaderExecutionConfig;
22
use crate::bootloader::constants::{
3-
FREE_L1_TX_NATIVE_PER_GAS, L1_TX_INTRINSIC_NATIVE_COST, L1_TX_INTRINSIC_PUBDATA,
4-
L1_TX_NATIVE_PRICE,
3+
FREE_L1_TX_NATIVE_PER_GAS, L1_TX_INTRINSIC_L2_GAS, L1_TX_INTRINSIC_NATIVE_COST,
4+
L1_TX_INTRINSIC_PUBDATA, L1_TX_NATIVE_PRICE,
55
};
66
use crate::bootloader::errors::BootloaderInterfaceError;
77
use crate::bootloader::errors::TxError;
@@ -90,7 +90,7 @@ where
9090
native_per_gas,
9191
native_per_pubdata,
9292
minimal_gas_used,
93-
} = prepare_and_check_resources::<S>(
93+
} = prepare_and_check_resources::<S, Config>(
9494
system,
9595
transaction,
9696
is_priority_op,
@@ -333,7 +333,7 @@ where
333333
Ok(ZkTxResult {
334334
result,
335335
tx_hash,
336-
is_l1_tx: is_priority_op,
336+
is_priority_tx: is_priority_op,
337337
is_upgrade_tx: !is_priority_op,
338338
is_service_tx: false,
339339
gas_used,
@@ -365,7 +365,11 @@ struct ResourceAndFeeInfo<S: EthereumLikeTypes> {
365365
/// The approach is to use saturating arithmetic and emit a system
366366
/// log if this situation ever happens.
367367
///
368-
fn prepare_and_check_resources<'a, S: EthereumLikeTypes + 'a>(
368+
fn prepare_and_check_resources<
369+
'a,
370+
S: EthereumLikeTypes + 'a,
371+
Config: BasicBootloaderExecutionConfig,
372+
>(
369373
system: &mut System<S>,
370374
transaction: &AbiEncodedTransaction<S::Allocator>,
371375
is_priority_op: bool,
@@ -382,15 +386,24 @@ where
382386
let native_price = L1_TX_NATIVE_PRICE;
383387
let native_per_gas = if is_priority_op {
384388
if gas_price.is_zero() {
385-
FREE_L1_TX_NATIVE_PER_GAS
386-
} else {
387-
u256_try_to_u64(&gas_price.div_ceil(native_price))
388-
.unwrap_or_else(|| {
389-
system_log!(
390-
system,
391-
"Native per gas calculation for L1 tx overflows, using saturated arithmetic instead");
389+
if Config::SIMULATION {
390+
u256_try_to_u64(&system.get_eip1559_basefee().div_ceil(native_price))
391+
.unwrap_or_else(|| {
392+
system_log!(
393+
system,
394+
"Native per gas calculation for L1 tx overflows, using saturated arithmetic instead");
392395
u64::MAX
393-
})
396+
})
397+
} else {
398+
FREE_L1_TX_NATIVE_PER_GAS
399+
}
400+
} else {
401+
u256_try_to_u64(&gas_price.div_ceil(native_price)).unwrap_or_else(|| {
402+
system_log!(
403+
system,
404+
"Native per gas calculation for L1 tx overflows, using saturated arithmetic instead");
405+
u64::MAX
406+
})
394407
}
395408
} else {
396409
// Upgrade txs are paid by the protocol, so we use a fixed native per gas
@@ -406,9 +419,15 @@ where
406419
u64::MAX
407420
});
408421

409-
let native_prepaid_from_gas = native_per_gas.saturating_mul(gas_limit);
422+
let native_prepaid_from_gas = native_per_gas.checked_mul(gas_limit)
423+
.unwrap_or_else(|| {
424+
system_log!(
425+
system,
426+
"Native prepaid from gas calculation for L1 tx overflows, using saturated arithmetic instead");
427+
u64::MAX
428+
});
410429

411-
let (calldata_tokens, intrinsic_cost) =
430+
let (calldata_tokens, minimal_gas_used) =
412431
compute_calldata_tokens(system, transaction.calldata(), true);
413432

414433
// With L1ResourcesPolicy, this returns Result<ResourcesForTx<S>, BootloaderSubsystemError>
@@ -422,14 +441,21 @@ where
422441
false, // is_deployment
423442
transaction.calldata().len() as u64,
424443
calldata_tokens,
425-
intrinsic_cost,
444+
L1_TX_INTRINSIC_L2_GAS,
426445
L1_TX_INTRINSIC_PUBDATA,
427446
L1_TX_INTRINSIC_NATIVE_COST,
428447
)?;
429448

430-
// L1 transactions might have a gas limit < intrinsic cost,
431-
// so we pick the min as minimal_gas_used.
432-
let minimal_gas_used = intrinsic_cost.min(gas_limit);
449+
// L1 transactions might have a gas limit < minimal_gas_used. This should be
450+
// prevented by L1 validation, but we log and saturate if it happens.
451+
if gas_limit < minimal_gas_used {
452+
system_log!(
453+
system,
454+
"L1 tx gas limit below intrinsic cost, using saturated arithmetic instead"
455+
);
456+
}
457+
// Pick the min to keep processing L1 txs even if the L1 validation is wrong.
458+
let minimal_gas_used = minimal_gas_used.min(gas_limit);
433459

434460
Ok(ResourceAndFeeInfo {
435461
resources,
@@ -578,7 +604,7 @@ where
578604
check_enough_resources_for_pubdata(system, native_per_pubdata, resources, None)?;
579605
let execution_result = if !enough {
580606
system_log!(system, "Not enough gas for pubdata after execution\n");
581-
execution_result.reverted()
607+
execution_result.to_reverted()
582608
} else {
583609
execution_result
584610
};

‎basic_bootloader/src/bootloader/transaction_flow/zk/validation_impl.rs‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -322,18 +322,19 @@ where
322322
// Parse blobs, if any
323323
// No need to feature gate this part, as blobs() should return an empty list
324324
// for non-EIP4844 transactions.
325+
let block_base_fee_per_blob_gas = system.get_blob_base_fee_per_gas();
326+
327+
#[cfg(not(feature = "eip-4844"))]
328+
crate::require_internal!(
329+
block_base_fee_per_blob_gas == U256::ONE,
330+
"Blob base fee should be set to 1 if EIP 4844 is disabled",
331+
system
332+
)?;
333+
325334
let blobs = if let Some(blobs_list) = transaction.blobs() {
326335
let tx_max_fee_per_blob_gas = transaction.max_fee_per_blob_gas().ok_or(internal_error!(
327336
"Tx with blobs must define max_fee_per_blob_gas"
328337
))?;
329-
let block_base_fee_per_blob_gas = system.get_blob_base_fee_per_gas();
330-
331-
#[cfg(not(feature = "eip-4844"))]
332-
crate::require_internal!(
333-
block_base_fee_per_blob_gas == U256::ONE,
334-
"Blob base fee should be set to 1 if EIP 4844 is disabled",
335-
system
336-
)?;
337338

338339
if &block_base_fee_per_blob_gas > tx_max_fee_per_blob_gas && !Config::SIMULATION {
339340
return Err(TxError::Validation(

‎basic_system/src/system_implementation/ethereum_storage_model/storage_model.rs‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use crate::system_implementation::ethereum_storage_model::caches::full_storage_c
1010
use crate::system_implementation::ethereum_storage_model::caches::preimage::BytecodeKeccakPreimagesStorage;
1111
use crate::system_implementation::ethereum_storage_model::persist_changes::EthereumStoragePersister;
1212
use core::alloc::Allocator;
13-
use ruint::aliases::B160;
1413
use storage_models::common_structs::snapshottable_io::SnapshottableIo;
1514
use storage_models::common_structs::StorageCacheModel;
1615
use storage_models::common_structs::StorageModel;
@@ -374,34 +373,6 @@ impl<
374373
);
375374
}
376375

377-
type AccountAddress<'a>
378-
= &'a B160
379-
where
380-
Self: 'a;
381-
type AccountDiff<'a>
382-
= BasicAccountDiff<Self::IOTypes>
383-
where
384-
Self: 'a;
385-
386-
fn get_account_diff<'a>(
387-
&'a self,
388-
_address: Self::AccountAddress<'a>,
389-
) -> Option<Self::AccountDiff<'a>> {
390-
None
391-
}
392-
fn accounts_diffs_iterator<'a>(
393-
&'a self,
394-
) -> impl ExactSizeIterator<Item = (Self::AccountAddress<'a>, Self::AccountDiff<'a>)> + Clone
395-
{
396-
self.account_cache.cache.iter().map(|v| {
397-
let current = v.current().value();
398-
(
399-
v.key().as_ref(),
400-
(current.nonce, current.balance, current.bytecode_hash),
401-
)
402-
})
403-
}
404-
405376
type StorageKey<'a>
406377
= &'a WarmStorageKey
407378
where

‎basic_system/src/system_implementation/flat_storage_model/mod.rs‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ use storage_models::common_structs::StorageCacheModel;
2525
use storage_models::common_structs::StorageModel;
2626
use zk_ee::system::errors::internal::InternalError;
2727
use zk_ee::system::BalanceSubsystemError;
28-
use zk_ee::system::BasicAccountDiff;
2928
use zk_ee::system::DeconstructionSubsystemError;
3029
use zk_ee::system::NonceSubsystemError;
3130
use zk_ee::system::Resources;
@@ -439,28 +438,6 @@ impl<
439438
.expect("must report preimages");
440439
}
441440

442-
type AccountAddress<'a>
443-
= &'a B160
444-
where
445-
Self: 'a;
446-
type AccountDiff<'a>
447-
= BasicAccountDiff<Self::IOTypes>
448-
where
449-
Self: 'a;
450-
451-
fn get_account_diff<'a>(
452-
&'a self,
453-
_address: Self::AccountAddress<'a>,
454-
) -> Option<Self::AccountDiff<'a>> {
455-
None
456-
}
457-
fn accounts_diffs_iterator<'a>(
458-
&'a self,
459-
) -> impl ExactSizeIterator<Item = (Self::AccountAddress<'a>, Self::AccountDiff<'a>)> + Clone
460-
{
461-
[].into_iter()
462-
}
463-
464441
type StorageKey<'a>
465442
= &'a WarmStorageKey
466443
where

‎basic_system/src/system_implementation/system/io_subsystem.rs‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -769,28 +769,6 @@ impl<
769769
self.storage.report_new_preimages(result_keeper);
770770
}
771771

772-
type AccountAddress<'a>
773-
= M::AccountAddress<'a>
774-
where
775-
Self: 'a;
776-
type AccountDiff<'a>
777-
= M::AccountDiff<'a>
778-
where
779-
Self: 'a;
780-
781-
fn get_account_diff<'a>(
782-
&'a self,
783-
address: Self::AccountAddress<'a>,
784-
) -> Option<Self::AccountDiff<'a>> {
785-
self.storage.get_account_diff(address)
786-
}
787-
fn accounts_diffs_iterator<'a>(
788-
&'a self,
789-
) -> impl ExactSizeIterator<Item = (Self::AccountAddress<'a>, Self::AccountDiff<'a>)> + Clone
790-
{
791-
self.storage.accounts_diffs_iterator()
792-
}
793-
794772
type StorageKey<'a>
795773
= M::StorageKey<'a>
796774
where

‎docs/system_hooks.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ The hook accepts the following ABI-encoded parameters:
3939
- `bytes32` - observable bytecode hash
4040

4141
Key features:
42-
- Enforces EIP-158 by rejecting bytecode longer than 24576 bytes
42+
- Enforces EIP-170 by rejecting bytecode longer than 24576 bytes
4343
- Used exclusively for protocol upgrades approved by governance
4444
- Does not publish full bytecode in pubdata to fit within gas/calldata limits
4545
- Bytecodes are published separately via Ethereum calldata

0 commit comments

Comments
 (0)