Skip to content

[ANCHOR-1233]: SEP-24 withdraw skips the destination-account check its SEP-24 deposit sibling enforces - #1968

Merged
amandagonsalves merged 3 commits into
developfrom
fix/anchor-1233
Jul 8, 2026
Merged

amandagonsalves merged 3 commits into
developfrom
fix/anchor-1233

Conversation

@amandagonsalves

Copy link
Copy Markdown
Collaborator

Description

Sep24Service.withdraw only applied the syntactic requestValidator.validateAccount(sourceAccount) check to the client-supplied account field before persisting it as from_account. Its sibling Sep24Service.deposit, on the same endpoint and reading the same shared CustodialClient.destinationAccounts/allowAnyDestination configuration, enforces the operator's account-authorization policy via requestValidator.validateDestinationAccount(token, account). A client could therefore record a withdraw from_account that is neither its own SEP-10 identity nor a member of the operator's configured allowlist - exactly what deposit already blocks on the same field. Impact is bounded (the user must still own and sign from that account for the on-chain payment to settle, so no third-party funds are redirected), but it's a real operator-policy bypass and an audit inconsistency.

The ANCHOR-1212 work that generalized this check into SepRequestValidator.validateDestinationAccount had already landed on develop, applied to SEP-6 deposit/deposit-exchange/withdraw/withdraw-exchange and SEP-24 deposit - SEP-24 withdraw was the one path left behind.

Changes

  • Sep24Service.withdraw: replaced requestValidator.validateAccount(sourceAccount) with requestValidator.validateDestinationAccount(token, sourceAccount), matching deposit and all four SEP-6 call sites. validateDestinationAccount already calls validateAccount internally, so syntactic validation is preserved.
  • Sep24ServiceTest: added withdraw-side tests mirroring the existing deposit templates — non-custodial token-mismatch rejected, custodial-allowlisted account accepted, custodial non-allowlisted account rejected, and allowAnyDestination accepted.

Acceptance Criteria

  • A SEP-24 withdraw account that doesn't match the token and has no custodial client config is rejected with ERR_TOKEN_ACCOUNT_MISMATCH.
  • A SEP-24 withdraw account in the caller's CustodialClient.destinationAccounts allowlist is accepted and persisted as from_account.
  • A SEP-24 withdraw account not in the allowlist is rejected with "Provided 'account' is not allowed".
  • A SEP-24 withdraw account is accepted unconditionally when the caller's CustodialClient has allowAnyDestination = true.
  • Existing withdraw tests (bad requests, quote handling) are unaffected.

Context

#3827007

Testing

  • Unit: ./gradlew :core:test --tests "org.stellar.anchor.sep24.Sep24ServiceTest"
  • Full suite: ./gradlew :core:build

Documentation

N/A

Known limitations

N/A

* add robust destination account validation for sep24 withdraw requests

* add tests for destination account whitelisting and 'allowAnyDestination' client configurations
@amandagonsalves amandagonsalves self-assigned this Jul 7, 2026
Copilot AI review requested due to automatic review settings July 7, 2026 14:40

Copilot AI 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.

Pull request overview

This PR closes a SEP-24 authorization gap by aligning Sep24Service.withdraw with deposit and SEP-6 flows: the client-supplied account is now validated against the operator’s configured destination-account policy (allowlist / allow-any), not just syntactically.

Changes:

  • Updated SEP-24 withdraw to call SepRequestValidator.validateDestinationAccount(token, sourceAccount) instead of validateAccount(sourceAccount).
  • Added SEP-24 withdraw unit tests mirroring deposit’s destination-account policy coverage (token mismatch, allowlisted, non-allowlisted, allowAnyDestination).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/src/main/java/org/stellar/anchor/sep24/Sep24Service.java Enforces operator destination-account policy on SEP-24 withdraw account, matching deposit/SEP-6 behavior.
core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Adds withdraw-side tests to cover token mismatch, allowlist, denylist, and allowAnyDestination cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt
Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt
* add `destination_accounts` to `client-wallet-server` configuration

* add `GAIUIZPHLIHQEMNJGSZKCEUWHAZVGUZDBDMO2JXNAJZZZVNSVHQCEWJ4` as a valid destination account
* update expected error message for sep6 account validation

* fix test assertions to match new error message
@amandagonsalves
amandagonsalves requested a review from JiahuiWho July 7, 2026 17:12
@amandagonsalves
amandagonsalves merged commit 7c56fe6 into develop Jul 8, 2026
11 checks passed
@amandagonsalves
amandagonsalves deleted the fix/anchor-1233 branch July 8, 2026 16:07
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.

3 participants