Polish the code

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

src/test/app/Loan_test.cpp

@@ -7028,6 +7028,140 @@ 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();
+    }
+
+    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();
+    }
+
 public:
     void
     run() override
@@ -7076,6 +7210,8 @@ public:
         testBorrowerIsBroker();
         testIssuerIsBorrower();
         testLimitExceeded();
+        testLoanPayBrokerOwnerMissingTrustline();
+        testLoanPayBrokerOwnerUnauthorizedMPT();
     }
 };
 

src/test/app/Vault_test.cpp

@@ -5243,46 +5243,6 @@ class Vault_test : public beast::unit_test::suite
         });
     }
 
-    void
-    testFrozenWithdrawToIssuer()
-    {
-        using namespace test::jtx;
-
-        testcase("frozen IOU can be withdrawn to issuer");
-
-        Env env{*this, testable_amendments() | featureSingleAssetVault};
-        Account issuer{"issuer"};
-        Account owner{"owner"};
-        Account depositor{"depositor"};
-        env.fund(XRP(1000), issuer, owner, depositor);
-        env.close();
-
-        PrettyAsset asset = issuer["IOU"];
-        env.trust(asset(1000), owner);
-        env.trust(asset(1000), depositor);
-        env(pay(issuer, owner, asset(100)));
-        env(pay(issuer, depositor, asset(200)));
-        env.close();
-
-        Vault vault{env};
-        auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
-        env(tx);
-        env.close();
-
-        env(vault.deposit(
-            {.depositor = depositor, .id = keylet.key, .amount = asset(50)}));
-        env.close();
-
-        env(fset(issuer, asfGlobalFreeze));
-        env.close();
-
-        auto withdraw = vault.withdraw(
-            {.depositor = depositor, .id = keylet.key, .amount = asset(10)});
-        withdraw[sfDestination] = issuer.human();
-        env(withdraw, ter{tesSUCCESS});
-        env.close();
-    }
-
 public:
     void
     run() override
@@ -5301,7 +5261,6 @@ public:
         testScaleIOU();
         testRPC();
         testDelegate();
-        testFrozenWithdrawToIssuer();
     }
 };
 

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
+    // _and_ if the owner can receive funds
-    // payment, add it to the cover balance (send it to the broker pseudo
+    // _and_ if the broker is authorized to hold funds. If not, so as not to
-    // account).
+    // 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,9 @@ LoanPay::doApply()
                    asset,
                    tenthBipsOfValue(debtTotalProxy.value(), coverRateMinimum),
                    loanScale) &&
-            !isDeepFrozen(view, brokerOwner, asset);
+            !isDeepFrozen(view, brokerOwner, asset) &&
+            requireAuth(view, asset, brokerOwner, AuthType::StrongAuth) ==
+            tesSUCCESS;
     }();
 
     auto const brokerPayee =

src/xrpld/app/tx/detail/VaultWithdraw.cpp

@@ -80,23 +80,13 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         return ter;
 
     // Cannot withdraw from a Vault an Asset frozen for the destination account
-    if (!vaultAsset.holds<Issue>() ||
+    if (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset))
-        (dstAcct != vaultAsset.getIssuer() &&
+        return ret;
-         account != vaultAsset.getIssuer()))
-    {
-        if (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset))
-            return ret;
-    }
 
     // Cannot return shares to the vault, if the underlying asset was frozen for
     // the submitter
-    if (!vaultAsset.holds<Issue>() ||
+    if (auto const ret = checkFrozen(ctx.view, account, vaultShare))
-        (dstAcct != vaultAsset.getIssuer() &&
+        return ret;
-         account != vaultAsset.getIssuer()))
-    {
-        if (auto const ret = checkFrozen(ctx.view, account, vaultShare))
-            return ret;
-    }
 
     return tesSUCCESS;
 }
@@ -125,7 +115,6 @@ VaultWithdraw::doApply()
 
     auto const amount = ctx_.tx[sfAmount];
     Asset const vaultAsset = vault->at(sfAsset);
-    auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);
     MPTIssue const share{mptIssuanceID};
     STAmount sharesRedeemed = {share};
     STAmount assetsWithdrawn;
@@ -176,21 +165,11 @@ VaultWithdraw::doApply()
         return tecPATH_DRY;
     }
 
-    // When withdrawing IOU to the issuer, ignore freeze since spec allows
-    // returning frozen IOU assets to their issuer. MPTs don't have this
-    // exemption - MPT locks function like "deep freeze" with no issuer
-    // exception.
-    FreezeHandling const freezeHandling = (vaultAsset.holds<Issue>() &&
-                                           (dstAcct == vaultAsset.getIssuer() ||
-                                            account_ == vaultAsset.getIssuer()))
-        ? FreezeHandling::fhIGNORE_FREEZE
-        : FreezeHandling::fhZERO_IF_FROZEN;
-
     if (accountHolds(
             view(),
             account_,
             share,
-            freezeHandling,
+            FreezeHandling::fhZERO_IF_FROZEN,
             AuthHandling::ahIGNORE_AUTH,
             j_) < sharesRedeemed)
     {
@@ -258,6 +237,8 @@ VaultWithdraw::doApply()
         // else quietly ignore, account balance is not zero
     }
 
+    auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);
+
     return doWithdraw(
         view(),
         ctx_.tx,