Validate Tempo transactions before broadcast - #18
Conversation
|
|
7abf806 to
c85d687
Compare
|
Reviewed the RLP decoder here against the RLP spec (all four prefix cases — single byte, short/long string, short/long list — look correct) and the ERC-20 |
c85d687 to
93bec90
Compare
|
Hi! Just following up on this. This addresses AGR-2026-037 / mpp-tools#113, which is marked high severity. I've updated the branch and verified the changes locally. The GitHub Actions runs appear to be waiting for approval. Happy to make any changes needed. Thanks! |
|
Checked the diff since my last review — the RLP decode primitives and ERC-20 selector/offset extraction I verified are unchanged. The force-push only tightens the outer structural check (list item count now requires exactly 14, was "at least 5") and adds an explicit 65-byte signature-length check. Both are strict improvements, not regressions — still looks correct to me. Also flagging since I have two open PRs (#21, #22) touching this same file: #22 (TransferWithMemo challenge binding, in matchTransferLogs) shouldn't conflict with this one — this PR's changes are all in verifyTransaction/the RLP decode path, before matchTransferLogs runs, so they're in different regions of the file. #21 is in StripeApi.java, unrelated. Just noting it in case ordering matters for whoever merges these. |
matchTransferLogs accepted any TransferWithMemo (or plain Transfer) log that matched currency/recipient/sender/amount, without ever inspecting the memo content. Canonical mppx and mpp-go both require, when a TransferWithMemo log carries the MPP attribution memo layout, that its challenge nonce is bound to the specific challenge being verified (server fingerprint = keccak256(realm)[0..9], nonce = keccak256(challengeId)[0..6]). This SDK skipped that check entirely, so a transaction whose attribution memo was minted for a *different* challenge (or realm) — but which otherwise matches this challenge's payment terms exactly — was accepted as satisfying this challenge. Adds Attribution.java, ported from wevm/mppx's src/tempo/Attribution.ts (and matching tempoxyz/mpp-go's pkg/tempo/attribution.go, which is why Go was clean in the audit) using BouncyCastle's Keccak.Digest256 — already a main dependency here via TempoRelay.java, so no new dependency needed. Scope, per the discussion on the linked issue: this only tightens TransferWithMemo handling. A memo that doesn't use the MPP attribution layout at all (an application-defined memo, or no memo/plain Transfer) is left untouched, so existing non-memo payment flows keep working unchanged — this closes the replay path without a breaking change to currently-supported plain-Transfer verification. Adds: - AttributionTest: direct coverage of the byte-layout verification (bound/unbound realm, bound/unbound challenge, non-attribution memo, malformed input never throws). - TempoChargeIntentTest: updates the existing transferWithMemoTopicAccepted test to use a properly bound memo (previously used an arbitrary unbound value and only happened to pass because binding wasn't checked at all), adds regression tests for memos bound to a different challenge and a different realm, and a test confirming the plain Transfer (no memo) flow is unaffected. As with stripe#21 and stripe#18, I could not compile/run this against the real com.stripe:stripe-java / bouncycastle dependencies in my sandbox (no Maven Central access) — please double-check compilation before merge. Fixes tempoxyz/mpp-tools#112 (AGR-2026-036)
2 participants
Summary
Motivation
Addresses tempoxyz/mpp-tools#113: a pull transaction whose payment call does not satisfy the challenge should not be irreversibly broadcast before validation fails.
Testing
./gradlew --no-daemon test— passed on Java 11.0.32./gradlew --no-daemon test— passed on Java 17.0.20./gradlew --no-daemon test— passed on Java 21.0.12./gradlew --no-daemon jar— passed on Java 17.0.20./gradlew integrationTest— not run locally; Docker is not installed/available on PATH, and the repo workflow starts the required Tempo node with Docker Compose