feat: Implement validation for cross-chain output oracles #307
Open
nahimterrazas wants to merge 2 commits intomainfrom
Open
feat: Implement validation for cross-chain output oracles #307nahimterrazas wants to merge 2 commits intomainfrom
nahimterrazas wants to merge 2 commits intomainfrom
Conversation
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughAdded defense-in-depth oracle validations for cross-chain outputs in EIP-7683 order handling: reject zero oracles, detect non-zero upper 12 bytes, and require lower-20-byte oracle address matches supported destinations across fill, claim, and validate flows; expanded tests and canonicalized oracle fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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
🧹 Nitpick comments (1)
crates/solver-order/src/implementations/standards/_7683.rs (1)
655-659: Useoutput_indexin all output-route error branches for consistent diagnostics.You already enumerate outputs; including index in unsupported-route/incompatible-oracle errors will make multi-output debugging much easier.
Also applies to: 686-690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-order/src/implementations/standards/_7683.rs` around lines 655 - 659, The error messages for unsupported routes and incompatible-oracle branches currently reference origin_chain, dest_chain and input_oracle but omit the output index; update the two error branches that return OrderError::ValidationFailed (the branch checking supported_destinations.contains(&dest_chain) and the similar branch around lines 686-690) to include the output_index variable in the formatted message (e.g. "output {output_index}: route from chain {origin_chain} to chain {dest_chain} ..." and similarly for the incompatible-oracle message) so diagnostics consistently show which output failed; keep the existing context (input_oracle, origin_chain, dest_chain) and insert output_index into the format strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/solver-order/src/implementations/standards/_7683.rs`:
- Around line 317-329: The current validation only checks that cross-chain
output oracles are non-zero and have zero upper bytes but does not verify the
oracle is actually supported for the destination chain; add a route-membership
check after the existing canonical checks to ensure the oracle corresponds to a
known/registered route for output.chain_id (e.g. call a routing lookup like
is_oracle_valid_for_chain(output.oracle, output.chain_id) or consult the
RouteRegistry/AllowedOracles for that chain), and return
OrderError::ValidationFailed with a clear message ("unsupported oracle for
destination chain {chain_id}") if the oracle is not a member; apply the same
additional check in the other analogous block (the fill/claim validation at the
other location) so both places enforce route membership.
---
Nitpick comments:
In `@crates/solver-order/src/implementations/standards/_7683.rs`:
- Around line 655-659: The error messages for unsupported routes and
incompatible-oracle branches currently reference origin_chain, dest_chain and
input_oracle but omit the output index; update the two error branches that
return OrderError::ValidationFailed (the branch checking
supported_destinations.contains(&dest_chain) and the similar branch around lines
686-690) to include the output_index variable in the formatted message (e.g.
"output {output_index}: route from chain {origin_chain} to chain {dest_chain}
..." and similarly for the incompatible-oracle message) so diagnostics
consistently show which output failed; keep the existing context (input_oracle,
origin_chain, dest_chain) and insert output_index into the format strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0136949f-09f7-4eff-9696-7f23488c105c
📒 Files selected for processing (1)
crates/solver-order/src/implementations/standards/_7683.rs
Comment on lines
+317
to
+329
| // Defense in depth: cross-chain outputs must have a canonical non-zero oracle. | ||
| if output.oracle == [0u8; 32] { | ||
| return Err(OrderError::ValidationFailed(format!( | ||
| "Zero oracle not allowed for cross-chain output on chain {}", | ||
| output.chain_id.to::<u64>() | ||
| ))); | ||
| } | ||
| if output.oracle[..12].iter().any(|&byte| byte != 0) { | ||
| return Err(OrderError::ValidationFailed(format!( | ||
| "Output oracle has dirty upper bytes for cross-chain output on chain {}", | ||
| output.chain_id.to::<u64>() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Add route-membership validation in fill/claim (not just zero/dirty-byte checks).
These blocks validate canonical encoding, but they still accept a canonical unsupported output oracle for a destination chain. That leaves a defense-in-depth gap in transaction generation paths.
Proposed patch direction
@@ async fn generate_fill_transaction(...)
- if output.oracle[..12].iter().any(|&byte| byte != 0) {
+ if output.oracle[..12].iter().any(|&byte| byte != 0) {
return Err(OrderError::ValidationFailed(format!(
"Output oracle has dirty upper bytes for cross-chain output on chain {}",
output.chain_id.to::<u64>()
)));
}
+ let dest_chain_id = output.chain_id.to::<u64>();
+ let output_oracle_address = &output.oracle[12..32];
+ let has_supported_output_oracle = self.oracle_routes.supported_routes.values().any(|outs| {
+ outs.iter().any(|supported| {
+ supported.chain_id == dest_chain_id
+ && supported.oracle.0.as_slice() == output_oracle_address
+ })
+ });
+ if !has_supported_output_oracle {
+ return Err(OrderError::ValidationFailed(format!(
+ "Output oracle is not supported for destination chain {}",
+ dest_chain_id
+ )));
+ }
@@ async fn generate_claim_transaction(...)
if output.oracle[..12].iter().any(|&byte| byte != 0) {
return Err(OrderError::ValidationFailed(format!(
"Output oracle has dirty upper bytes for cross-chain output on chain {}",
output.chain_id.to::<u64>()
)));
}
+ let dest_chain_id = output.chain_id.to::<u64>();
+ let output_oracle_address = &output.oracle[12..32];
+ let has_supported_output_oracle = self.oracle_routes.supported_routes.values().any(|outs| {
+ outs.iter().any(|supported| {
+ supported.chain_id == dest_chain_id
+ && supported.oracle.0.as_slice() == output_oracle_address
+ })
+ });
+ if !has_supported_output_oracle {
+ return Err(OrderError::ValidationFailed(format!(
+ "Output oracle is not supported for destination chain {}",
+ dest_chain_id
+ )));
+ }
}Also applies to: 444-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/solver-order/src/implementations/standards/_7683.rs` around lines 317
- 329, The current validation only checks that cross-chain output oracles are
non-zero and have zero upper bytes but does not verify the oracle is actually
supported for the destination chain; add a route-membership check after the
existing canonical checks to ensure the oracle corresponds to a known/registered
route for output.chain_id (e.g. call a routing lookup like
is_oracle_valid_for_chain(output.oracle, output.chain_id) or consult the
RouteRegistry/AllowedOracles for that chain), and return
OrderError::ValidationFailed with a clear message ("unsupported oracle for
destination chain {chain_id}") if the oracle is not a member; apply the same
additional check in the other analogous block (the fill/claim validation at the
other location) so both places enforce route membership.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ation in Eip7683OrderImpl
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/solver-order/src/implementations/standards/_7683.rs (1)
317-329:⚠️ Potential issue | 🟠 MajorEnforce supported-route membership in fill/claim, not only canonical encoding.
Line 317 and Line 444 only validate canonical bytes32 shape (non-zero + clean upper 12 bytes). A clean but unsupported oracle for the destination chain still passes these tx-generation paths, so the defense-in-depth gap remains.
🔧 Proposed patch direction
@@ async fn generate_fill_transaction( - // Defense in depth: cross-chain outputs must have a canonical non-zero oracle. + // Defense in depth: cross-chain outputs must have a canonical non-zero oracle. + let dest_chain_id = output.chain_id.to::<u64>(); if output.oracle == [0u8; 32] { return Err(OrderError::ValidationFailed(format!( "Zero oracle not allowed for cross-chain output on chain {}", - output.chain_id.to::<u64>() + dest_chain_id ))); } if output.oracle[..12].iter().any(|&byte| byte != 0) { return Err(OrderError::ValidationFailed(format!( "Output oracle has dirty upper bytes for cross-chain output on chain {}", - output.chain_id.to::<u64>() + dest_chain_id ))); } + let output_oracle_address = &output.oracle[12..32]; + let has_supported_output_oracle = + self.oracle_routes.supported_routes.values().any(|outs| { + outs.iter().any(|supported| { + supported.chain_id == dest_chain_id + && supported.oracle.0.as_slice() == output_oracle_address + }) + }); + if !has_supported_output_oracle { + return Err(OrderError::ValidationFailed(format!( + "Output oracle is not supported for destination chain {}", + dest_chain_id + ))); + } @@ - let dest_chain_id = output.chain_id.to::<u64>(); + // dest_chain_id already derived above @@ async fn generate_claim_transaction( if output.oracle[..12].iter().any(|&byte| byte != 0) { return Err(OrderError::ValidationFailed(format!( "Output oracle has dirty upper bytes for cross-chain output on chain {}", output.chain_id.to::<u64>() ))); } + let dest_chain_id = output.chain_id.to::<u64>(); + let output_oracle_address = &output.oracle[12..32]; + let has_supported_output_oracle = + self.oracle_routes.supported_routes.values().any(|outs| { + outs.iter().any(|supported| { + supported.chain_id == dest_chain_id + && supported.oracle.0.as_slice() == output_oracle_address + }) + }); + if !has_supported_output_oracle { + return Err(OrderError::ValidationFailed(format!( + "Output oracle is not supported for destination chain {}", + dest_chain_id + ))); + } }Also applies to: 444-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-order/src/implementations/standards/_7683.rs` around lines 317 - 329, The current checks only assert canonical oracle shape (output.oracle not all-zero and upper 12 bytes zero) but do not ensure the oracle is a supported destination route; update both validation sites that inspect output.oracle (the blocks returning OrderError::ValidationFailed for zero oracle and dirty upper bytes) to also verify membership in the supported-route registry (e.g., call the route table or SupportedRoutes::contains/get for the destination chain_id and/or oracle bytes). If the oracle is not present in the supported routes, return OrderError::ValidationFailed with a clear message including output.chain_id and the oracle identifier; apply this same membership check at the second validation site handling cross-chain outputs so unsupported-but-canonical oracles are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/solver-order/src/implementations/standards/_7683.rs`:
- Around line 317-329: The current checks only assert canonical oracle shape
(output.oracle not all-zero and upper 12 bytes zero) but do not ensure the
oracle is a supported destination route; update both validation sites that
inspect output.oracle (the blocks returning OrderError::ValidationFailed for
zero oracle and dirty upper bytes) to also verify membership in the
supported-route registry (e.g., call the route table or
SupportedRoutes::contains/get for the destination chain_id and/or oracle bytes).
If the oracle is not present in the supported routes, return
OrderError::ValidationFailed with a clear message including output.chain_id and
the oracle identifier; apply this same membership check at the second validation
site handling cross-chain outputs so unsupported-but-canonical oracles are
rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9876b0c-2d7e-4ad9-8e12-b77b00c890ae
📒 Files selected for processing (1)
crates/solver-order/src/implementations/standards/_7683.rs
pepebndc
approved these changes
Mar 6, 2026
shahnami
approved these changes
Mar 9, 2026
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
3 participants
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 by CodeRabbit