ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-18 21:10:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Authorize AMM pseudo-accounts holding vault shares

Browse Source 
A pseudo-account that holds vault shares but not the underlying asset fails
requireAuth's recursive share->underlying check, which left an AMM unable to
read its own vault-share balance (read as zero via accountHolds) and blocked
the MPToken transfer invariant on AMM create.

Exempt pseudo-accounts before the recursive check in requireAuth, gated by
fixCleanup3_3_0 to preserve pre-amendment consensus, and treat them as
authorized in the MPToken transfer invariant (fork-safe: that invariant is
enforced only under MPTokensV2). Create and withdraw of AMM-held vault shares
now succeed once fixCleanup3_3_0 is enabled.

Add AMM/vault-share coverage and side-specific AMM_WITHDRAW freeze invariant
tests, and correct the pre-amendment test expectations: pre-fixCleanup3_3_0
the shares remain inaccessible to the pseudo-account, so the withdraw fails
with tecAMM_FAILED.
This commit is contained in:
Vito
2026-06-25 14:34:22 +02:00
parent 95ddd1d251
commit ea8c6ff96f
5 changed files with 223 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
src/libxrpl/ledger/helpers/MPTokenHelpers.cpp
View File

@@ -308,6 +308,18 @@ requireAuth(
AuthType authType,
std::uint8_t depth)
{
bool const fix330Enabled = view.rules().enabled(fixCleanup3_3_0);
bool const featureSAVEnabled = view.rules().enabled(featureSingleAssetVault);
bool const featureMPTV2Enabled = view.rules().enabled(featureMPTokensV2);
// Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of their participants.
// They are implicitly authorized for any MPT they hold, including vault shares whose
// underlying asset would otherwise require auth.
auto const isPseudoAccountExempt = [&] {
return (featureSAVEnabled || featureMPTV2Enabled) &&
isPseudoAccount(view, account, {&sfVaultID, &sfLoanBrokerID, &sfAMMID});
};
auto const mptID = keylet::mptIssuance(mptIssue.getMptID());
auto const sleIssuance = view.read(mptID);
if (!sleIssuance)
@@ -319,18 +331,9 @@ requireAuth(
if (mptIssuer == account) // Issuer won't have MPToken
return tesSUCCESS;
bool const featureSAVEnabled = view.rules().enabled(featureSingleAssetVault);
bool const featureMPTV2Enabled = view.rules().enabled(featureMPTokensV2);
if (featureSAVEnabled || featureMPTV2Enabled)
{
// Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of
// their participants and do not have trustlines to underlying assets.
// They are implicitly authorized for any MPT they hold, including vault
// shares whose underlying asset would otherwise require auth.
if (isPseudoAccount(view, account, {&sfVaultID, &sfLoanBrokerID, &sfAMMID}))
return tesSUCCESS;
}
// Post-fix330: exempt before the recursive underlying-asset auth check.
if (fix330Enabled && isPseudoAccountExempt())
return tesSUCCESS;
if (featureSAVEnabled)
{
@@ -393,6 +396,10 @@ requireAuth(
// belong to someone who is explicitly authorized e.g. a vault owner.
}
// Pre-fix330: exempt after domain/sleToken checks, preserving prior behavior.
if (!fix330Enabled && isPseudoAccountExempt())
return tesSUCCESS;
// mptoken must be authorized if issuance enabled requireAuth
if (sleIssuance->isFlag(lsfMPTRequireAuth) &&
(!sleToken || !sleToken->isFlag(lsfMPTAuthorized)))

35
src/libxrpl/tx/invariants/MPTInvariant.cpp
View File

@@ -5,6 +5,7 @@
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
@@ -510,6 +511,15 @@ ValidMPTTransfer::isAuthorized(
AccountID const& holder,
bool reqAuth) const
{
// Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of their
// participants and are implicitly authorized for any MPT they hold,
// including vault shares whose underlying asset would otherwise require
// auth. Exempt them here rather than relying on requireAuth: the recursive
// share -> underlying descent in requireAuth fails for a pseudo-account
// that holds the share but not the underlying.
if (isPseudoAccount(view, holder, {&sfVaultID, &sfLoanBrokerID, &sfAMMID}))
return true;
auto const key = keylet::mptoken(mptid, holder);
auto const it = deletedAuthorized_.find(key.key);
if (it != deletedAuthorized_.end())
@@ -590,13 +600,24 @@ ValidMPTTransfer::finalize(
// Check once: if any involved account is frozen, the whole issuance transfer is
// considered frozen. Only need to check for frozen if there is a transfer of funds.
//
// The isVaultPseudoAccountFrozen check (transitive freeze via vault underlying) is
// only enforced post-fixCleanup3_3_0; pre-amendment, AMM withdrawals of vault
// shares are permitted even when the underlying asset is individually frozen.
bool const accountFrozen = isGlobalFrozen(view, MPTIssue{mptID}) ||
isIndividualFrozen(view, account, MPTIssue{mptID}) ||
(fix330Enabled &&
isVaultPseudoAccountFrozen(view, account, MPTIssue{mptID}, 0));
// Post-fix330: full isFrozen() applies — vault-share transitive freeze is part of
// the freeze semantics for all changed holders.
//
// Pre-fix330: legacy AMM withdraw only checked individual freeze on the
// destination, not the transitive vault freeze. All other paths (and the AMM
// account itself as sender) did apply the full check.
MPTIssue const issue{mptID};
auto const legacyAccountFrozen = [&] {
if (isGlobalFrozen(view, issue) || isIndividualFrozen(view, account, issue))
return true;
bool const isReceiver =
!value.amtBefore.has_value() || *value.amtAfter > *value.amtBefore;
if (txnType == ttAMM_WITHDRAW && isReceiver)
return false;
return isVaultPseudoAccountFrozen(view, account, issue, 0);
};
bool const accountFrozen =
fix330Enabled ? isFrozen(view, account, issue) : legacyAccountFrozen();
if (!invalidTransfer &&
(accountFrozen || !isAuthorized(view, mptID, account, reqAuth)))
{

52
src/test/app/AMMMPT_test.cpp
View File

@@ -18,12 +18,12 @@
#include <test/jtx/ter.h>
#include <test/jtx/trust.h>
#include <test/jtx/txflags.h>
#include <test/jtx/utility.h>
#include <test/jtx/vault.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/basics/chrono.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/json/json_value.h>
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/helpers/AMMHelpers.h>
@@ -6914,7 +6914,7 @@ private:
auto runIOU = [&](FeatureBitset const& features) {
bool const fix330 = features[fixCleanup3_3_0];
Env env{*this, features};
Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled};
env.fund(XRP(100'000), gw_, alice_);
env(fset(gw_, asfDefaultRipple));
@@ -6954,21 +6954,33 @@ private:
.asset1In = XRP(1),
.err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecLOCKED))});
// post-fix330: checkDeepFrozen(alice, share) descends to alice's
// frozen IOU → tecLOCKED
// pre-fix330: old path checks AMM account's MPToken lock only
// (unset) → tesSUCCESS
// post-fix330: checkWithdrawFreeze → checkDeepFrozen(alice, share)
// descends to alice's frozen IOU → tecLOCKED
// pre-fix330: the AMM pseudo-account is not authorized for the
// share's underlying (requireAuth recurses share→IOU
// and the AMM holds no IOU trustline), so
// accountHolds(ZeroIfUnauthorized) reports the pool's
// share balance as 0 and the withdrawal math fails →
// tecAMM_FAILED. Vault shares deposited into an AMM are
// only withdrawable once fixCleanup3_3_0 exempts the
// pseudo-account from the recursive auth check.
amm.withdraw(
{.account = alice_,
.tokens = 1'000,
.err = Ter(fix330 ? TER(tecLOCKED) : TER(tesSUCCESS))});
.err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))});
env(trust(gw_, iou(0), alice_, tfClearFreeze));
env.close();
// Confirm both operations succeed once the freeze is lifted.
// Lifting the freeze lets the deposit through in both cases. The
// withdrawal only succeeds post-fix330; pre-fix330 the share balance
// remains inaccessible to the unauthorized pseudo-account, so the
// shares stay stuck → tecAMM_FAILED.
amm.deposit({.account = alice_, .asset1In = XRP(1)});
amm.withdraw({.account = alice_, .tokens = 1'000});
amm.withdraw(
{.account = alice_,
.tokens = 1'000,
.err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))});
};
runIOU(all);
@@ -6976,7 +6988,9 @@ private:
auto runMPT = [&](FeatureBitset const& features) {
bool const fix330 = features[fixCleanup3_3_0];
Env env{*this, features};
// Expected freeze failures fire invariant checks that log at Error;
// silence them so the test output stays clean.
Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled};
env.fund(XRP(100'000), gw_, alice_);
env.close();
@@ -7015,15 +7029,27 @@ private:
// {.tokens = 1'000} → frac = 1000/1e6 = 0.001
// XRP out = 1e8 * 0.001 = 1e5 drops, shares out = 10000 * 0.001 = 10
// post-fix330: checkWithdrawFreeze sees alice's locked underlying
// MPT via the share → tecLOCKED.
// pre-fix330: the AMM pseudo-account is unauthorized for the
// share's underlying MPT (it holds no underlying
// MPToken), so accountHolds(ZeroIfUnauthorized) zeros
// the pool's share balance and the math fails →
// tecAMM_FAILED.
amm.withdraw(
{.account = alice_,
.tokens = 1'000,
.err = Ter(fix330 ? TER(tecLOCKED) : TER(tesSUCCESS))});
.err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))});
mptt.set({.holder = alice_, .flags = tfMPTUnlock});
// Unlocking lets the deposit through; the withdrawal only succeeds
// post-fix330 (pre-fix330 the shares remain stuck → tecAMM_FAILED).
amm.deposit({.account = alice_, .asset1In = XRP(1)});
amm.withdraw({.account = alice_, .tokens = 1'000});
amm.withdraw(
{.account = alice_,
.tokens = 1'000,
.err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))});
};
runMPT(all);
@@ -7252,7 +7278,7 @@ private:
FeatureBitset const all{jtx::testableAmendments()};
testInstanceCreate();
testInvalidInstance();
for (auto const& f : jtx::amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}))
for (auto const& f : jtx::amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}, all))
testInvalidDeposit(f);
testDeposit();
testInvalidWithdraw();

1
src/test/app/AMM_test.cpp
View File

@@ -19,7 +19,6 @@
#include <test/jtx/ter.h>
#include <test/jtx/trust.h>
#include <test/jtx/txflags.h>
#include <test/jtx/utility.h>
#include <xrpl/basics/Number.h>
#include <xrpl/basics/base_uint.h>

143
src/test/app/Invariants_test.cpp
View File

@@ -4,6 +4,7 @@
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/mpt.h>
#include <test/jtx/pay.h>
#include <test/jtx/permissioned_domains.h>
@@ -4622,8 +4623,11 @@ class Invariants_test : public beast::unit_test::Suite
shareID = env.le(vaultKeylet)->at(sfShareMPTID);
// Freeze a1's IOU trustline from the issuer side.
env(trust(gw, gw["IOU"](0), a1, tfSetFreeze));
// Freeze a2's IOU trustline from the issuer side.
// a2 is the receiver in the simulated AMM withdraw; the
// distinction under test is that pre-fix330 the invariant
// does not apply the transitive vault freeze to receivers.
env(trust(gw, gw["IOU"](0), a2, tfSetFreeze));
env.close();
return true;
};
@@ -4642,8 +4646,9 @@ class Invariants_test : public beast::unit_test::Suite
return true;
};
// post-fixCleanup3_3_0: isVaultPseudoAccountFrozen finds a1's
// underlying IOU frozen → invalidTransfer → invariant fires.
// post-fixCleanup3_3_0: full isFrozen() applies to all holders;
// isVaultPseudoAccountFrozen finds a2's underlying IOU frozen →
// invalidTransfer → invariant fires.
doInvariantCheck(
Env{*this, defaultAmendments()},
{{"invalid MPToken transfer between holders"}},
@@ -4653,8 +4658,9 @@ class Invariants_test : public beast::unit_test::Suite
{tecINVARIANT_FAILED, tefINVARIANT_FAILED},
preclose);
// pre-fixCleanup3_3_0: isVaultPseudoAccountFrozen is not called;
// transfer is allowed even though the underlying IOU is frozen.
// pre-fixCleanup3_3_0: legacy AMM withdraw only checked
// checkIndividualFrozen on the destination, not the transitive
// vault freeze; a2 as receiver is exempt → invariant passes.
doInvariantCheck(
Env{*this, defaultAmendments() - fixCleanup3_3_0},
{},
@@ -4664,6 +4670,131 @@ class Invariants_test : public beast::unit_test::Suite
{tesSUCCESS, tesSUCCESS},
preclose);
}
// Side-specific vault-share AMM_WITHDRAW invariant tests.
// Both cases use a real vault (IOU underlying) and a real AMM whose
// pool includes vault shares. precheck simulates an AMM_WITHDRAW by
// transferring 10 vault shares from the AMM pseudo-account to a2.
{
MPTID shareID{};
AccountID ammAcctID{};
AccountID vaultPseudoID{};
Account const gw{"gw"};
// Simulate AMM_WITHDRAW: AMM pseudo-account sends 10 vault shares
// to a2. The AMM pseudo is the sender (decreasing balance);
// a2 is the receiver (increasing balance).
auto const precheck2 =
[&](Account const& /*a1*/, Account const& a2, ApplyContext& ac) -> bool {
auto sleAMM = ac.view().peek(keylet::mptoken(shareID, ammAcctID));
auto sle2 = ac.view().peek(keylet::mptoken(shareID, a2.id()));
if (!sleAMM || !sle2)
return false;
(*sleAMM)[sfMPTAmount] -= 10;
(*sle2)[sfMPTAmount] += 10;
ac.view().update(sleAMM);
ac.view().update(sle2);
return true;
};
// Shared vault + AMM setup: a1 deposits 500 IOU into a vault and
// creates an AMM with XRP + 100 vault shares, giving the AMM
// pseudo-account a vault-share MPToken balance.
auto const setupVaultAMM = [&](Account const& a1, Account const& a2, Env& env) -> bool {
env.fund(XRP(1'000), gw);
env(fset(gw, asfDefaultRipple));
env.trust(gw["IOU"](10'000), a1);
env.trust(gw["IOU"](10'000), a2);
env.close();
env(pay(gw, a1, gw["IOU"](1'000)));
env(pay(gw, a2, gw["IOU"](500)));
env.close();
Vault const vault{env};
auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = gw["IOU"]});
env(createTx);
env.close();
env(vault.deposit(
{.depositor = a1, .id = vaultKeylet.key, .amount = gw["IOU"](500)}));
env(vault.deposit(
{.depositor = a2, .id = vaultKeylet.key, .amount = gw["IOU"](200)}));
env.close();
shareID = env.le(vaultKeylet)->at(sfShareMPTID);
vaultPseudoID = env.le(vaultKeylet)->at(sfAccount);
// a1 creates AMM with XRP + 100 vault shares; the AMM
// pseudo-account receives an MPToken record for shareID.
AMM const amm(env, a1, XRP(100), STAmount{MPTIssue{shareID}, 100});
ammAcctID = amm.ammAccount();
return true;
};
// Case 1: freeze the vault pseudo-account's IOU trustline.
// isVaultPseudoAccountFrozen(ammAcct) calls isAnyFrozen({vaultPseudo,
// ammAcct}, IOU); since vaultPseudo is frozen it returns true. The
// AMM sender has a decreasing balance (not a receiver) so it is
// never exempt from the check — invariant fires both pre- and
// post-fixCleanup3_3_0.
auto const preclose3 = [&](Account const& a1, Account const& a2, Env& env) -> bool {
if (!setupVaultAMM(a1, a2, env))
return false;
env(trust(gw, gw["IOU"](0), Account{"vaultPseudo", vaultPseudoID}, tfSetFreeze));
env.close();
return true;
};
doInvariantCheck(
Env{*this, defaultAmendments()},
{{"invalid MPToken transfer between holders"}},
precheck2,
XRPAmount{},
STTx{ttAMM_WITHDRAW, [](STObject&) {}},
{tecINVARIANT_FAILED, tefINVARIANT_FAILED},
preclose3);
doInvariantCheck(
Env{*this, defaultAmendments() - fixCleanup3_3_0},
{{"invalid MPToken transfer between holders"}},
precheck2,
XRPAmount{},
STTx{ttAMM_WITHDRAW, [](STObject&) {}},
{tecINVARIANT_FAILED, tefINVARIANT_FAILED},
preclose3);
// Case 2: freeze a2's (receiver's) IOU trustline.
// isVaultPseudoAccountFrozen(a2) → isAnyFrozen({vaultPseudo, a2},
// IOU) → true. The AMM sender's check passes (vaultPseudo and
// ammAcct are not frozen). Pre-fix330: receiver is exempt from
// isVaultPseudoAccountFrozen in ttAMM_WITHDRAW → passes.
// Post-fix330: full isFrozen() applied to a2 → fires.
auto const preclose4 = [&](Account const& a1, Account const& a2, Env& env) -> bool {
if (!setupVaultAMM(a1, a2, env))
return false;
env(trust(gw, gw["IOU"](0), a2, tfSetFreeze));
env.close();
return true;
};
doInvariantCheck(
Env{*this, defaultAmendments()},
{{"invalid MPToken transfer between holders"}},
precheck2,
XRPAmount{},
STTx{ttAMM_WITHDRAW, [](STObject&) {}},
{tecINVARIANT_FAILED, tefINVARIANT_FAILED},
preclose4);
doInvariantCheck(
Env{*this, defaultAmendments() - fixCleanup3_3_0},
{},
precheck2,
XRPAmount{},
STTx{ttAMM_WITHDRAW, [](STObject&) {}},
{tesSUCCESS, tesSUCCESS},
preclose4);
}
}
void