From ea8c6ff96f575e2df6110394e35007fe8950e0ca Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 25 Jun 2026 14:34:22 +0200 Subject: [PATCH] fix: Authorize AMM pseudo-accounts holding vault shares 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. --- src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | 31 ++-- src/libxrpl/tx/invariants/MPTInvariant.cpp | 35 ++++- src/test/app/AMMMPT_test.cpp | 52 +++++-- src/test/app/AMM_test.cpp | 1 - src/test/app/Invariants_test.cpp | 143 +++++++++++++++++- 5 files changed, 223 insertions(+), 39 deletions(-) diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index 78475c2b34..9e97869519 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -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))) diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 202518d0f9..ff5683b2b6 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -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))) { diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index ddd0c9248d..9ff6c65c17 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -18,12 +18,12 @@ #include #include #include -#include #include #include #include #include +#include #include #include #include @@ -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(); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index dde60792d0..3672c4a68a 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 6d63b0ebce..2b4a036203 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -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