fix: address MorphTx audit findings#134
Open
panos-xyz wants to merge 19 commits into
Open
Conversation
Align legacy MorphTx version routing with geth so zero-prefixed payloads enter the V0 decoder instead of failing as unsupported versions.
Apply the EIP-1559 fee-cap checks to MorphTx regardless of fee asset so token-fee blocks cannot diverge from geth validation.
Build fee-token-only MorphTx requests as V0 while reserving V1 for reference or memo fields, matching geth RPC defaults.
Return an explicit RPC construction error when legacy gasPrice is mixed with Morph fields instead of silently changing fee semantics.
Allow V0 MorphTx requests with an all-zero reference so RPC version inference and validation match geth normalization.
Revalidate pooled MorphTx gas limits against the latest block gas limit so stale over-limit transactions are removed with their descendants.
Use max_fee_per_gas for token-fee MorphTx pool admission so mempool validation matches geth's conservative affordability check.
Fill missing MorphTx request chain IDs from the EVM environment on tx-env conversion so simulation paths do not depend on implicit alloy defaults.
Reject conflicting data/input fields and empty contract creation in MorphTx RPC construction to match geth request validation.
Point the Bernoulli precompile comment at the matching geth contract map instead of the Morph203 section.
Run the same MorphTx structural validation on tx-env conversion so eth_call and estimate paths reject requests that real construction would reject.
Treat failed committed-storage reads during Morph SLOAD/SSTORE original-value restoration as fatal instead of continuing with uncertain gas accounting state.
Apply rustfmt to earlier audit fixes so the branch remains format-clean.
Use reth's latest sealed header lookup so canonical head number, hash, and timestamp come from the same provider snapshot.
This reverts commit dd98bc9.
This reverts commit 0b761eb.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMultiple MorphTx correctness fixes across the stack: V0 ChangesMorphTx versioning, fee validation, and txpool hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rpc/src/eth/transaction.rs (1)
108-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize zero references before storing them.
is_nonzero_referenceandmorph_tx_versiontreatB256::ZEROas absent, buttx_env.referenceandTxMorph.referencecan still storeSome(B256::ZERO). That leaves V0 MorphTxs with an in-memory/RPC reference that their V0 encoding/signing omits, which can break reference indexing and response parity.Proposed canonicalization
@@ - let reference = self.reference; + let reference = self.reference.filter(|reference| *reference != B256::ZERO); @@ fn try_build_morph_tx_from_request( req: &alloy_rpc_types_eth::TransactionRequest, fee_token_id: U64, fee_limit: U256, reference: Option<alloy_primitives::B256>, memo: Option<alloy_primitives::Bytes>, ) -> Result<Option<TxMorph>, &'static str> { + let reference = reference.filter(|reference| *reference != B256::ZERO); let fee_token_id_u16 = u16::try_from(fee_token_id.to::<u64>()).map_err(|_| "invalid token")?;Also applies to: 183-195, 230-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rpc/src/eth/transaction.rs` around lines 108 - 136, The reference value is being stored in tx_env.reference without normalizing zero values. Since is_nonzero_reference and morph_tx_version treat B256::ZERO as absent, any B256::ZERO value should be converted to None before assignment. Modify the code where tx_env.reference is set to normalize the reference value by checking if it is Some(B256::ZERO) and converting it to None. Apply the same normalization pattern to the other locations mentioned in the comment where reference is being stored or used (around lines 183-195 and 230-231).crates/primitives/src/transaction/morph_transaction.rs (1)
393-400:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the zero-byte V0 route to the primary decode paths too.
Line 393 only updates
decode_fields, but this file documentsrlp_decode_with_signatureas the primary signed decode path, and both it andDecodable::decodestill return"unsupported morph tx version"forfirst_byte == 0. A zero-prefixed 2718/signed payload will miss the new V0/RLP-level error behavior.Proposed consistency fix
@@ - } else if first_byte >= 0xC0 { + } else if first_byte == MORPH_TX_VERSION_0 || first_byte >= 0xC0 { MORPH_TX_VERSION_0 } else { return Err(alloy_rlp::Error::Custom("unsupported morph tx version")); @@ - } else if first_byte < 0xC0 { + } else if first_byte != MORPH_TX_VERSION_0 && first_byte < 0xC0 { // Invalid: not a version we support and not an RLP list return Err(alloy_rlp::Error::Custom("unsupported morph tx version")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/primitives/src/transaction/morph_transaction.rs` around lines 393 - 400, The zero-byte V0 route fix has been applied to decode_fields but not to the other primary decode paths. Apply the same zero-byte V0 handling logic (checking if first_byte == 0 to treat it as V0 format with direct RLP decode) to the rlp_decode_with_signature method and the Decodable::decode trait implementation, ensuring they handle first_byte == 0 consistently by routing to the appropriate V0 decoding path instead of returning "unsupported morph tx version" error.
🧹 Nitpick comments (1)
crates/txpool/src/morph_tx_validation.rs (1)
135-139: Thebase_fee_per_gasfield inMorphTxValidationInputis no longer referenced in validation logic.After this change,
token_gas_feeis set directly togas_fee(which usesmax_fee_per_gas), andbase_fee_per_gasis not read withinvalidate_morph_tx. The field is still populated by callers (validator.rsline 448 andmaintain.rsline 261), but it serves no purpose in the current validation logic. Consider removing it from the struct and its call sites if it's no longer needed elsewhere, or add a comment explaining it's retained for future use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/txpool/src/morph_tx_validation.rs` around lines 135 - 139, The `base_fee_per_gas` field in the `MorphTxValidationInput` struct is no longer being read in the `validate_morph_tx` function, as `token_gas_fee` is now set directly to `gas_fee`. Either remove the `base_fee_per_gas` field from the `MorphTxValidationInput` struct and its assignments in the call sites (validator.rs and maintain.rs), or if this field is needed for future use, add an explanatory comment to the struct definition clarifying why it is retained despite not being currently used in the validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 129-137: The try_into_tx_env method currently lacks validation to
reject gas_price when combined with Morph fields, creating inconsistency with
try_build_morph_tx_from_request which performs this check. Add validation in
try_into_tx_env that rejects requests combining legacy gas_price with
Morph-specific fields (fee_token_id, reference, or memo). This check should
occur before the is_morph_tx detection logic to ensure eth_call and
eth_estimateGas reject the same invalid shapes that signing and simulation
reject.
---
Outside diff comments:
In `@crates/primitives/src/transaction/morph_transaction.rs`:
- Around line 393-400: The zero-byte V0 route fix has been applied to
decode_fields but not to the other primary decode paths. Apply the same
zero-byte V0 handling logic (checking if first_byte == 0 to treat it as V0
format with direct RLP decode) to the rlp_decode_with_signature method and the
Decodable::decode trait implementation, ensuring they handle first_byte == 0
consistently by routing to the appropriate V0 decoding path instead of returning
"unsupported morph tx version" error.
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 108-136: The reference value is being stored in tx_env.reference
without normalizing zero values. Since is_nonzero_reference and morph_tx_version
treat B256::ZERO as absent, any B256::ZERO value should be converted to None
before assignment. Modify the code where tx_env.reference is set to normalize
the reference value by checking if it is Some(B256::ZERO) and converting it to
None. Apply the same normalization pattern to the other locations mentioned in
the comment where reference is being stored or used (around lines 183-195 and
230-231).
---
Nitpick comments:
In `@crates/txpool/src/morph_tx_validation.rs`:
- Around line 135-139: The `base_fee_per_gas` field in the
`MorphTxValidationInput` struct is no longer being read in the
`validate_morph_tx` function, as `token_gas_fee` is now set directly to
`gas_fee`. Either remove the `base_fee_per_gas` field from the
`MorphTxValidationInput` struct and its assignments in the call sites
(validator.rs and maintain.rs), or if this field is needed for future use, add
an explanatory comment to the struct definition clarifying why it is retained
despite not being currently used in the validation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02bfda11-83f4-4ebf-82b8-96ea88697717
📒 Files selected for processing (8)
crates/engine-api/src/builder.rscrates/primitives/src/transaction/morph_transaction.rscrates/revm/src/handler.rscrates/revm/src/precompiles.rscrates/rpc/src/eth/transaction.rscrates/txpool/src/maintain.rscrates/txpool/src/morph_tx_validation.rscrates/txpool/src/validator.rs
panos-xyz
changed the title
Align MorphTx simulation validation with signing, canonicalize zero references, extend zero-byte V0 decode routing, and remove unused txpool validation input.
Treat requests that include legacy gas_price as standard transactions even when Morph-specific fields are present, matching geth's transaction type precedence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
0x00payloads across decoding and validation flows.datavsinput.