trade execute: bind EVM receipt confirmation to the locally-derived tx hash - #518
trade execute: bind EVM receipt confirmation to the locally-derived tx hash#518kome12 wants to merge 1 commit into
Found 1 finding within acceptable thresholds
Review Status
✅ Passed
Findings
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
Review effort: 3/5 (Moderate)
Details
This PR adds broadcaster-integrity checking to the EVM trade execute flow: transaction receipts are now confirmed against a locally-derived keccak256 hash of the signed bytes rather than whatever hash a potentially-compromised broadcaster reports. The design is sound, the fail-closed semantics are correct, and the gasless carve-out is properly scoped. Test coverage is thorough (known-vector unit tests, round-trip signing tests, a fail-closed mismatch test, a gasless regression test, and updates to all existing EVM execute mocks). No bugs or security issues were found.
Findings (1 medium)
src/trading.js — Success-log hash source (medium)
In the Privy signing path, after confirmEvmBroadcast succeeds, the confirmation log line still prints the broadcaster's reported hash rather than the locally-derived one:
// Line 2314 (revoke) and 2373 (approval):
const receipt = await confirmEvmBroadcast(chain, signedRevoke, revokeResult.txHash);
log(` ✓ Allowance revoked in block ${parseInt(receipt.blockNumber, 16)}: ${revokeResult.txHash}`);
// ^^^ broadcaster hashWhen the hashes agree (the normal success path), both values are identical, so this is never wrong. However, consider that confirmEvmBroadcast always polls on localHash, so the hash that was actually used to confirm on-chain is evmTxHash(signedRevoke) — not revokeResult.txHash. If you ever want to remove the equality-check requirement (e.g. tolerate a normalized vs. non-normalized hash form), you would silently show the wrong hash in the log. It's a minor consistency gap.
Suggested fix: derive the local hash once and use it for both the equality check and the success log:
// After calling confirmEvmBroadcast (which already computed the local hash internally):
const localHash = evmTxHash(signedRevoke);
const receipt = await confirmEvmBroadcast(chain, signedRevoke, revokeResult.txHash);
log(` ✓ Allowance revoked in block ${parseInt(receipt.blockNumber, 16)}: ${localHash}`);Alternatively, have confirmEvmBroadcast return the local hash alongside the receipt so callers don't need to recompute it.
This affects lines 2314, 2373 (Privy revoke/approval), 2978, 3030 (local-key revoke/approval), and the analogous log at 3030.
Note: Claude suggested: APPROVE_WITH_COMMENTS. Final status determined by severity thresholds.