Skip to content

[ANCHOR-1228]: SEP-24 per-asset transaction limit (min/max_amount) bypassable via quote_id with omitted amount #1970

Merged
amandagonsalves merged 4 commits into
developfrom
fix/anchor-1228
Jul 13, 2026
Merged

[ANCHOR-1228]: SEP-24 per-asset transaction limit (min/max_amount) bypassable via quote_id with omitted amount #1970
amandagonsalves merged 4 commits into
developfrom
fix/anchor-1228

Conversation

@amandagonsalves

Copy link
Copy Markdown
Collaborator

Description

SEP-24 withdraw/deposit guarded their asset's min_amount/max_amount checks with strAmount != null (Sep24Service.java). When a client supplied quote_id and omitted amount - both legal per the SEP-24 spec - those checks were skipped entirely, and validateAndPopulateQuote copied the quote's sell_amount/buy_amount straight into the transaction (amountExpected/amountIn/amountOut) with no limit check anywhere in that path: not in ExchangeAmountsCalculator.validateQuoteAgainstRequestInfo (only cross-checks the request amount against the quote when the request amount is non-null), and not in SEP-38 quote creation (only validates sign/scale). A user could drive a deposit or withdrawal arbitrarily above or below the operator's configured cap with two ordinary requests. SEP-6's exchange endpoints were never affected - they call the min/max-checking validateAmount unconditionally, before the quote is ever applied.

Changes

  • Sep24Service.validateAndPopulateQuote: now returns the Sep38Quote it fetches instead of discarding it.
  • Sep24Service.withdraw/deposit: on the quoteId != null branch, unconditionally validate the relevant side of the quote against the asset's already-known minAmount/maxAmount via requestValidator.validateAmount(...) (the same generic method SEP-6 already uses unconditionally). Withdraw checks quote.getSellAmount() (the withdraw asset is the quote's sell side); deposit checks quote.getBuyAmount() (the deposit asset is the quote's buy side). Runs regardless of whether amount was also supplied - when both are present, the existing cross-check already ties them together, so this is harmless-redundant there and load-bearing only on the omitted-amount path.
  • Sep24ServiceTest: added six tests - for each of withdraw/deposit: rejects a quote above max_amount, rejects a quote below min_amount, and succeeds when amount is omitted and the quote is legitimately in-bounds (previously uncovered - the existing quote_id tests always included an amount field).

Acceptance Criteria

  • POST /sep24/transactions/{withdraw,deposit}/interactive with quote_id and no amount, where the quote's relevant amount is outside the asset's min_amount/max_amount, is rejected with the same error as the explicit-amount path.
  • The same request succeeds when the quote's amount is within bounds.
  • Existing quote-based and plain-amount tests are unaffected.

Context

#3819378

Testing

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

Documentation

N/A

Known limitations

N/A

* update `validateandpopulatequote` to return the `sep38quote` object

* add amount validation for quoted `buy_amount` in deposit requests

* add amount validation for quoted `sell_amount` in withdraw requests

* add unit tests to verify quote amount validation
@amandagonsalves amandagonsalves self-assigned this Jul 7, 2026
Copilot AI review requested due to automatic review settings July 7, 2026 17:19

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 validation gap where min_amount/max_amount could be bypassed by providing quote_id while omitting amount, by ensuring the quote’s relevant side is validated against the asset limits and adding targeted unit tests for the omitted-amount path.

Changes:

  • Sep24Service.withdraw / deposit: validate quote-derived amount against the asset’s min_amount / max_amount on the quote_id path.
  • Sep24Service.validateAndPopulateQuote: now returns the fetched Sep38Quote so callers can validate it.
  • Sep24ServiceTest: adds coverage for quote_id + omitted amount (in-bounds and out-of-bounds).

Reviewed changes

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

File Description
core/src/main/java/org/stellar/anchor/sep24/Sep24Service.java Adds min/max validation for quote-derived amounts and returns the quote from validateAndPopulateQuote.
core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Adds tests for quote_id requests that omit amount, including min/max boundary cases.

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

Comment thread core/src/main/java/org/stellar/anchor/sep24/Sep24Service.java Outdated
Comment thread core/src/main/java/org/stellar/anchor/sep24/Sep24Service.java Outdated
Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Outdated
Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Outdated
Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Outdated
Comment thread core/src/test/kotlin/org/stellar/anchor/sep24/Sep24ServiceTest.kt Outdated
Comment thread core/src/main/java/org/stellar/anchor/sep24/Sep24Service.java Outdated
@amandagonsalves
amandagonsalves marked this pull request as draft July 7, 2026 17:43
*   refactor amount limit validation logic into a new private method, `validateamountlimits`

*   remove duplicated min and max amount validation code from deposit and withdraw flows

*   update deposit and withdraw methods to use the new `validateamountlimits` method

*   update sep24service tests to assert specific validation error messages
@amandagonsalves
amandagonsalves marked this pull request as ready for review July 13, 2026 13:22
@amandagonsalves
amandagonsalves merged commit f9e9d4b into develop Jul 13, 2026
17 of 21 checks passed
@amandagonsalves
amandagonsalves deleted the fix/anchor-1228 branch July 13, 2026 17:44
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