Skip to content

[ANCHOR-1323]: Stop SEP-24 withdrawal memos from colliding across protocols - #2096

Open
ceciliaromao wants to merge 11 commits into
developfrom
fix/anchor-1323
Open

ceciliaromao wants to merge 11 commits into
developfrom
fix/anchor-1323

Conversation

@ceciliaromao

Copy link
Copy Markdown
Collaborator

Description

  • SEP-24 withdrawals are now routed by a memo the server mints. Sep24DepositInfoSelfGenerator always returns a generated id memo, as the SEP-6 and SEP-31 generators already do, and Sep24Service.withdraw still validates the request's memo/memo_type but no longer stores them, so the row holds no memo until request_onchain_funds assigns one.
  • request_onchain_funds refuses a memo (and account) already held by a pending SEP-6, SEP-24 or SEP-31 transaction, with the existing memo_in_use message. The check lives in the application, so it also holds without Flyway/V28. The three store interfaces expose the pending-memo lookup it needs.
  • DefaultPaymentListener looks up SEP-31, SEP-24 and SEP-6 before handling any of them. When one payment matches pending rows of more than one protocol it credits none, logs the matched ids and increments PAYMENT_OBSERVER_AMBIGUOUS_ROUTING with sep=cross. Same-protocol ambiguity keeps its 24/6/31 tags.
  • Behavior change: a wallet that pays a SEP-24 withdrawal with a memo it chose, instead of the withdraw_memo the anchor returns, is no longer credited. The contract-account e2e test is changed to pay the returned memo. Needs a release note.

Context

  • HackerOne 4021554
  • Duplicates covered by the same change: 4034621, 4164098, 4169935
  • SEP-24 let the client choose the routing memo, while the three protocols receive on one distribution account and the memo uniqueness was enforced one table at a time (V28, only where Flyway runs). A memo copied from another customer's pending SEP-6 withdrawal made the observer attach that customer's payment to the SEP-24 row. Making the memo server-side removes the primitive; the application check and the router guard keep a collision from being created or credited, including for rows that already collide when this is deployed.

Testing

  • ./gradlew test
  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.service.Sep24DepositInfoSelfGeneratorTest" (client id memo replaced, minted per call and in range, text/hash/absent types)
  • Unit: ./gradlew :core:test --tests "org.stellar.anchor.sep24.Sep24ServiceTest" (withdraw stores no memo of any type, deposit memo unchanged, invalid memo still 400)
  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.rpc.RequestOnchainFundsHandlerTest" (every protocol pair refused, own row and non-pending rows accepted, refusal leaves the row unsaved, business-server memo canonicalised, no database constraint needed)
  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.DefaultPaymentListenerTest" (cross-protocol refusal for 24+6, 31+24, 31+6 and muxed ids, the reported scenario, the control case, single match, same-protocol tags, either row created first)
  • Integration: ./gradlew runEssentialTests; Sep24End2EndTests test contract account withdraw now pays the returned withdraw_memo and asserts it differs from the memo the wallet asked for. Run in CI (essential_tests), not run locally.
  • Manual: not run. The reporters' scenarios are covered at unit level (a SEP-24 row declaring a pending SEP-6 memo no longer receives the payment).

Documentation

  • N/A: no new feature and no documentation change in this repository.
  • Release note needed for the behavior change above (wallets must pay the withdraw_memo the anchor reports; rows already pending keep their memo).

Known limitations

  • No e2e case for the attack itself: it needs a SEP-6 withdrawal with KYC and a second wallet on the docker stack and was not run before pushing. The scenario is proven at the router (DefaultPaymentListenerTest) and by the server-minted memo.
  • No cross-table database constraint; V28 stays per table. The application check is read-then-write, so two concurrent assignments of the same business-server memo (deposit_info_generator_type: none) can still race. Server-minted memos collide with probability 2^-63.
  • Expired or terminal rows keep their routing memo and are not counted as holders; with server-minted memos they cannot be reused without guessing. Pending rows still do not expire on their own.
  • The assignment check uses the exact (account, memo) lookup; the muxed-id second lookup is applied only by the router guard.
  • A SEP-31 payment now costs two extra indexed lookups, because SEP-31, SEP-24 and SEP-6 are always looked up together.
  • The legacy PATCH /transactions path still assigns memos without this check; it is out of scope here.

@ceciliaromao ceciliaromao self-assigned this Nov 5, 2026
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.

1 participant