diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index c31502be98..ece574e52d 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -654,14 +654,17 @@ createPseudoAccount( uint256 const& pseudoOwnerKey, SField const& ownerField); -// Returns true iff sleAcct is a pseudo-account. +// Returns true iff sleAcct is a pseudo-account or specific +// pseudo-accounts in pseudoFieldFilter. // // Returns false if sleAcct is // * NOT a pseudo-account OR // * NOT a ltACCOUNT_ROOT OR // * null pointer [[nodiscard]] bool -isPseudoAccount(std::shared_ptr sleAcct); +isPseudoAccount( + std::shared_ptr sleAcct, + std::set const& pseudoFieldFilter = {}); // Returns the list of fields that define an ACCOUNT_ROOT as a pseudo-account if // set @@ -675,9 +678,13 @@ isPseudoAccount(std::shared_ptr sleAcct); getPseudoAccountFields(); [[nodiscard]] inline bool -isPseudoAccount(ReadView const& view, AccountID const& accountId) +isPseudoAccount( + ReadView const& view, + AccountID const& accountId, + std::set const& pseudoFieldFilter = {}) { - return isPseudoAccount(view.read(keylet::account(accountId))); + return isPseudoAccount( + view.read(keylet::account(accountId)), pseudoFieldFilter); } [[nodiscard]] TER @@ -1001,7 +1008,8 @@ requireAuth( * purely defensive, as we currently do not allow such vaults to be created. * * If StrongAuth then return tecNO_AUTH if MPToken doesn't exist or - * lsfMPTRequireAuth is set and MPToken is not authorized. + * lsfMPTRequireAuth is set and MPToken is not authorized. Vault and LoanBroker + * pseudo-accounts are implicitly authorized. * * If WeakAuth then return tecNO_AUTH if lsfMPTRequireAuth is set and MPToken * doesn't exist or is not authorized (explicitly or via credentials, if diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 78e1663773..1e57bc663b 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -1204,7 +1204,8 @@ getPseudoAccountFields() { // LCOV_EXCL_START LogicError( - "ripple::isPseudoAccount : unable to find account root ledger " + "ripple::getPseudoAccountFields : unable to find account root " + "ledger " "format"); // LCOV_EXCL_STOP } @@ -1222,7 +1223,9 @@ getPseudoAccountFields() } [[nodiscard]] bool -isPseudoAccount(std::shared_ptr sleAcct) +isPseudoAccount( + std::shared_ptr sleAcct, + std::set const& pseudoFieldFilter) { auto const& fields = getPseudoAccountFields(); @@ -1230,8 +1233,12 @@ isPseudoAccount(std::shared_ptr sleAcct) // semantics of true return value clean. return sleAcct && sleAcct->getType() == ltACCOUNT_ROOT && std::count_if( - fields.begin(), fields.end(), [&sleAcct](SField const* sf) -> bool { - return sleAcct->isFieldPresent(*sf); + fields.begin(), + fields.end(), + [&sleAcct, &pseudoFieldFilter](SField const* sf) -> bool { + return sleAcct->isFieldPresent(*sf) && + (pseudoFieldFilter.empty() || + pseudoFieldFilter.contains(sf)); }) > 0; } @@ -3099,7 +3106,10 @@ requireAuth( if (mptIssuer == account) // Issuer won't have MPToken return tesSUCCESS; - if (view.rules().enabled(featureSingleAssetVault)) + bool const featureSAVEnabled = + view.rules().enabled(featureSingleAssetVault); + + if (featureSAVEnabled) { if (depth >= maxAssetCheckDepth) return tecINTERNAL; // LCOV_EXCL_LINE @@ -3158,6 +3168,13 @@ requireAuth( // belong to someone who is explicitly authorized e.g. a vault owner. } + if (featureSAVEnabled) + { + // Implicitly authorize Vault and LoanBroker pseudo-accounts + if (isPseudoAccount(view, account, {&sfVaultID, &sfLoanBrokerID})) + return tesSUCCESS; + } + // mptoken must be authorized if issuance enabled requireAuth if (sleIssuance->isFlag(lsfMPTRequireAuth) && (!sleToken || !sleToken->isFlag(lsfMPTAuthorized))) diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 171b699bbf..ff70290331 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1265,6 +1265,126 @@ class LoanBroker_test : public beast::unit_test::suite (void)LoanBrokerCoverDeposit::preclaim(pctx); } + void + testRequireAuth() + { + testcase("Require Auth - Implicit Pseudo-account authorization"); + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Env env(*this); + Vault vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + + auto asset = MPTTester({ + .env = env, + .issuer = issuer, + .holders = {alice}, + .flags = MPTDEXFlags | tfMPTRequireAuth | tfMPTCanClawback | + tfMPTCanLock, + .authHolder = true, + }); + + env(pay(issuer, alice, asset(100'000))); + env.close(); + + // Alice is not authorized, can still create the vault + asset.authorize( + {.account = issuer, .holder = alice, .flags = tfMPTUnauthorize}); + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); + env(tx); + env.close(); + + auto const le = env.le(vaultKeylet); + VaultInfo vaultInfo = [&]() { + if (BEAST_EXPECT(le)) + return VaultInfo{asset, vaultKeylet.key, le->at(sfAccount)}; + return VaultInfo{asset, {}, {}}; + }(); + if (vaultInfo.vaultID == uint256{}) + return; + + // Can't unauthorize Vault pseudo-account + asset.authorize( + {.account = issuer, + .holder = vaultInfo.pseudoAccount, + .flags = tfMPTUnauthorize, + .err = tecNO_PERMISSION}); + + auto forUnauthAuth = [&](auto&& doTx) { + for (auto const flag : {tfMPTUnauthorize, 0u}) + { + asset.authorize( + {.account = issuer, .holder = alice, .flags = flag}); + env.close(); + doTx(flag == 0); + env.close(); + } + }; + + // Can't deposit into Vault if the vault owner is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(vault.deposit( + {.depositor = alice, + .id = vaultKeylet.key, + .amount = asset(51)}), + err); + }); + + // Can't withdraw from Vault if the vault owner is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(vault.withdraw( + {.depositor = alice, + .id = vaultKeylet.key, + .amount = asset(1)}), + err); + }); + + auto const brokerKeylet = + keylet::loanbroker(alice.id(), env.seq(alice)); + // Can create LoanBroker if the vault owner is not authorized + forUnauthAuth([&](auto) { env(set(alice, vaultInfo.vaultID)); }); + + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Account brokerPseudo("pseudo", broker->at(sfAccount)); + + // Can't unauthorize LoanBroker pseudo-account + asset.authorize( + {.account = issuer, + .holder = brokerPseudo, + .flags = tfMPTUnauthorize, + .err = tecNO_PERMISSION}); + + // Can't cover deposit into Vault if the vault owner is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(coverDeposit(alice, brokerKeylet.key, vaultInfo.asset(10)), + err); + }); + + // Can't cover withdraw from Vault if the vault owner is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(coverWithdraw(alice, brokerKeylet.key, vaultInfo.asset(5)), + err); + }); + + // Issuer can always cover clawback. The holder authorization is n/a. + forUnauthAuth([&](bool) { + env(coverClawback(issuer), + loanBrokerID(brokerKeylet.key), + amount(vaultInfo.asset(1))); + }); + } + public: void run() override @@ -1278,6 +1398,7 @@ public: testInvalidLoanBrokerCoverWithdraw(); testInvalidLoanBrokerDelete(); testInvalidLoanBrokerSet(); + testRequireAuth(); // TODO: Write clawback failure tests with an issuer / MPT that doesn't // have the right flags set. diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 575d687dfa..67bc49f076 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -5333,6 +5333,65 @@ protected: } } + void + testRequireAuth() + { + testcase("Require Auth - Implicit Pseudo-account authorization"); + using namespace jtx; + using namespace loan; + Account const lender{"lender"}; + Account const issuer{"issuer"}; + Account const borrower{"borrower"}; + Env env(*this); + + env.fund(XRP(100'000), issuer, lender, borrower); + env.close(); + + auto asset = MPTTester({ + .env = env, + .issuer = issuer, + .holders = {lender, borrower}, + .flags = MPTDEXFlags | tfMPTRequireAuth | tfMPTCanClawback | + tfMPTCanLock, + .authHolder = true, + }); + + env(pay(issuer, lender, asset(5'000'000))); + BrokerInfo brokerInfo{createVaultAndBroker(env, asset, lender)}; + + auto const loanSetFee = fee(env.current()->fees().base * 2); + STAmount const debtMaximumRequest = brokerInfo.asset(1'000).value(); + + auto forUnauthAuth = [&](auto&& doTx) { + for (auto const flag : {tfMPTUnauthorize, 0u}) + { + asset.authorize( + {.account = issuer, .holder = borrower, .flags = flag}); + env.close(); + doTx(flag == 0); + env.close(); + } + }; + + // Can't create a loan if the borrower is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(set(borrower, brokerInfo.brokerID, debtMaximumRequest), + sig(sfCounterpartySignature, lender), + loanSetFee, + err); + }); + + std::uint32_t constexpr loanSequence = 1; + auto const loanKeylet = keylet::loan(brokerInfo.brokerID, loanSequence); + + // Can't loan pay if the borrower is not authorized + forUnauthAuth([&](bool authorized) { + auto const err = !authorized ? ter(tecNO_AUTH) : ter(tesSUCCESS); + env(pay(borrower, loanKeylet.key, debtMaximumRequest), err); + }); + } + #if LOANTODO void testCoverDepositAllowsNonTransferableMPT() @@ -6193,6 +6252,8 @@ public: testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(); testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(); testLoanNextPaymentDueDateOverflow(); + + testRequireAuth(); } }; diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp index e9ba2cda64..25075582c5 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp @@ -65,6 +65,10 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) // Pseudo-account cannot receive if asset is deep frozen if (auto const ret = checkDeepFrozen(ctx.view, pseudoAccountID, vaultAsset)) return ret; + // Cannot transfer unauthorized asset + if (auto const ret = + requireAuth(ctx.view, vaultAsset, account, AuthType::StrongAuth)) + return ret; if (accountHolds( ctx.view, diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index d99d2f4544..3edc109c80 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -187,6 +187,11 @@ LoanPay::preclaim(PreclaimContext const& ctx) << "Vault pseudo-account can not receive funds (deep frozen)."; return ret; } + if (auto const ret = requireAuth(ctx.view, asset, account)) + { + JLOG(ctx.j.warn()) << "Borrower account is not authorized."; + return ret; + } // Make sure the borrower has enough funds to make the payment! // Do not support "partial payments" - if the transaction says to pay X, // then the account must have X available, even if the loan payment takes diff --git a/src/xrpld/app/tx/detail/MPTokenAuthorize.cpp b/src/xrpld/app/tx/detail/MPTokenAuthorize.cpp index 858fd6d0d6..b6eaacb372 100644 --- a/src/xrpld/app/tx/detail/MPTokenAuthorize.cpp +++ b/src/xrpld/app/tx/detail/MPTokenAuthorize.cpp @@ -94,7 +94,8 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } - if (!ctx.view.exists(keylet::account(*holderID))) + auto const sleHolder = ctx.view.read(keylet::account(*holderID)); + if (!sleHolder) return tecNO_DST; auto const sleMptIssuance = @@ -124,6 +125,12 @@ MPTokenAuthorize::preclaim(PreclaimContext const& ctx) keylet::mptoken(ctx.tx[sfMPTokenIssuanceID], *holderID))) return tecOBJECT_NOT_FOUND; + // Can't unauthorize the pseudo-accounts because they are implicitly + // always authorized. No need to amendment gate since Vault and LoanBroker + // can only be created if the Vault amendment is enabled. + if (isPseudoAccount(ctx.view, *holderID, {&sfVaultID, &sfLoanBrokerID})) + return tecNO_PERMISSION; + return tesSUCCESS; }