6.1 Block gas limit FIXED_GAS cost adjustment can be skipped
During the block building process, the external nullifiers in a PBH transaction are extracted and stored in a HashSet called spent_nullifier_hashes. The purpose of this set is to track all used nullifier hashes, and at the end of the block building process a final transaction can be crafted to mark these hashes as used on the entrypoint contract. If any duplicate nullifier hash is identified within the same block, then that transaction will be marked as invalid and it will be skipped. A snippet showing this logic is presented below:
if let Some(payloads) = pooled_tx.pbh_payload() {
if info.cumulative_gas_used + tx.gas_limit() > verified_gas_limit {
best_txs.mark_invalid(tx.signer(), tx.nonce());
continue;
}
if payloads
.iter()
.any(|payload| !spent_nullifier_hashes.insert(payload.nullifier_hash))
{
best_txs.mark_invalid(tx.signer(), tx.nonce());
invalid_txs.push(*pooled_tx.hash());
continue;
}
}
The result of calling insert will be True if the nullifier hash did not previously exist in the set, and False if the hash has already been inserted into the set before. For two separate transactions sharing the same nullifier hash this approach functions correctly, where the nullifier hash was correctly consumed by the first transaction and the second transaction with the duplicate hash was dropped. The nullifier hash should remain in the set because the first transaction has still properly executed and consumed it.
However, for a single transaction that has two duplicate nullifier hashes, the first insert into the set will be unique, but the second insert will flag as a duplicate so the transaction will be skipped. Even though the transaction was skipped and therefore was not executed, the nullifier hash from the first insert will remain in the set and be marked as used.
This can affect the dynamic block gas limit adjustments which are applied at the end of every transaction execution. There is a FIXED_GAS cost and a COLD_SSTORE_GAS cost which the builder logic accounts for, and reduces that amount from the block gas limit. The FIXED_GAS is only applied on the first PBH transaction, and the COLD_SSTORE_GAS is applied once for every nullifier hash in every PBH transaction. This logic is shown below:
if let Some(payloads) = pooled_tx.pbh_payload() {
if spent_nullifier_hashes.len() == payloads.len() {
block_gas_limit -= FIXED_GAS
}
block_gas_limit -= COLD_SSTORE_GAS * payloads.len() as u64;
}
If a PBH transaction with two duplicate external nullifier hashes is submitted with the highest priority, the block builder will attempt to execute it first. This will result in the execution failing due to duplicate hashes, but one of the nullifier hashes will still remain in spent_nullifier_hashes. The genuine PBH transaction will follow, and the condition required to apply the FIXED_GAS will be impossible to fulfill due to the single entry from the first transaction.
Since the reduction of FIXED_GAS on the block_gas_limit is meant to account for a portion of the cost of the final transaction by the block builder to spend nullifier hashes, by skipping this reduction it introduces a small margin of error where if the amount of gas consumed by regular transactions is at or near the unadjusted block_gas_limit, executing the final builder transaction may exceed the total amount of gas on the block which will lead to execution reverting. The execution of this final transaction is done in a different area than regular transactions, where a revert will cause the block building attempt to fail.
Recommendation
Consider adding a check to ensure that there are no duplicate nullifier hashes in the same transaction, before inserting into the spent_nullifier_hashes hash set.
6.2 PBH gas and nonce limits can become stale
The fields max_pbh_nonce and max_pbh_gas_limit represent the maximum number of PBH transactions per user per month, and the maximum gas limit for a PBH transaction respectively. The values for these fields are loaded from the storage state of the entrypoint contract, as shown below:
pub fn new(...) -> Result<...> {
let state = inner.client().state_by_block_id(BlockId::latest())?;
// Load these fields from the entrypoint storage state
let max_pbh_nonce: u16 = ((state
.storage(pbh_entrypoint, PBH_NONCE_LIMIT_SLOT.into())?
.unwrap_or_default()
>> PBH_NONCE_LIMIT_OFFSET)
& MAX_U16)
.to();
let max_pbh_gas_limit: u64 = state
.storage(pbh_entrypoint, PBH_GAS_LIMIT_SLOT.into())?
.unwrap_or_default()
.to();
//...
}
These fields are read from the contract and only set one time during the initialization of the WorldChainTransactionValidator. The entrypoint smart contract exposes setter functions which are able to change the values at any time by the owner address. If the owner changes the PBH nonce limit or the PBH gas limit on the entrypoint contract, the onchain validations will become out-of-sync with the builder validations. This can cause PBH transactions which should be considered valid according to the entrypoint contract, to be considered as invalid by the builder which may contain stale data, or vice versa. As a result, PBH transactions may be dropped by the builder unexpectedly or the builder may accept transactions only to have them revert during onchain execution.
The function on_new_head_block does have a check which allows the max_pbh_nonce and max_pbh_gas_limit fields to be updated, but this can only occur when both fields are zero. Changing the values in the entrypoint contract from some non-zero value to another value will not result in the fields being updated.
fn on_new_head_block<B>(...) where ...
{
// Will only trigger if both fields are zero
// Cannot handle entrypoint fields being updated
if self.max_pbh_gas_limit.load(Ordering::Relaxed) == 0
&& self.max_pbh_nonce.load(Ordering::Relaxed) == 0
{
// Try and fetch the max pbh nonce and gas limit
if let Ok(state) = self.inner.client().state_by_block_id(...) {
// Try update max pbh nonce
if let Some(max_pbh_nonce) = state.storage(...).ok().flatten(){...}
// Try update pbh gas limit
if let Some(max_pbh_gas_limit) = state.storage(...).ok().flatten(){...}
}
}
//...
}
Recommendation
Consider checking the max_pbh_nonce and max_pbh_gas_limit upon every new block to ensure that the PBH requirements specified in the entrypoint contract remain consistent with the PBH requirements tracked in the builder.
6.3 Dependencies contain known vulnerabilities
cargo-audit reported several dependencies having known vulnerabilities:
Crate: crossbeam-channel
Version: 0.5.14
Title: crossbeam-channel: double free on Drop
Date: 2025-04-08
ID: RUSTSEC-2025-0024
URL: https://rustsec.org/advisories/RUSTSEC-2025-0024
Solution: Upgrade to >=0.5.15
Crate: openssl
Version: 0.10.71
Title: Use-After-Free in `Md::fetch` and `Cipher::fetch`
Date: 2025-04-04
ID: RUSTSEC-2025-0022
URL: https://rustsec.org/advisories/RUSTSEC-2025-0022
Solution: Upgrade to >=0.10.72
Crate: tokio
Version: 1.44.0
Warning: unsound
Title: Broadcast channel calls clone in parallel, but does not require `Sync`
Date: 2025-04-07
ID: RUSTSEC-2025-0023
URL: https://rustsec.org/advisories/RUSTSEC-2025-0023
Full log: cargo-audit-out.log
Recommendation
Consider updating tokio, crossbeam-channel and openssl to latest versions. Note that latter two are transitive dependencies, so consider either updating source dependencies (can be discovered via cargo tree –invert <crate-name>), or patching the dependency directly.
6.4 Inner transaction validation failure does not return early
The function WorldChainTransactionValidator::validate_pbh_bundle has a comment indicating that it should return early if the generic OpTransactionValidation validation fails. However, after the transaction is validated there is no check on tx_outcome to return early if the outcome is invalid. By not returning early, the PBH validations are unnecessarily completed and a failure is only detected at the end of the function:
pub async fn validate_pbh_bundle(...) -> TransactionValidationOutcome<Tx> {
// Ensure that the tx is a valid OP transaction and return early if invalid
let mut tx_outcome = self.inner.validate_one(origin, tx.clone()).await;
// An early return on invalid tx_outcome would be expected here
// PBH validation logic here
// ...
}
Recommendation
Consider adding a check to return early on an invalid tx_outcome to reduce unnecessary PBH validations and to ensure that comments align with the code.
6.5 Redundant transaction gas limit check during block building
When a block is being built in execute_best_transactions, the function is_tx_over_limits is used to validate the transaction and block data limit, as well as the transaction's gas limit:
pub fn is_tx_over_limits(...) -> bool {
// Verify tx data limit
if tx_data_limit.is_some_and(|da_limit| tx.length() as u64 > da_limit) {
return true;
}
// Verify block data limit
if block_data_limit.is_some_and(...){
return true;
}
// Verify tx gas limit
self.cumulative_gas_used + tx.gas_limit() > block_gas_limit
}
Once this validation is passed, there is another gas limit check after the conditional transaction validation. This additional check is unnecessary since the is_tx_over_limits function already ensures that the transaction's gas will be within the specified limits.
// ensure we still have capacity for this transaction
if info.cumulative_gas_used + tx.gas_limit() > block_gas_limit {
// we can't fit this transaction into the block, so we need to mark it as
// invalid which also removes all dependent transaction from
// the iterator before we can continue
best_txs.mark_invalid(tx.signer(), tx.nonce());
continue;
}
Recommendation
Consider removing the unnecessary transaction gas limit check.
6.6 The builder EOA must be adequately funded for blocks to be built
After all PBH and regular transactions have been processed during the block building process, a final transaction is crafted which marks all relevant nullifier hashes as used. The signer for this transaction is a trusted EOA which has permissions to access privileged functions on the entrypoint contract.
pub fn spend_nullifiers_tx<...>(...) -> ... where ..., {
// ...
let mut tx = OpTransactionRequest::default()
.nonce(nonce)
.gas_limit(dyn_gas_limit(nullifier_hashes.len() as u64))
.max_priority_fee_per_gas(evm.ctx().block.basefee.into())
.max_fee_per_gas(evm.ctx().block.basefee.into())
.with_chain_id(evm.ctx().cfg.chain_id)
.with_call(&spendNullifierHashesCall {
_nullifierHashes: nullifier_hashes.into_iter().collect(),
})
.to(ctx.pbh_entry_point)
.build_typed_tx()
.map_err(|e| eyre!("{:?}", e))?;
// ...
}
This transaction has its gas pricing set to the current block's basefee which will be some nonzero amount means each transaction will incur a cost. Therefore, builder EOA addresses must always be adequately funded with native assets to successfully execute these hardcoded transactions. If a builder EOA does not have enough native assets for the transaction, the execute_best_transactions function will fail and blocks cannot be built until the builder EOA address has adequate funds again.
Recommendation
Ensure that the builder EOA address always has adequate funds to avoid block building issues. Alternatively, since these builder transactions are hardcoded to be included in the block regardless of gas priority, the gas price for the transaction could be set to zero and it would still be included in the block. This approach may lead to a simplification of the block_gas_limit dynamic adjustment based on the number of PBH nullifier hashes as well. In this case, please ensure that such a transaction won't fail validation on the op-geth node.
Resolution
Fixed in PR #241. The builder should continue constructing a payload even if it can not submit a transaction to insert used PBH nullifier hashes into the PBH Entrypoint. This prevents a scenario where block production fails due to insufficient builder funds. By continuing with block construction, we ensure there are no slow downs, stuck transactions or negative impact on the user’s ability to transact. Unspent nullifier hashes can be reused while the builder is underfunded. Proper monitoring, alerting, and automated funding should be in place to mitigate this.
6.7 TransactionConditional validations can be reordered to improve performance
The function validate_conditional_options is used during the block building process to determine if the conditions provided with the transaction can be satisfied. These checks can involve block timestamps, block numbers and storage state of contract addresses.
pub fn validate_conditional_options<...>(...) -> RpcResult<()> where ...,
{
//...
// Contract address storage state is checked first
validate_known_accounts(
&options.known_accounts,
latest.header().number().into(),
provider,
)?;
// Timestamps and block are checked after
if let Some(min_block) = options.block_number_min {
if min_block > latest.header().number() {
return Err(ErrorCode::from(-32003).into());
}
}
// More block and timestamp checks...
// ...
}
Validating the storage state of contract addresses is much more expensive than timestamp and block related validations. By validating the block numbers and timestamps first, conditional transactions which have an invalid block number or timestamp can be dropped before reaching the more expensive storage state checks.
Recommendation
Consider changing the logic flow in validate_conditional_options to validate the inexpensive block numbers and timestamp checks before running the more expensive storage state checks.
6.8 ExternalNullifierError is only partially used
The file external_nullifier.rs defines an error enum ExternalNullifierError which contains errors related to invalid external nullifier fields and parsing issues. Only one of the fields (RlpError) is used, with all other fields remaining unused.
#[derive(Debug, Clone, PartialEq, Eq, Error)]
pub enum ExternalNullifierError {
#[error("invalid format: {0}")]
InvalidFormat(#[from] ruint::ParseError),
#[error("{0} is not a valid month number")]
InvalidMonth(u8),
#[error("error parsing external nullifier version")]
InvalidVersion,
#[error("error parsing external nullifier")]
InvalidExternalNullifier,
// Only this field is used
#[error(transparent)]
RlpError(#[from] alloy_rlp::Error),
}
In other functions defined in external_nullifier.rs the alloy_rlp::Error::Custom error is used instead, where some fields from the ExternalNullifierError might be more appropriate. For example the following code snippet could use the ExternalNullifierError::InvalidExternalNullifier instead of the custom error:
if version != Prefix::V1 as u8 {
return Err(alloy_rlp::Error::Custom(
"invalid external nullifier version",
));
}
Recommendation
To improve error consistency related to external nullifiers consider using all fields in ExternalNullifierError where applicable and removing unused errors. Alternatively, the ExternalNullifierError could be removed and alloy_rlp errors could be used for all errors.
