From b247d2db2987bee9cafa3bed24d954af064d65e1 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 7 Jul 2026 14:29:48 -0400 Subject: [PATCH] Fix cross-issuer MPToken authorization in AMMClawback Commit 21cfba66fd recreated a holder's missing MPToken as authorized for both pool assets. On a cross-issuer MPT/MPT pool this let one issuer's clawback re-authorize the holder for the *other* issuer's asset, bypassing its lsfMPTRequireAuth. Only auto-authorize the recreated MPToken for the clawback issuer's own asset; recreate the paired asset unauthorized. --- include/xrpl/tx/transactors/dex/AMMWithdraw.h | 7 ++ .../tx/transactors/dex/AMMClawback.cpp | 9 ++ .../tx/transactors/dex/AMMWithdraw.cpp | 21 ++++- src/test/app/AMMClawbackMPT_test.cpp | 82 +++++++++++++++++++ 4 files changed, 116 insertions(+), 3 deletions(-) diff --git a/include/xrpl/tx/transactors/dex/AMMWithdraw.h b/include/xrpl/tx/transactors/dex/AMMWithdraw.h index 8f6700037d..220c5ca109 100644 --- a/include/xrpl/tx/transactors/dex/AMMWithdraw.h +++ b/include/xrpl/tx/transactors/dex/AMMWithdraw.h @@ -116,6 +116,7 @@ public: Sandbox& view, SLE const& ammSle, AccountID const account, + std::optional const& clawbackIssuer, AccountID const& ammAccount, STAmount const& amountBalance, STAmount const& amount2Balance, @@ -135,6 +136,11 @@ public: * @param view * @param ammSle AMM ledger entry * @param ammAccount AMM account + * @param clawbackIssuer when set (AMMClawback path), the issuer performing + * the clawback. A recreated MPToken is only auto-authorized when the + * asset's issuer matches this account, so a clawback cannot grant + * authorization on behalf of a different (paired-asset) issuer. + * @param account LP account * @param amountBalance current LP asset1 balance * @param amountWithdraw asset1 withdraw amount * @param amount2Withdraw asset2 withdraw amount @@ -150,6 +156,7 @@ public: Sandbox& view, SLE const& ammSle, AccountID const& ammAccount, + std::optional const& clawbackIssuer, AccountID const& account, STAmount const& amountBalance, STAmount const& amountWithdraw, diff --git a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp index c1ef9f875e..7a4b22e801 100644 --- a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp @@ -227,6 +227,7 @@ AMMClawback::applyGuts(Sandbox& sb) sb, *ammSle, holder, + issuer, ammAccount, amountBalance, amount2Balance, @@ -311,6 +312,11 @@ AMMClawback::equalWithdrawMatchingOneAmount( STAmount const& holdLPtokens, STAmount const& amount) { + // The clawback issuer signs for its own asset only. Threaded into the + // withdrawal so a recreated MPToken is auto-authorized only for the + // clawback issuer's asset, never for a paired asset from another issuer. + AccountID const issuer = ctx_.tx[sfAccount]; + auto frac = Number{amount} / amountBalance; auto amount2Withdraw = amount2Balance * frac; @@ -324,6 +330,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( sb, ammSle, holder, + issuer, ammAccount, amountBalance, amount2Balance, @@ -357,6 +364,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( sb, ammSle, ammAccount, + issuer, holder, amountBalance, amountRounded, @@ -377,6 +385,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( sb, ammSle, ammAccount, + issuer, holder, amountBalance, amount, diff --git a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp index d6ac1ebb9a..44972aa4fb 100644 --- a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp @@ -493,6 +493,7 @@ AMMWithdraw::withdraw( view, ammSle, ammAccount, + std::nullopt, accountID_, amountBalance, amountWithdraw, @@ -513,6 +514,7 @@ AMMWithdraw::withdraw( Sandbox& view, SLE const& ammSle, AccountID const& ammAccount, + std::optional const& clawbackIssuer, AccountID const& account, STAmount const& amountBalance, STAmount const& amountWithdraw, @@ -689,9 +691,18 @@ AMMWithdraw::withdraw( // AMMClawback ignores authorization so the issuer can recover // MPT locked in the pool even if the holder deleted their - // MPToken. Recreate that MPToken as authorized for the - // clawback withdrawal path only. - createFlags = lsfMPTAuthorized; + // MPToken. Only auto-authorize the recreated MPToken for the + // clawback issuer's own asset: authorization is granted by an + // asset's issuer, and the clawback transaction is signed by + // that issuer only for its own asset. For a paired asset issued + // by a different account, recreate the MPToken *unauthorized* so + // the clawback does not grant authorization on behalf of that + // issuer (which would bypass its lsfMPTRequireAuth). The holder + // still receives the paired asset (accountSend only requires the + // MPToken to exist, not to be authorized); the balance remains + // gated by its issuer until that issuer authorizes it. + if (clawbackIssuer && asset.getIssuer() == *clawbackIssuer) + createFlags = lsfMPTAuthorized; } if (auto const err = checkCreateMPT(view, mptIssue, account, createFlags, journal); @@ -790,6 +801,7 @@ AMMWithdraw::equalWithdrawTokens( view, ammSle, accountID_, + std::nullopt, ammAccount, amountBalance, amount2Balance, @@ -841,6 +853,7 @@ AMMWithdraw::equalWithdrawTokens( Sandbox& view, SLE const& ammSle, AccountID const account, + std::optional const& clawbackIssuer, AccountID const& ammAccount, STAmount const& amountBalance, STAmount const& amount2Balance, @@ -863,6 +876,7 @@ AMMWithdraw::equalWithdrawTokens( view, ammSle, ammAccount, + clawbackIssuer, account, amountBalance, amountBalance, @@ -898,6 +912,7 @@ AMMWithdraw::equalWithdrawTokens( view, ammSle, ammAccount, + clawbackIssuer, account, amountBalance, amountWithdraw, diff --git a/src/test/app/AMMClawbackMPT_test.cpp b/src/test/app/AMMClawbackMPT_test.cpp index 3c1dbb420e..9895874c90 100644 --- a/src/test/app/AMMClawbackMPT_test.cpp +++ b/src/test/app/AMMClawbackMPT_test.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -1979,6 +1980,86 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite } } + void + testClawbackCrossIssuerPairedAssetAuth(FeatureBitset features) + { + testcase("test AMMClawback recreates paired-issuer MPToken unauthorized"); + using namespace jtx; + + // Cross-issuer MPT/MPT pool: btc is issued by gw, eth by gw2, and both + // require authorization. Alice deposits her entire balance of both and + // deletes the resulting zero-balance MPTokens. When gw claws back its + // own asset (btc), the two-asset withdrawal must recreate both of + // Alice's MPTokens so the pool can pay her the paired asset. The + // recreated MPToken may only be auto-authorized for the clawback + // issuer's own asset (btc); the paired asset's issuer (gw2) never + // consented, so eth must be recreated *unauthorized*, leaving gw2 in + // control of its own token and preserving its RequireAuth guarantee. + Env env(*this, features); + Account const gw{"gateway"}; + Account const gw2{"gateway2"}; + Account const alice{"alice"}; + env.fund(XRP(100'000), gw, gw2, alice); + env.close(); + + MPTTester btc( + {.env = env, + .issuer = gw, + .holders = {alice}, + .pay = 10'000, + .flags = tfMPTCanClawback | tfMPTRequireAuth | kMptDexFlags, + .authHolder = true}); + + MPTTester eth( + {.env = env, + .issuer = gw2, + .holders = {alice}, + .pay = 10'000, + .flags = tfMPTCanClawback | tfMPTRequireAuth | kMptDexFlags, + .authHolder = true}); + + // Alice deposits everything into the pool; her MPT balances drop to 0. + AMM const amm(env, alice, btc(10'000), eth(10'000)); + env.close(); + BEAST_EXPECT(amm.expectBalances(btc(10'000), eth(10'000), IOUAmount{10'000})); + BEAST_EXPECT(env.balance(alice, btc) == btc(0)); + BEAST_EXPECT(env.balance(alice, eth) == eth(0)); + + // Alice deletes both zero-balance MPTokens to reclaim reserves. + btc.authorize({.account = alice, .flags = tfMPTUnauthorize}); + eth.authorize({.account = alice, .flags = tfMPTUnauthorize}); + BEAST_EXPECT(!env.le(keylet::mptoken(btc.issuanceID(), alice.id()))); + BEAST_EXPECT(!env.le(keylet::mptoken(eth.issuanceID(), alice.id()))); + + // gw (issuer of btc) claws back part of Alice's btc. This is a + // cross-issuer pool, so tfClawTwoAssets is not permitted: only btc is + // clawed back, while the paired eth is returned to Alice. + env(amm::ammClawback(gw, alice, btc, eth, btc(1'000))); + env.close(); + + // Both MPTokens were recreated so the withdrawal could pay Alice. + auto const sleBtc = env.le(keylet::mptoken(btc.issuanceID(), alice.id())); + auto const sleEth = env.le(keylet::mptoken(eth.issuanceID(), alice.id())); + BEAST_EXPECT(sleBtc); + BEAST_EXPECT(sleEth); + + // The clawback issuer's own asset (btc) may be recreated authorized: + // gw has authority over its own token. + BEAST_EXPECT(sleBtc && sleBtc->isFlag(lsfMPTAuthorized)); + + // The paired asset (eth) is issued by gw2, who did not sign this + // transaction. It must be recreated *unauthorized* so gw2's RequireAuth + // is not bypassed. This is the core assertion for the cross-issuer fix. + BEAST_EXPECT(sleEth && !sleEth->isFlag(lsfMPTAuthorized)); + + // The clawback still completed: btc was clawed back (Alice keeps a zero + // btc balance) and the paired eth was delivered into Alice's now + // unauthorized, gw2-gated MPToken (non-zero raw balance). + BEAST_EXPECT(sleBtc && sleBtc->getFieldU64(sfMPTAmount) == 0); + BEAST_EXPECT(sleEth && sleEth->getFieldU64(sfMPTAmount) > 0); + BEAST_EXPECT(amm.ammExists()); + } + void run() override { @@ -1994,6 +2075,7 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite testAssetFrozenOrLocked(all); testClawbackCreatesMissingMPToken(all); testClawbackAfterDeletingMPTokens(all); + testClawbackCrossIssuerPairedAssetAuth(all); testSingleDepositAndClawback(all); testLastHolderLPTokenBalance(all); testLastHolderLPTokenBalance(all - fixAMMv1_3 - fixAMMClawbackRounding);