h-22 fix: reject unsupported intent assets before fill simulation#375
h-22 fix: reject unsupported intent assets before fill simulation#375nahimterrazas wants to merge 2 commits into
Conversation
nahimterrazas
requested review from
pepebndc,
shahnami and
tirumerla
as code owners
📝 WalkthroughWalkthroughThis PR adds a pre-fill simulation validation gate to the cost-profit service and integrates it into the intent handler. The new ChangesPre-fill validation and integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
pepebndc
left a comment
pepebndc
left a comment
There was a problem hiding this comment.
Validated against scan finding H-22 — fully fixed.
New CostProfitService::validate_before_fill_simulation runs in IntentHandler::handle immediately before generate_fill_transaction/simulate_callback_and_estimate_gas, performing only cheap local checks (token allowlist via in-memory get_token_info, callback calldata hex/size cap, callback-whitelist policy) and emitting OrderEvent::Skipped on failure — so unsupported assets never reach eth_estimateGas. Tests assert generate_fill_transaction.times(0).
Minor follow-up (out of scope, default-off): the quote-path try_live_fill_estimate lacks an equivalent pre-check and fails open on token lookup, but is gated behind live_fill_estimate_enabled=false. Independently verified. LGTM.
…ects-before-estimate
![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
🤖 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/solver-core/src/engine/cost_profit.rs`:
- Around line 1261-1281: The code currently treats an empty callback whitelist
as deny-all because is_whitelisted uses any(...) over callback_whitelist; change
the logic to short-circuit: if config.order.callback_whitelist.is_empty() then
treat the recipient as allowed, otherwise perform the existing comparison
against recipient_interop_hex (keep the existing CostProfitError path when not
allowed). Apply the same helper/logic in simulate_callback_and_estimate_gas so
both paths use the identical check (i.e., centralize the "is recipient allowed"
logic and call it from where recipient_interop_hex, output_chain_id,
recipient_eth_address and is_whitelisted are used).
🪄 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: 1a695f41-1665-40b1-8585-9313b44681cd
📒 Files selected for processing (2)
crates/solver-core/src/engine/cost_profit.rscrates/solver-core/src/handlers/intent.rs
| let recipient_interop_hex = output.receiver.to_hex().to_lowercase(); | ||
| let output_chain_id = output.receiver.ethereum_chain_id().map_err(|e| { | ||
| CostProfitError::Config(format!("Failed to extract chain ID from recipient: {e}")) | ||
| })?; | ||
| let recipient_eth_address = output | ||
| .receiver | ||
| .ethereum_address() | ||
| .map(|addr| format!("0x{}", alloy_primitives::hex::encode(addr))) | ||
| .unwrap_or_else(|_| "unknown".to_string()); | ||
|
|
||
| let is_whitelisted = config | ||
| .order | ||
| .callback_whitelist | ||
| .iter() | ||
| .any(|entry| entry.to_lowercase() == recipient_interop_hex); | ||
|
|
||
| if !is_whitelisted { | ||
| return Err(CostProfitError::Config(format!( | ||
| "Callback recipient {recipient_eth_address} on chain {output_chain_id} not in whitelist. Add '{recipient_interop_hex}' to order.callback_whitelist in config (EIP-7930 format)" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Preserve the repo's "empty whitelist = allow all" contract.
This branch rejects every callback when config.order.callback_whitelist is empty, because any(...) returns false on []. That diverges from crates/solver-service/src/apis/quote/validation.rs, which explicitly treats an empty whitelist as allow-all, so an order can pass quote-time validation and then be skipped here. Please short-circuit on callback_whitelist.is_empty() before the whitelist comparison, and have simulate_callback_and_estimate_gas reuse the same helper so the later path does not keep the old semantics.
Suggested fix
if !config.order.simulate_callbacks {
return Err(CostProfitError::Config(
"Order has callback data but callback simulation is disabled. \
Callbacks are not supported when simulate_callbacks = false in config."
.to_string(),
));
}
+
+ if config.order.callback_whitelist.is_empty() {
+ continue;
+ }
let recipient_interop_hex = output.receiver.to_hex().to_lowercase();
let output_chain_id = output.receiver.ethereum_chain_id().map_err(|e| {
CostProfitError::Config(format!("Failed to extract chain ID from recipient: {e}"))
})?;🤖 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/solver-core/src/engine/cost_profit.rs` around lines 1261 - 1281, The
code currently treats an empty callback whitelist as deny-all because
is_whitelisted uses any(...) over callback_whitelist; change the logic to
short-circuit: if config.order.callback_whitelist.is_empty() then treat the
recipient as allowed, otherwise perform the existing comparison against
recipient_interop_hex (keep the existing CostProfitError path when not allowed).
Apply the same helper/logic in simulate_callback_and_estimate_gas so both paths
use the identical check (i.e., centralize the "is recipient allowed" logic and
call it from where recipient_interop_hex, output_chain_id, recipient_eth_address
and is_whitelisted are used).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary
Testing Process
Checklist
Summary by CodeRabbit
New Features
Tests