Uh oh!
There was an error while loading. Please reload this page.
fix(tempo): reject TransferWithMemo logs bound to a different challenge - #22
Open
ygd58 wants to merge 1 commit into
Open
fix(tempo): reject TransferWithMemo logs bound to a different challenge#22ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
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. Fixestempoxyz/mpp-tools#112 (AGR-2026-036)
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
matchTransferLogsaccepted anyTransferWithMemo(or plainTransfer) log that matched currency/recipient/sender/amount, without ever inspecting the memo content. Canonicalmppxandmpp-goboth require, when aTransferWithMemolog carries the MPP attribution memo layout, that its challenge nonce is bound to the specific challenge being verified (serverFingerprint = 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.Flagged by the cross-SDK audit as AGR-2026-036. See that thread for the design discussion on scope before I wrote this.
Fix
Adds
Attribution.java, ported fromwevm/mppx'ssrc/tempo/Attribution.ts(and matchingtempoxyz/mpp-go'spkg/tempo/attribution.go, which is why Go was clean in the audit) using BouncyCastle'sKeccak.Digest256— already a main dependency here viaTempoRelay.java, so no new dependency needed.Scope (discussed on #112 first): this only tightens
TransferWithMemohandling. A memo that doesn't use the MPP attribution layout at all (an application-defined memo, or no memo/plainTransfer) 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-Transferverification. A follow-up to require attribution memos unconditionally (matching canonical exactly) would be a separate, deliberately-breaking change if that's wanted later.Testing
AttributionTest: direct coverage of the byte-layout verification (bound/unbound realm, bound/unbound challenge, non-attribution memo, malformed input never throws — always returnsfalserather than propagating an exception).TempoChargeIntentTest: updates the existingtransferWithMemoTopicAcceptedtest to use a properly bound memo (it previously used an arbitrary unbound value and only passed because binding wasn't checked at all — this was masking the exact bug this PR fixes), 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 #21 and #18, I couldn't 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, happy to fix anything that doesn't match.Fixestempoxyz/mpp-tools#112