Skip to content

feat: small defense in depth check for the evm inspector - #3781

Merged
gilcu3 merged 4 commits into
mainfrom
3681-bind-evm-inspector-receipt-and-log-to-the-requested-transaction
Jul 13, 2026
Merged

feat: small defense in depth check for the evm inspector#3781
gilcu3 merged 4 commits into
mainfrom
3681-bind-evm-inspector-receipt-and-log-to-the-requested-transaction

Conversation

@gilcu3

@gilcu3 gilcu3 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Closes #3681

@gilcu3 gilcu3 linked an issue Jul 13, 2026 that may be closed by this pull request
@gilcu3
gilcu3 marked this pull request as ready for review July 13, 2026 06:39
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Pull request overview

Adds a small defense-in-depth layer to the EVM inspector: the receipt returned by eth_getTransactionReceipt must echo the requested transaction hash, and any log selected by index must be internally bound to that receipt (matching tx hash, block hash, and block height). Closes 3681. Only guards against inconsistent/tampered RPC responses — a fully Byzantine provider returning internally consistent forged data still passes (out of scope, per the linked issue).

Changes:

  • Add transaction_hash: H256 to GetTransactionReceiptResponse (deserialized from transactionHash).
  • In EvmInspector::extract, reject a receipt whose transaction_hash differs from the requested hash (reusing InconsistentRpcResponse).
  • In EvmExtractor::extract_value for Log, verify log.transaction_hash == receipt.transaction_hash, log.block_hash == receipt.block_hash, and log.block_number == receipt.block_number; return a new LogNotBoundToReceipt error variant otherwise.
  • Add four new unit tests for the mismatch paths and backfill transaction_hash on every existing test fixture.

Reviewed changes

Per-file summary
File Description
crates/foreign-chain-rpc-interfaces/src/evm.rs Adds transaction_hash: H256 field to GetTransactionReceiptResponse.
crates/foreign-chain-inspector/src/evm/inspector.rs Adds the receipt-hash check in extract and the log-to-receipt binding check in EvmExtractor::extract_value.
crates/foreign-chain-inspector/src/lib.rs Rewords InconsistentRpcResponse message; adds new LogNotBoundToReceipt error variant.
crates/foreign-chain-inspector/tests/evm_inspector.rs Enables non_snake_case, backfills transaction_hash on existing fixtures, adds four new tests plus an extract_log_from_receipt_with helper.

Findings

Non-blocking (nits, follow-ups, suggestions):

  • crates/foreign-chain-rpc-interfaces/src/evm.rs:14transaction_hash is now a required field on GetTransactionReceiptResponse. Any Ethereum-compatible RPC provider currently returns transactionHash, so this is safe in practice, but note that this is a wire-format breaking change: a previously-serialized GetTransactionReceiptResponse (e.g. cached JSON without the field) would now fail to deserialize. No such consumers were found in-tree, so this is a heads-up rather than a blocker.
  • crates/foreign-chain-inspector/src/evm/inspector.rs:204-206 — The check omits log.removed and log.transaction_index. removed should be false after the canonical/finality checks (side-block reorg), but explicitly rejecting log.removed == true would be a cheap and mirrored defense. transaction_index is transitively bound by transaction_hash, so leaving it unchecked is fine.
  • crates/foreign-chain-inspector/src/evm/inspector.rs:201-203 — Comment states the transitive binding argument, which is genuinely useful "why" content. Consider adding one line noting that the receipt has also had its block hash verified as canonical (verify_block_is_canonical), so the log block-hash check transitively pins the log to a canonical block too. Not required.
  • crates/foreign-chain-inspector/src/evm/inspector.rs:67-68 — Small phrasing nit: the comment describes the "well-behaved backend" case, but the actual value of the check is catching misbehaving backends. Consider rephrasing as "a well-behaved backend always echoes the hash we queried, so a mismatch means the backend is buggy or dishonest".
  • crates/foreign-chain-inspector/src/starknet/inspector.rs:38-47 — Original issue 3681 notes "the Bitcoin inspector already echoes back and checks the queried hash; the EVM path (and similarly Starknet) does not." Starknet remains unbound after this PR. If that is intentionally deferred, consider filing a follow-up issue and linking it in the PR description; otherwise it is a scope gap.
  • crates/foreign-chain-inspector/tests/evm_inspector.rs:566-604 — New tests follow the required <sut>__should_<assertion> convention with // Given / When / Then sections, but the file mixes them with older extract_returns_X_when_Y tests. Not a blocker (existing tests predate the convention), just worth flagging that future work in this file should keep the new convention.

✅ Approved

@haiyuechen-nearone

Copy link
Copy Markdown
Contributor

Should we add this guard for Starknet as well?

@gilcu3

gilcu3 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Should we add this guard for Starknet as well?

I thought about it but it seems not simple for Starknet, as the receipts have a different structure (but I do not understand the details very well, as this is Starknet specific stuff)

@haiyuechen-nearone

Copy link
Copy Markdown
Contributor

@haiyuechen-nearone haiyuechen-nearone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starknet tx hash check can be added as a follow up, I think this is good to go.

anodar
anodar previously approved these changes Jul 13, 2026

@anodar anodar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Dropped some optional nits.

Comment thread crates/foreign-chain-inspector/tests/evm_inspector.rs
Comment thread crates/foreign-chain-inspector/tests/evm_inspector.rs Outdated
@gilcu3
gilcu3 dismissed stale reviews from anodar and haiyuechen-nearone via 578763d July 13, 2026 10:53
…or-receipt-and-log-to-the-requested-transaction

@anodar anodar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@gilcu3
gilcu3 enabled auto-merge July 13, 2026 11:10
@gilcu3
gilcu3 added this pull request to the merge queue Jul 13, 2026
Merged via the queue into main with commit e0ec710 Jul 13, 2026
27 of 28 checks passed
@gilcu3
gilcu3 deleted the 3681-bind-evm-inspector-receipt-and-log-to-the-requested-transaction branch July 13, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bind EVM inspector receipt and log to the requested transaction

3 participants