Skip to content

Commit c1ed5a3

Browse files
amandagonsalvesamandagonsalves
authored
[ANCHOR-1226]: SEP-6 deposit/withdraw-exchange skips the quote buy-asset check SEP-24 enforces, binding a quote to a mismatched destination asset (#1969)
### Description SEP-6 `deposit-exchange`/`withdraw-exchange` never checked that a SEP-38 quote's buy asset matched the request's `destination_asset`. `ExchangeAmountsCalculator.calculateFromQuote` — the only entry point SEP-6 uses — hardcoded the buy-asset argument to `null` when calling `validateQuoteAgainstRequestInfo`, skipping the buy-asset-mismatch check that method already implements (and that SEP-24 already relies on via a different entry point). A user could bind a quote priced for a cheap asset to a transaction recorded against a different, valuable `destination_asset`, causing the operator to lose the difference at settlement. **Changes** - [x] `ExchangeAmountsCalculator.calculateFromQuote`: signature changed to accept `buyAsset`, forwarded into `validateQuoteAgainstRequestInfo` instead of `null`. No new overload — the 3-arg version was unconditionally unsafe and had exactly two callers, both already fixed below. - [x] `Sep6Service.depositExchange`/`withdrawExchange`: both now pass their already-resolved destination/buy asset into the call. - [x] `ExchangeAmountsCalculatorTest`: updated all 8 existing calls for the new signature; added tests proving the fixed method rejects a mismatched buy asset and accepts a matching one. - [x] `Sep6ServiceTest`: updated all 8 mock stubs for the new arity; added two tests (deposit-exchange, withdraw-exchange) capturing the `calculateFromQuote` call and asserting the resolved destination asset is what's actually passed as `buyAsset`. **Acceptance Criteria** - [x] `deposit-exchange`/`withdraw-exchange` reject a quote whose buy asset doesn't match `destination_asset`. - [x] A quote whose buy asset matches `destination_asset` succeeds unchanged. - [x] No other caller of `calculateFromQuote` exists or broke (only the two `Sep6Service` call sites in production code). ### Context #3815607 ### Testing - Unit: `./gradlew :core:test --tests "org.stellar.anchor.util.ExchangeAmountsCalculatorTest"` - Unit: `./gradlew :core:test --tests "org.stellar.anchor.sep6.Sep6ServiceTest"` - Full suite: `./gradlew :core:build` ### Documentation N/A ### Known limitations None.
1 parent 546503b commit c1ed5a3

4 files changed

Lines changed: 150 additions & 22 deletions

File tree

‎core/src/main/java/org/stellar/anchor/sep6/Sep6Service.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ public StartDepositResponse depositExchange(WebAuthJwt token, StartDepositExchan
224224
if (request.getQuoteId() != null) {
225225
amounts =
226226
exchangeAmountsCalculator.calculateFromQuote(
227-
request.getQuoteId(), sellAsset, request.getAmount());
227+
request.getQuoteId(), sellAsset, buyAsset, request.getAmount());
228228
} else {
229229
// TODO(philip): remove this
230230
// If a quote is not provided, set the fee and out amounts to 0.
@@ -411,7 +411,7 @@ public StartWithdrawResponse withdrawExchange(
411411
if (request.getQuoteId() != null) {
412412
amounts =
413413
exchangeAmountsCalculator.calculateFromQuote(
414-
request.getQuoteId(), sellAsset, request.getAmount());
414+
request.getQuoteId(), sellAsset, buyAsset, request.getAmount());
415415
} else {
416416
// TODO(philip): remove this
417417
// If a quote is not provided, set the fee and out amounts to 0.

‎core/src/main/java/org/stellar/anchor/util/ExchangeAmountsCalculator.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,15 @@ public class ExchangeAmountsCalculator {
2727
*
2828
* @param quoteId The quote ID
2929
* @param sellAsset The asset the user is selling
30+
* @param buyAsset The asset the user is buying (the request's destination asset)
3031
* @param sellAmount The amount the user is selling
3132
* @return The amounts
3233
* @throws AnchorException if the quote is invalid
3334
*/
34-
public Amounts calculateFromQuote(String quoteId, AssetInfo sellAsset, String sellAmount)
35+
public Amounts calculateFromQuote(
36+
String quoteId, AssetInfo sellAsset, AssetInfo buyAsset, String sellAmount)
3537
throws AnchorException {
36-
Sep38Quote quote = validateQuoteAgainstRequestInfo(quoteId, sellAsset, null, sellAmount);
38+
Sep38Quote quote = validateQuoteAgainstRequestInfo(quoteId, sellAsset, buyAsset, sellAmount);
3739
return Amounts.builder()
3840
.amountIn(quote.getSellAmount())
3941
.amountInAsset(quote.getSellAsset())

‎core/src/test/kotlin/org/stellar/anchor/sep6/Sep6ServiceTest.kt‎

Lines changed: 98 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import org.stellar.anchor.TestConstants.Companion.TEST_ASSET_SEP38_FORMAT
2020
import org.stellar.anchor.TestConstants.Companion.TEST_MEMO
2121
import org.stellar.anchor.TestConstants.Companion.TEST_QUOTE_ID
2222
import org.stellar.anchor.TestHelper
23+
import org.stellar.anchor.api.asset.AssetInfo
2324
import org.stellar.anchor.api.asset.StellarAssetInfo
2425
import org.stellar.anchor.api.event.AnchorEvent
2526
import org.stellar.anchor.api.exception.BadRequestException
@@ -482,7 +483,9 @@ class Sep6ServiceTest {
482483
val slotEvent = slot<AnchorEvent>()
483484
every { eventSession.publish(capture(slotEvent)) } returns Unit
484485

485-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } returns
486+
every {
487+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
488+
} returns
486489
Amounts.builder()
487490
.amountIn("100")
488491
.amountInAsset(sourceAsset)
@@ -520,7 +523,7 @@ class Sep6ServiceTest {
520523

521524
// Verify effects
522525
verify(exactly = 1) {
523-
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), "100")
526+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), "100")
524527
}
525528
verify(exactly = 1) { txnStore.save(any()) }
526529
verify(exactly = 1) { eventSession.publish(any()) }
@@ -561,6 +564,45 @@ class Sep6ServiceTest {
561564
)
562565
}
563566

567+
@Test
568+
fun `test deposit-exchange with quote passes the resolved destination asset as buyAsset`() {
569+
val sourceAsset = "iso4217:USD"
570+
val destinationAsset = TEST_ASSET
571+
572+
every { txnStore.save(any()) } returns null
573+
every { eventSession.publish(any()) } returns Unit
574+
575+
val slotBuyAsset = slot<AssetInfo>()
576+
every {
577+
exchangeAmountsCalculator.calculateFromQuote(
578+
TEST_QUOTE_ID,
579+
any(),
580+
capture(slotBuyAsset),
581+
any(),
582+
)
583+
} returns
584+
Amounts.builder()
585+
.amountIn("100")
586+
.amountInAsset(sourceAsset)
587+
.amountOut("98")
588+
.amountOutAsset(TEST_ASSET_SEP38_FORMAT)
589+
.feeDetails(FeeDetails("2", TEST_ASSET_SEP38_FORMAT))
590+
.build()
591+
592+
val request =
593+
StartDepositExchangeRequest.builder()
594+
.destinationAsset(destinationAsset)
595+
.sourceAsset(sourceAsset)
596+
.quoteId(TEST_QUOTE_ID)
597+
.amount("100")
598+
.account(TEST_ACCOUNT)
599+
.fundingMethod("SWIFT")
600+
.build()
601+
sep6Service.depositExchange(token, request)
602+
603+
assertEquals(asset, slotBuyAsset.captured)
604+
}
605+
564606
@Test
565607
fun `test deposit-exchange without quote`() {
566608
val sourceAsset = "iso4217:USD"
@@ -1066,7 +1108,9 @@ class Sep6ServiceTest {
10661108
val slotEvent = slot<AnchorEvent>()
10671109
every { eventSession.publish(capture(slotEvent)) } returns Unit
10681110

1069-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } returns
1111+
every {
1112+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
1113+
} returns
10701114
Amounts.builder()
10711115
.amountIn("100")
10721116
.amountInAsset(TEST_ASSET_SEP38_FORMAT)
@@ -1105,7 +1149,7 @@ class Sep6ServiceTest {
11051149

11061150
// Verify effects
11071151
verify(exactly = 1) {
1108-
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), "100")
1152+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), "100")
11091153
}
11101154
verify(exactly = 1) { txnStore.save(any()) }
11111155
verify(exactly = 1) { eventSession.publish(any()) }
@@ -1137,6 +1181,44 @@ class Sep6ServiceTest {
11371181
)
11381182
}
11391183

1184+
@Test
1185+
fun `test withdraw-exchange with quote passes the resolved destination asset as buyAsset`() {
1186+
val sourceAsset = TEST_ASSET
1187+
val destinationAsset = "iso4217:USD"
1188+
1189+
every { txnStore.save(any()) } returns null
1190+
every { eventSession.publish(any()) } returns Unit
1191+
1192+
val slotBuyAsset = slot<AssetInfo>()
1193+
every {
1194+
exchangeAmountsCalculator.calculateFromQuote(
1195+
TEST_QUOTE_ID,
1196+
any(),
1197+
capture(slotBuyAsset),
1198+
any(),
1199+
)
1200+
} returns
1201+
Amounts.builder()
1202+
.amountIn("100")
1203+
.amountInAsset(TEST_ASSET_SEP38_FORMAT)
1204+
.amountOut("98")
1205+
.amountOutAsset(destinationAsset)
1206+
.feeDetails(FeeDetails("2", destinationAsset))
1207+
.build()
1208+
1209+
val request =
1210+
StartWithdrawExchangeRequest.builder()
1211+
.sourceAsset(sourceAsset)
1212+
.destinationAsset(destinationAsset)
1213+
.quoteId(TEST_QUOTE_ID)
1214+
.fundingMethod("bank_account")
1215+
.amount("100")
1216+
.build()
1217+
sep6Service.withdrawExchange(token, request)
1218+
1219+
assertEquals(assetService.getAssetById(destinationAsset), slotBuyAsset.captured)
1220+
}
1221+
11401222
@Test
11411223
fun `test withdraw-exchange without quote`() {
11421224
val deadline = 100L
@@ -1708,8 +1790,9 @@ class Sep6ServiceTest {
17081790

17091791
@Test
17101792
fun `test depositExchange rejects already-bound quote`() {
1711-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } throws
1712-
BadRequestException("quote(id=$TEST_QUOTE_ID) has already been used")
1793+
every {
1794+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
1795+
} throws BadRequestException("quote(id=$TEST_QUOTE_ID) has already been used")
17131796
val request =
17141797
StartDepositExchangeRequest.builder()
17151798
.destinationAsset(TEST_ASSET)
@@ -1725,7 +1808,9 @@ class Sep6ServiceTest {
17251808

17261809
@Test
17271810
fun `test depositExchange bind failure rejects second use`() {
1728-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } returns
1811+
every {
1812+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
1813+
} returns
17291814
Amounts.builder()
17301815
.amountIn("100")
17311816
.amountInAsset("iso4217:USD")
@@ -1751,8 +1836,9 @@ class Sep6ServiceTest {
17511836

17521837
@Test
17531838
fun `test withdrawExchange rejects already-bound quote`() {
1754-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } throws
1755-
BadRequestException("quote(id=$TEST_QUOTE_ID) has already been used")
1839+
every {
1840+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
1841+
} throws BadRequestException("quote(id=$TEST_QUOTE_ID) has already been used")
17561842
val request =
17571843
StartWithdrawExchangeRequest.builder()
17581844
.sourceAsset(TEST_ASSET)
@@ -1767,7 +1853,9 @@ class Sep6ServiceTest {
17671853

17681854
@Test
17691855
fun `test withdrawExchange bind failure rejects second use`() {
1770-
every { exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any()) } returns
1856+
every {
1857+
exchangeAmountsCalculator.calculateFromQuote(TEST_QUOTE_ID, any(), any(), any())
1858+
} returns
17711859
Amounts.builder()
17721860
.amountIn("100")
17731861
.amountInAsset(TEST_ASSET_SEP38_FORMAT)

‎core/src/test/kotlin/org/stellar/anchor/util/ExchangeAmountsCalculatorTest.kt‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class ExchangeAmountsCalculatorTest {
5757
val quoteId = "id"
5858
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote()
5959

60-
val result = calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "100")
60+
val result = calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "100")
6161
assertEquals(
6262
Amounts.builder()
6363
.amountIn("100")
@@ -74,7 +74,7 @@ class ExchangeAmountsCalculatorTest {
7474
fun `test calculateFromQuote with invalid quote id`() {
7575
every { sep38QuoteStore.findByQuoteId(any()) } returns null
7676
assertThrows<BadRequestException> {
77-
calculator.calculateFromQuote("id", assetService.getAsset("USDC"), "100")
77+
calculator.calculateFromQuote("id", assetService.getAsset("USDC"), null, "100")
7878
}
7979
}
8080

@@ -88,7 +88,7 @@ class ExchangeAmountsCalculatorTest {
8888
}
8989

9090
assertThrows<BadRequestException> {
91-
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "100")
91+
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "100")
9292
}
9393
}
9494

@@ -97,7 +97,7 @@ class ExchangeAmountsCalculatorTest {
9797
val quoteId = "id"
9898
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote()
9999
assertThrows<BadRequestException> {
100-
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "99")
100+
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "99")
101101
}
102102
}
103103

@@ -106,7 +106,7 @@ class ExchangeAmountsCalculatorTest {
106106
val quoteId = "id"
107107
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote()
108108
assertThrows<BadRequestException> {
109-
calculator.calculateFromQuote(quoteId, assetService.getAsset("JPYC"), "100")
109+
calculator.calculateFromQuote(quoteId, assetService.getAsset("JPYC"), null, "100")
110110
}
111111
}
112112

@@ -115,7 +115,7 @@ class ExchangeAmountsCalculatorTest {
115115
val quoteId = "id"
116116
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote().apply { fee = null }
117117
assertThrows<SepValidationException> {
118-
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "100")
118+
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "100")
119119
}
120120
}
121121

@@ -133,14 +133,52 @@ class ExchangeAmountsCalculatorTest {
133133
}
134134
}
135135

136+
@Test
137+
fun `test calculateFromQuote with mismatched buy asset throws`() {
138+
val quoteId = "id"
139+
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote()
140+
assertThrows<BadRequestException> {
141+
calculator.calculateFromQuote(
142+
quoteId,
143+
assetService.getAsset("USDC"),
144+
assetService.getAsset("JPYC"),
145+
"100",
146+
)
147+
}
148+
}
149+
150+
@Test
151+
fun `test calculateFromQuote with matching buy asset succeeds`() {
152+
val quoteId = "id"
153+
every { sep38QuoteStore.findByQuoteId(quoteId) } returns usdcQuote()
154+
155+
val result =
156+
calculator.calculateFromQuote(
157+
quoteId,
158+
assetService.getAsset("USDC"),
159+
assetService.getAsset("USD"),
160+
"100",
161+
)
162+
assertEquals(
163+
Amounts.builder()
164+
.amountIn("100")
165+
.amountInAsset(TEST_ASSET_SEP38_FORMAT)
166+
.amountOut("98")
167+
.amountOutAsset("iso4217:USD")
168+
.feeDetails(FeeDetails("2", "iso4217:USD"))
169+
.build(),
170+
result,
171+
)
172+
}
173+
136174
@Test
137175
fun `test calculateFromQuote rejects already-bound quote`() {
138176
val quoteId = "id"
139177
every { sep38QuoteStore.findByQuoteId(quoteId) } returns
140178
usdcQuote().apply { transactionId = "existing-txn-id" }
141179
val ex =
142180
assertThrows<BadRequestException> {
143-
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "100")
181+
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "100")
144182
}
145183
assert(ex.message!!.contains("has already been used"))
146184
}
@@ -179,7 +217,7 @@ class ExchangeAmountsCalculatorTest {
179217
usdcQuote().apply { transactionId = "T1-cancelled" }
180218
val ex =
181219
assertThrows<BadRequestException> {
182-
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), "100")
220+
calculator.calculateFromQuote(quoteId, assetService.getAsset("USDC"), null, "100")
183221
}
184222
assert(ex.message!!.contains("has already been used"))
185223
}

0 commit comments

Comments
 (0)