fix: add permissioned EVM validation when gas is specified

[chatton]

Description

The logic to check for allow list was only happening in the flow for gas estimation, and was not happening when a gas value was being directly applied.

This PR applies the check in both places, and also extracts out the logic to a single check_deploy_allowed function.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Performance improvement
• Refactoring

Related Issues

Fixes #(issue)

Checklist

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Testing

Additional Notes

[claude]

Claude finished @chatton's task —— View job

---

Code Review for PR #122

I've conducted a comprehensive review of this PR that fixes the permissioned EVM validation bug. Overall, this is a well-structured fix that addresses a critical security issue.

Todo List:

• Read and analyze the changed files
• Review code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Examine test coverage
• Provide constructive feedback

---

✅ Positive Aspects

1. Security Fix: Successfully addresses the critical bug where deploy allowlist checks were bypassed when gas values were directly specified
2. Code Organization: Good refactoring with the extracted check_deploy_allowed function promoting reusability
3. Comprehensive Integration: Properly integrates the allowlist check in both execution handler and transaction pool validator
4. Error Handling: Adds appropriate error types (DeployCheckError, EvTxPoolError::DeployNotAllowed)
5. Test Coverage: Includes new test case for allowlist enforcement

⚠️ Issues & Concerns

High Priority Issues:

1. Silent Failure Risk in crates/node/src/txpool.rs:413-415:

   let block_number = self.inner.client().best_block_number().map_err(|err| {
       InvalidPoolTransactionError::other(EvTxPoolError::StateProvider(err.to_string()))
   })?;

   The current code properly handles the error (contrary to Gemini's review). However, there's still a potential issue - if the block number query fails, validation fails entirely. Consider if this is the desired behavior vs. defaulting to a safe fallback.

2. Configuration Error Handling in crates/node/src/txpool.rs:568-577:

   let evolve_config = crate::config::EvolvePayloadBuilderConfig::from_chain_spec(
       ctx.chain_spec().as_ref(),
   )
   .unwrap_or_else(|err| {
       warn!(target: "reth::cli", "Failed to parse evolve config from chainspec: {err}");
       Default::default()
   });

   Good: The code properly logs warnings when config parsing fails, making debugging easier.

Medium Priority Issues:

3. Test Assertion Weakness in crates/node/src/txpool.rs:771:

   assert!(matches!(err, InvalidPoolTransactionError::Other(_)));

   The test assertion is too generic. Consider strengthening it:

   if let Err(InvalidPoolTransactionError::Other(err)) = result {
       assert!(
           err.as_any().downcast_ref::<EvTxPoolError>().map_or(false, |e| matches!(e, EvTxPoolError::DeployNotAllowed)),
           "Expected DeployNotAllowed error, got: {:?}", err
       );
   }

4. Code Style in crates/ev-revm/src/deploy.rs:68-81:
   The multiple early returns could be simplified for better readability:

   pub fn check_deploy_allowed(
       settings: Option<&DeployAllowlistSettings>,
       caller: Address,
       is_top_level_create: bool,
       block_number: u64,
   ) -> Result<(), DeployCheckError> {
       if let (true, Some(settings)) = (is_top_level_create, settings) {
           if settings.is_active(block_number) && !settings.is_allowed(caller) {
               return Err(DeployCheckError::NotAllowed);
           }
       }
       Ok(())
   }

🔒 Security Assessment

Strengths:

• Fixes critical security bypass where allowlist could be circumvented
• Consistent enforcement across both execution paths
• Proper error propagation prevents silent failures

No Major Security Issues Found - The implementation correctly enforces the deploy allowlist policy.

⚡ Performance Considerations

• Block number lookup: Single database query per transaction validation - acceptable overhead
• Allowlist lookup: O(log n) binary search - efficient for reasonable allowlist sizes
• Overall performance impact is minimal and justified for security

🧪 Test Coverage

Good:

• New test case evnode_create_rejected_when_not_allowlisted validates the fix
• Existing test coverage for related functionality

Could be improved:

• Test for Ethereum (non-EvNode) transaction allowlist enforcement
• Test for edge cases like empty allowlist behavior
• Test for activation height boundary conditions

📋 Summary

This PR successfully addresses the reported security bug. The code quality is good with appropriate error handling and integration points. The main issues are minor improvements to test assertions and potential code style enhancements.

Recommendation: ✅ Approve after addressing the test assertion weakness for better debugging.

---

[gemini-code-assist]

Summary of Changes

Hello @chatton, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug in the permissioned EVM where contract deployment allowlist checks were inconsistently applied. Previously, these checks were bypassed when gas values were directly specified, creating a potential loophole. The changes centralize the allowlist validation logic into a dedicated function and integrate it across both the EVM execution handler and the transaction pool validator. This ensures that all contract creation attempts are now properly vetted against the configured allowlist, significantly enhancing the security and integrity of contract deployments within the EVM environment.

Highlights

• Bug Fix: Corrected a bug where the deploy allowlist check was only applied during gas estimation and not when gas values were directly provided, leading to inconsistent enforcement of contract deployment policies.
• Refactoring: Introduced a new DeployCheckError enum and a check_deploy_allowed function to centralize and encapsulate the contract deployment allowlist logic, improving code reusability and maintainability.
• Integration: Integrated the check_deploy_allowed function into the EVM handler (crates/ev-revm/src/handler.rs) and the transaction pool validator (crates/node/src/txpool.rs) to ensure consistent application of the allowlist policy across all contract creation attempts.
• Error Handling: Added a new error type, EvTxPoolError::DeployNotAllowed, to explicitly handle transactions that violate the deployment allowlist policy within the transaction pool.
• Testing: Implemented new unit tests to verify the correct rejection of non-allowlisted contract deployments in the transaction pool, ensuring the fix behaves as expected.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• crates/ev-revm/src/deploy.rs
  • Defined DeployCheckError enum for allowlist failures.
  • Implemented check_deploy_allowed function to encapsulate allowlist validation.
• crates/ev-revm/src/handler.rs
  • Refactored validate_tx_env to utilize the new check_deploy_allowed function.
• crates/node/src/txpool.rs
  • Imported BlockNumReader trait.
  • Added DeployNotAllowed variant to EvTxPoolError and updated is_bad_transaction logic.
  • Modified EvTransactionValidator to store deploy_allowlist settings and updated its constructor.
  • Introduced a unified deploy allowlist check within validate_evnode for both Ethereum and EvNode transactions.
  • Updated TransactionValidator trait bounds for EvTransactionValidator.
  • Enhanced EvolvePoolBuilder to load and pass deploy allowlist settings from the chain specification to the transaction validator.
  • Added a new test helper create_non_sponsored_evnode_create_tx.
  • Updated create_test_validator to accept an optional deploy_allowlist.
  • Modified existing tests to correctly use the updated create_test_validator signature.
  • Added a new test case evnode_create_rejected_when_not_allowlisted to validate allowlist enforcement.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a bug where the deploy allowlist check was not consistently applied. The introduction of a centralized check_deploy_allowed function and its application in both transaction pool validation and execution is a solid approach. My review includes suggestions to improve code clarity, enhance error handling to prevent silent failures, and strengthen a test assertion. I also found a high-severity issue where failing to fetch the block number could lead to incorrect allowlist enforcement.

[tac0turtle]

look good, gemini and claude reviews have valid feedback. we should integrate