From 0aa4eed0f7af5bc2d812f81679e4c271fa74e305 Mon Sep 17 00:00:00 2001 From: Jingchen Date: Wed, 17 Dec 2025 04:46:51 +0000 Subject: [PATCH] Check permissions in LoanSet and LoanPay (#6108) --- src/test/app/Loan_test.cpp | 353 +++++++++++++++++++++++++++- src/xrpld/app/tx/detail/LoanPay.cpp | 10 +- src/xrpld/app/tx/detail/LoanSet.cpp | 8 +- 3 files changed, 361 insertions(+), 10 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 61cda5d021..756f7216ee 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -3464,11 +3464,12 @@ protected: ter{tecNO_AUTH}); env.close(); - // Can create loan without origination fee + // Cannot create loan, even without an origination fee env(set(borrower, broker.brokerID, principalRequest), counterparty(lender), sig(sfCounterpartySignature, lender), - fee(env.current()->fees().base * 5)); + fee(env.current()->fees().base * 5), + ter{tecNO_AUTH}); env.close(); // No MPToken for lender - no authorization and no payment @@ -7038,6 +7039,350 @@ protected: paymentParams); } + void + testLoanPayBrokerOwnerMissingTrustline() + { + testcase << "LoanPay Broker Owner Missing Trustline (PoC)"; + using namespace jtx; + using namespace loan; + Account const issuer("issuer"); + Account const borrower("borrower"); + Account const broker("broker"); + auto const IOU = issuer["IOU"]; + Env env(*this, all); + env.fund(XRP(20'000), issuer, broker, borrower); + env.close(); + // Set up trustlines and fund accounts + env(trust(broker, IOU(20'000'000))); + env(trust(borrower, IOU(20'000'000))); + env(pay(issuer, broker, IOU(10'000'000))); + env(pay(issuer, borrower, IOU(1'000))); + env.close(); + // Create vault and broker + auto const brokerInfo = createVaultAndBroker(env, IOU, broker); + // Create a loan first (this creates debt) + auto const keylet = keylet::loan(brokerInfo.brokerID, 1); + env(set(borrower, brokerInfo.brokerID, 10'000), + sig(sfCounterpartySignature, broker), + loanServiceFee(IOU(100).value()), + paymentInterval(100), + fee(XRP(100))); + env.close(); + // Ensure broker has sufficient cover so brokerPayee == brokerOwner + // We need coverAvailable >= (debtTotal * coverRateMinimum) + // Deposit enough cover to ensure the fee goes to broker owner + // The default coverRateMinimum is 10%, so for a 10,000 loan we need + // at least 1,000 cover. Default cover is 1,000, so we add more to be + // safe. + auto const additionalCover = IOU(50'000).value(); + env(loanBroker::coverDeposit( + broker, brokerInfo.brokerID, STAmount{IOU, additionalCover})); + env.close(); + // Verify broker owner has a trustline + auto const brokerTrustline = keylet::line(broker, IOU); + BEAST_EXPECT(env.le(brokerTrustline) != nullptr); + // Broker owner deletes their trustline + // First, pay any positive balance to issuer to zero it out + auto const brokerBalance = env.balance(broker, IOU); + env(pay(broker, issuer, brokerBalance)); + env.close(); + // Remove the trustline by setting limit to 0 + env(trust(broker, IOU(0))); + env.close(); + // Verify trustline is deleted + BEAST_EXPECT(env.le(brokerTrustline) == nullptr); + // Now borrower tries to make a payment + // We should get a tesSUCCESS instead of a tecNO_LINE. + env(pay(borrower, keylet.key, IOU(10'100)), + fee(XRP(100)), + ter(tesSUCCESS)); + env.close(); + // Verify trustline is still deleted + BEAST_EXPECT(env.le(brokerTrustline) == nullptr); + // Verify the service fee went to the broker pseudo-account + if (auto const brokerSle = + env.le(keylet::loanbroker(brokerInfo.brokerID)); + BEAST_EXPECT(brokerSle)) + { + Account const pseudo("pseudo-account", brokerSle->at(sfAccount)); + auto const balance = env.balance(pseudo, IOU); + // 1,000 default + 50,000 extra + 100 service fee from LoanPay + BEAST_EXPECTS( + balance == IOU(51'100), to_string(Json::Value(balance))); + } + } + + void + testLoanPayBrokerOwnerUnauthorizedMPT() + { + testcase << "LoanPay Broker Owner MPT unauthorized"; + using namespace jtx; + using namespace loan; + + Account const issuer("issuer"); + Account const borrower("borrower"); + Account const broker("broker"); + + Env env(*this, all); + env.fund(XRP(20'000), issuer, broker, borrower); + env.close(); + + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + + PrettyAsset const MPT{mptt.issuanceID()}; + + // Authorize broker and borrower + mptt.authorize({.account = broker}); + mptt.authorize({.account = borrower}); + + env.close(); + + // Fund accounts + env(pay(issuer, broker, MPT(10'000'000))); + env(pay(issuer, borrower, MPT(1'000))); + env.close(); + + // Create vault and broker + auto const brokerInfo = createVaultAndBroker(env, MPT, broker); + // Create a loan first (this creates debt) + auto const keylet = keylet::loan(brokerInfo.brokerID, 1); + env(set(borrower, brokerInfo.brokerID, 10'000), + sig(sfCounterpartySignature, broker), + loanServiceFee(MPT(100).value()), + paymentInterval(100), + fee(XRP(100))); + env.close(); + // Ensure broker has sufficient cover so brokerPayee == brokerOwner + // We need coverAvailable >= (debtTotal * coverRateMinimum) + // Deposit enough cover to ensure the fee goes to broker owner + // The default coverRateMinimum is 10%, so for a 10,000 loan we need + // at least 1,000 cover. Default cover is 1,000, so we add more to be + // safe. + auto const additionalCover = MPT(50'000).value(); + env(loanBroker::coverDeposit( + broker, brokerInfo.brokerID, STAmount{MPT, additionalCover})); + env.close(); + // Verify broker owner is authorized + auto const brokerMpt = keylet::mptoken(mptt.issuanceID(), broker); + BEAST_EXPECT(env.le(brokerMpt) != nullptr); + // Broker owner unauthorizes. + // First, pay any positive balance to issuer to zero it out + auto const brokerBalance = env.balance(broker, MPT); + env(pay(broker, issuer, brokerBalance)); + env.close(); + // Then, unauthorize the MPT. + mptt.authorize({.account = broker, .flags = tfMPTUnauthorize}); + env.close(); + // Verify the MPT is unauthorized. + BEAST_EXPECT(env.le(brokerMpt) == nullptr); + // Now borrower tries to make a payment + // We should get a tesSUCCESS instead of a tecNO_AUTH. + auto const borrowerBalance = env.balance(borrower, MPT); + env(pay(borrower, keylet.key, MPT(10'100)), + fee(XRP(100)), + ter(tesSUCCESS)); + env.close(); + // Verify the MPT is still unauthorized. + BEAST_EXPECT(env.le(brokerMpt) == nullptr); + // Verify the service fee went to the broker pseudo-account + if (auto const brokerSle = + env.le(keylet::loanbroker(brokerInfo.brokerID)); + BEAST_EXPECT(brokerSle)) + { + Account const pseudo("pseudo-account", brokerSle->at(sfAccount)); + auto const balance = env.balance(pseudo, MPT); + // 1,000 default + 50,000 extra + 100 service fee from LoanPay + BEAST_EXPECTS( + balance == MPT(51'100), to_string(Json::Value(balance))); + } + } + + void + testLoanPayBrokerOwnerNoPermissionedDomainMPT() + { + testcase + << "LoanPay Broker Owner without permissioned domain of the MPT"; + using namespace jtx; + using namespace loan; + + Account const issuer("issuer"); + Account const borrower("borrower"); + Account const broker("broker"); + + Env env(*this, all); + env.fund(XRP(20'000), issuer, broker, borrower); + env.close(); + + auto credType = "credential1"; + + pdomain::Credentials const credentials1{{issuer, credType}}; + env(pdomain::setTx(issuer, credentials1)); + env.close(); + + auto domainID = pdomain::getNewDomain(env.meta()); + + env(credentials::create(broker, issuer, credType)); + env(credentials::accept(broker, issuer, credType)); + env.close(); + + env(credentials::create(borrower, issuer, credType)); + env(credentials::accept(borrower, issuer, credType)); + env.close(); + + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create({ + .flags = tfMPTCanClawback | tfMPTRequireAuth | tfMPTCanTransfer | + tfMPTCanLock, + .domainID = domainID, + }); + + PrettyAsset const MPT{mptt.issuanceID()}; + + // Authorize broker and borrower + mptt.authorize({.account = broker}); + mptt.authorize({.account = borrower}); + + env.close(); + + // Fund accounts + env(pay(issuer, broker, MPT(10'000'000))); + env(pay(issuer, borrower, MPT(1'000))); + env.close(); + + // Create vault and broker + auto const brokerInfo = createVaultAndBroker(env, MPT, broker); + // Create a loan first (this creates debt) + auto const keylet = keylet::loan(brokerInfo.brokerID, 1); + env(set(borrower, brokerInfo.brokerID, 10'000), + sig(sfCounterpartySignature, broker), + loanServiceFee(MPT(100).value()), + paymentInterval(100), + fee(XRP(100))); + env.close(); + // Ensure broker has sufficient cover so brokerPayee == brokerOwner + // We need coverAvailable >= (debtTotal * coverRateMinimum) + // Deposit enough cover to ensure the fee goes to broker owner + // The default coverRateMinimum is 10%, so for a 10,000 loan we need + // at least 1,000 cover. Default cover is 1,000, so we add more to be + // safe. + auto const additionalCover = MPT(50'000).value(); + env(loanBroker::coverDeposit( + broker, brokerInfo.brokerID, STAmount{MPT, additionalCover})); + env.close(); + // Verify broker owner is authorized + auto const brokerMpt = keylet::mptoken(mptt.issuanceID(), broker); + BEAST_EXPECT(env.le(brokerMpt) != nullptr); + // Remove the credentials for the Broker owner. + // First, pay any positive balance to issuer to zero it out + auto const brokerBalance = env.balance(broker, MPT); + env(pay(broker, issuer, brokerBalance)); + env.close(); + + env(credentials::deleteCred(broker, broker, issuer, credType)); + env.close(); + + // Make sure the broker is not authorized to hold the MPT after we + // deleted the credentials + env(pay(issuer, broker, MPT(1'000)), ter(tecNO_AUTH)); + + // Now borrower tries to make a payment + // We should get a tesSUCCESS instead of a tecNO_AUTH. + auto const borrowerBalance = env.balance(borrower, MPT); + env(pay(borrower, keylet.key, MPT(10'100)), + fee(XRP(100)), + ter(tesSUCCESS)); + env.close(); + // Verify broker is still not authorized + env(pay(issuer, broker, MPT(1'000)), ter(tecNO_AUTH)); + // Verify the service fee went to the broker pseudo-account + if (auto const brokerSle = + env.le(keylet::loanbroker(brokerInfo.brokerID)); + BEAST_EXPECT(brokerSle)) + { + Account const pseudo("pseudo-account", brokerSle->at(sfAccount)); + auto const balance = env.balance(pseudo, MPT); + // 1,000 default + 50,000 extra + 100 service fee from LoanPay + BEAST_EXPECTS( + balance == MPT(51'100), to_string(Json::Value(balance))); + } + } + + void + testLoanSetBrokerOwnerNoPermissionedDomainMPT() + { + testcase + << "LoanSet Broker Owner without permissioned domain of the MPT"; + using namespace jtx; + using namespace loan; + + Account const issuer("issuer"); + Account const borrower("borrower"); + Account const broker("broker"); + + Env env(*this, all); + env.fund(XRP(20'000), issuer, broker, borrower); + env.close(); + + auto credType = "credential1"; + + pdomain::Credentials const credentials1{{issuer, credType}}; + env(pdomain::setTx(issuer, credentials1)); + env.close(); + + auto domainID = pdomain::getNewDomain(env.meta()); + + // Add credentials for the broker and borrower + env(credentials::create(broker, issuer, credType)); + env(credentials::accept(broker, issuer, credType)); + env.close(); + + env(credentials::create(borrower, issuer, credType)); + env(credentials::accept(borrower, issuer, credType)); + env.close(); + + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create({ + .flags = tfMPTCanClawback | tfMPTRequireAuth | tfMPTCanTransfer | + tfMPTCanLock, + .domainID = domainID, + }); + + PrettyAsset const MPT{mptt.issuanceID()}; + + // Authorize broker and borrower + mptt.authorize({.account = broker}); + mptt.authorize({.account = borrower}); + env.close(); + + // Fund accounts + env(pay(issuer, broker, MPT(10'000'000))); + env(pay(issuer, borrower, MPT(1'000))); + env.close(); + + // Create vault and broker + auto const brokerInfo = createVaultAndBroker(env, MPT, broker); + + // Remove the credentials for the Broker owner. + // Clear the balance first. + auto const brokerBalance = env.balance(broker, MPT); + env(pay(broker, issuer, brokerBalance)); + env.close(); + // Delete the credentials + env(credentials::deleteCred(broker, broker, issuer, credType)); + env.close(); + + // Create a loan, this should fail for tecNO_AUTH + env(set(borrower, brokerInfo.brokerID, 10'000), + sig(sfCounterpartySignature, broker), + loanServiceFee(MPT(100).value()), + paymentInterval(100), + fee(XRP(100)), + ter(tecNO_AUTH)); + env.close(); + } + public: void run() override @@ -7086,6 +7431,10 @@ public: testBorrowerIsBroker(); testIssuerIsBorrower(); testLimitExceeded(); + testLoanPayBrokerOwnerMissingTrustline(); + testLoanPayBrokerOwnerUnauthorizedMPT(); + testLoanPayBrokerOwnerNoPermissionedDomainMPT(); + testLoanSetBrokerOwnerNoPermissionedDomainMPT(); } }; diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 27aec66d92..7dbec5a351 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -262,9 +262,10 @@ LoanPay::doApply() auto debtTotalProxy = brokerSle->at(sfDebtTotal); // Send the broker fee to the owner if they have sufficient cover available, - // _and_ if the owner can receive funds. If not, so as not to block the - // payment, add it to the cover balance (send it to the broker pseudo - // account). + // _and_ if the owner can receive funds + // _and_ if the broker is authorized to hold funds. If not, so as not to + // block the payment, add it to the cover balance (send it to the broker + // pseudo account). // // Normally freeze status is checked in preflight, but we do it here to // avoid duplicating the check. It'll claim a fee either way. @@ -278,7 +279,8 @@ LoanPay::doApply() asset, tenthBipsOfValue(debtTotalProxy.value(), coverRateMinimum), loanScale) && - !isDeepFrozen(view, brokerOwner, asset); + !isDeepFrozen(view, brokerOwner, asset) && + !requireAuth(view, asset, brokerOwner, AuthType::StrongAuth); }(); auto const brokerPayee = diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index c7253472ee..b1ed8f968f 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -538,12 +538,12 @@ LoanSet::doApply() // ignore tecDUPLICATE. That means the holding already exists, // and is fine here return ter; - - if (auto const ter = requireAuth( - view, vaultAsset, brokerOwner, AuthType::StrongAuth)) - return ter; } + if (auto const ter = + requireAuth(view, vaultAsset, brokerOwner, AuthType::StrongAuth)) + return ter; + if (auto const ter = accountSendMulti( view, vaultPseudo,