diff --git a/include/xrpl/tx/invariants/MPTInvariant.h b/include/xrpl/tx/invariants/MPTInvariant.h index 5740cd5be2..ddda348d2e 100644 --- a/include/xrpl/tx/invariants/MPTInvariant.h +++ b/include/xrpl/tx/invariants/MPTInvariant.h @@ -215,6 +215,13 @@ class ValidMPTTransfer // Deleted MPToken // MPToken key: true if MPTAuthorized is set hash_map deletedAuthorized_; + // Every touched AccountRoot (not only pseudos): + // AccountID -> whether it was a pseudo-account BEFORE this transaction + // applied. Needed because a transaction may erase a pseudo-account and + // move MPT out of it in the same transaction; by finalize() time the + // view no longer shows it as a pseudo-account (or as existing at all). + // False entries freeze the pre-tx classification for touched non-pseudos. + hash_map pseudoAccountsBefore_; public: /** diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 66b9028ed2..09b4308165 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -832,6 +832,14 @@ ValidMPTTransfer::visitEntry( if (after) update(*after, false); + + // Record whether every touched AccountRoot was a pseudo-account BEFORE + // the transaction applied (true and false). A transaction that erases a + // pseudo-account (and moves MPT out of it) in the same transaction leaves + // no trace of its pseudo-account status in the post-transaction view + // isAuthorized() sees at finalize() time. + if (before && before->getType() == ltACCOUNT_ROOT) + pseudoAccountsBefore_[before->at(sfAccount)] = isPseudoAccount(before); } bool @@ -844,10 +852,19 @@ ValidMPTTransfer::isAuthorized( // 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 + // 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)) + // + // Use the pre-transaction classification for any account this + // transaction touched (pseudoAccountsBefore_): the post-transaction view + // is wrong for an account this same transaction erased. Untouched + // accounts aren't in the map, so fall back to the current view, which is + // still accurate for them since nothing changed. + auto const pseudoIt = pseudoAccountsBefore_.find(holder); + bool const isPseudo = + pseudoIt != pseudoAccountsBefore_.end() ? pseudoIt->second : isPseudoAccount(view, holder); + if (isPseudo) return true; auto const key = keylet::mptoken(mptid, holder); diff --git a/src/test/app/lending/LoanBroker_test.cpp b/src/test/app/lending/LoanBroker_test.cpp index d75b359868..5b3ea854f8 100644 --- a/src/test/app/lending/LoanBroker_test.cpp +++ b/src/test/app/lending/LoanBroker_test.cpp @@ -1849,6 +1849,96 @@ class LoanBroker_test : public beast::unit_test::Suite BEAST_EXPECT(aliceBalanceAfter == aliceBalanceBefore); } + void + testLoanBrokerDeleteRequireAuthMPT(FeatureBitset features) + { + testcase << "LoanBrokerDelete - auth-required broker pseudo-account MPT " + << (features[fixCleanup3_4_0] ? "post-fix" : "pre-fix"); + using namespace jtx; + using namespace loan_broker; + + Account const issuer("issuer"); + Account const alice("alice"); + + Env env(*this, features); + env.fund(XRP(100'000), issuer, alice); + env.close(); + + // Create an auth-required MPT and authorize alice as a holder. The + // broker pseudo-account's cover MPToken is auto-created later + // (addEmptyHolding -> authorizeMPToken) with lsfMPTAuthorized clear; + // the pseudo-account is implicitly authorized to hold any MPT + // regardless of that flag. + auto tester = MPTTester( + {.env = env, + .issuer = issuer, + .holders = {alice}, + .pay = 20'000, + .flags = tfMPTRequireAuth | tfMPTCanTransfer, + .authHolder = true}); + + PrettyAsset const mpt{tester.issuanceID()}; + + // Create vault + Vault const vault{env}; + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = mpt}); + env(tx); + env.close(); + + // Deposit into vault + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = mpt(10'000)})); + env.close(); + + // Create loan broker + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice, vaultKeylet.key)); + env.close(); + + // Deposit cover + env(coverDeposit(alice, brokerKeylet.key, mpt(5'000).value())); + env.close(); + + // Verify cover is deposited + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + BEAST_EXPECT(broker->at(sfCoverAvailable) > 0); + + // Get the broker pseudo-account + auto const brokerPseudoID = broker->at(sfAccount); + + // Verify the broker pseudo-account has an MPToken, and that it was + // never explicitly authorized (issuer cannot authorize a + // pseudo-account holder; see MPTokenAuthorize::preclaim). + auto const pseudoMptKey = keylet::mptoken(tester.issuanceID(), brokerPseudoID); + auto const pseudoMpt = env.le(pseudoMptKey); + if (!BEAST_EXPECT(pseudoMpt)) + return; + BEAST_EXPECT(!pseudoMpt->isFlag(lsfMPTAuthorized)); + + // Record alice's balance before deletion + auto const aliceBalanceBefore = env.balance(alice, mpt); + + // LoanBrokerDelete sends the remaining cover out of the broker pseudo-account, deletes its + // now-empty MPToken, and erases the pseudo AccountRoot. Before the fix, + // ValidMPTTransfer::isAuthorized evaluates isPseudoAccount() on the post-transaction view + // (where the pseudo-account is already gone) and falls back to the MPToken's + // lsfMPTAuthorized flag, which was never set, so the invariant treats the broker as an + // unauthorized sender and the whole transaction fails once fixCleanup3_4_0 makes the check + // enforcing. + env(del(alice, brokerKeylet.key), Ter(tesSUCCESS)); + env.close(); + + // Broker and its pseudo-account MPToken are gone + BEAST_EXPECT(env.le(brokerKeylet) == nullptr); + BEAST_EXPECT(env.le(pseudoMptKey) == nullptr); + + // Alice received the cover + auto const aliceBalanceAfter = env.balance(alice, mpt); + BEAST_EXPECT(aliceBalanceAfter > aliceBalanceBefore); + } + void testCoverDepositFreezes() { @@ -2550,7 +2640,7 @@ class LoanBroker_test : public beast::unit_test::Suite using namespace jtx; using namespace std::chrono_literals; - bool const fixEnabled = features[fixCleanup3_4_0]; + bool const fix340Enabled = features[fixCleanup3_4_0]; Env env(*this, features); @@ -2611,7 +2701,7 @@ class LoanBroker_test : public beast::unit_test::Suite env(coverWithdrawToDest(), loan_broker::kDestination(dest), Ter{tecNO_PERMISSION}); env.close(); - if (!fixEnabled) + if (!fix340Enabled) { // Pre-fix: sfCredentialIDs in LoanBrokerCoverWithdraw is disabled env(coverWithdrawToDest(), @@ -3035,6 +3125,13 @@ public: testLoanBrokerDeleteFrozenIOU(all_); testLoanBrokerDeleteFrozenIOU(all_ - fixCleanup3_2_0); + + // featureMPTokensV2 independently makes ValidMPTTransfer enforcing, + // but it's Supported::No (never enabled on real networks); exclude + // it here so fixCleanup3_4_0 alone is the deciding amendment, as it + // would be on mainnet. + testLoanBrokerDeleteRequireAuthMPT(all_ - featureMPTokensV2); + testLoanBrokerDeleteRequireAuthMPT(all_ - featureMPTokensV2 - fixCleanup3_4_0); // TODO: Write clawback failure tests with an issuer / MPT that doesn't // have the right flags set. }