fix: Add assorted Lending Protocol fixes (#6678)

Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>

.github/workflows/publish-docs.yml

@@ -4,17 +4,6 @@ name: Build and publish documentation
 
 on:
   push:
-    branches:
-      - "develop"
-    paths:
-      - ".github/workflows/publish-docs.yml"
-      - "*.md"
-      - "**/*.md"
-      - "docs/**"
-      - "include/**"
-      - "src/libxrpl/**"
-      - "src/xrpld/**"
-  pull_request:
     paths:
       - ".github/workflows/publish-docs.yml"
       - "*.md"
@@ -33,26 +22,21 @@ defaults:
     shell: bash
 
 env:
-  BUILD_DIR: build
-  # ubuntu-latest has only 2 CPUs for private repositories
-  # https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for--private-repositories
-  NPROC_SUBTRACT: ${{ github.event.repository.visibility == 'public' && '2' || '1' }}
+  BUILD_DIR: .build
+  NPROC_SUBTRACT: 2
 
 jobs:
-  build:
+  publish:
     runs-on: ubuntu-latest
     container: ghcr.io/xrplf/ci/tools-rippled-documentation:sha-a8c7be1
+    permissions:
+      contents: write
     steps:
       - name: Checkout repository
-        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
-
-      - name: Prepare runner
-        uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab
-        with:
-          enable_ccache: false
+        uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
 
       - name: Get number of processors
-        uses: XRPLF/actions/get-nproc@cf0433aa74563aead044a1e395610c96d65a37cf
+        uses: XRPLF/actions/.github/actions/get-nproc@046b1620f6bfd6cd0985dc82c3df02786801fe0a
         id: nproc
         with:
           subtract: ${{ env.NPROC_SUBTRACT }}
@@ -80,23 +64,9 @@ jobs:
           cmake -Donly_docs=ON ..
           cmake --build . --target docs --parallel ${BUILD_NPROC}
 
-      - name: Create documentation artifact
-        if: ${{ github.event.repository.visibility == 'public' && github.event_name == 'push' }}
-        uses: actions/upload-pages-artifact@7b1f4a764d45c48632c6b24a0339c27f5614fb0b # v4.0.0
+      - name: Publish documentation
+        if: ${{ github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch }}
+        uses: peaceiris/actions-gh-pages@4f9cc6602d3f66b9c108549d475ec49e8ef4d45e # v4.0.0
         with:
-          path: ${{ env.BUILD_DIR }}/docs/html
-
-  deploy:
-    if: ${{ github.repository == 'XRPLF/rippled' && github.event_name == 'push' }}
-    needs: build
-    runs-on: ubuntu-latest
-    permissions:
-      pages: write
-      id-token: write
-    environment:
-      name: github-pages
-      url: ${{ steps.deploy.outputs.page_url }}
-    steps:
-      - name: Deploy to GitHub Pages
-        id: deploy
-        uses: actions/deploy-pages@cd2ce8fcbc39b97be8ca5fce6e763baed58fa128 # v5.0.0
+          github_token: ${{ secrets.GITHUB_TOKEN }}
+          publish_dir: ${{ env.BUILD_DIR }}/docs/html

.pre-commit-config.yaml

@@ -15,6 +15,7 @@ repos:
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer
+      - id: mixed-line-ending
       - id: check-merge-conflict
         args: [--assume-in-merge]
 
@@ -29,7 +30,6 @@ repos:
     rev: 5ba47274f9b181bce26a5150a725577f3c336011 # frozen: v3.6.2
     hooks:
       - id: prettier
-        args: [--end-of-line=auto]
 
 exclude: |
   (?x)^(

src/libxrpl/ledger/View.cpp

@@ -1559,7 +1559,9 @@ authorizeMPToken(
         {
             auto const mptokenKey = keylet::mptoken(mptIssuanceID, account);
             auto const sleMpt = view.peek(mptokenKey);
-            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
+            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0 ||
+                (view.rules().enabled(fixSecurity3_1_3) &&
+                 (*sleMpt)[~sfLockedAmount].value_or(0) != 0))
                 return tecINTERNAL;  // LCOV_EXCL_LINE
 
             if (!view.dirRemove(
@@ -1868,7 +1870,9 @@ removeEmptyHolding(
     // balance, it can not just be deleted, because that will throw the issuance
     // accounting out of balance, so fail. Since this should be impossible
     // anyway, I'm not going to put any effort into it.
-    if (mptoken->at(sfMPTAmount) != 0)
+    if (mptoken->at(sfMPTAmount) != 0 ||
+        (view.rules().enabled(fixSecurity3_1_3) &&
+         (*mptoken)[~sfLockedAmount].value_or(0) != 0))
         return tecHAS_OBLIGATIONS;
 
     return authorizeMPToken(

src/test/app/Invariants_test.cpp

@@ -1939,39 +1939,41 @@ class Invariants_test : public beast::unit_test::suite
         {
             // Initialize with a placeholder value because there's no default
             // ctor
-            Keylet loanBrokerKeylet = keylet::amendments();
-            Preclose createLoanBroker = [&, this](
-                                            Account const& alice,
-                                            Account const& issuer,
-                                            Env& env) {
-                PrettyAsset const asset = [&]() {
-                    switch (assetType)
-                    {
-                        case Asset::IOU: {
-                            PrettyAsset const iouAsset = issuer["IOU"];
-                            env(trust(alice, iouAsset(1000)));
-                            env(pay(issuer, alice, iouAsset(1000)));
-                            env.close();
-                            return iouAsset;
-                        }
-
-                        case Asset::MPT: {
-                            MPTTester mptt{env, issuer, mptInitNoFund};
-                            mptt.create(
-                                {.flags = tfMPTCanClawback | tfMPTCanTransfer |
-                                     tfMPTCanLock});
-                            PrettyAsset const mptAsset = mptt.issuanceID();
-                            mptt.authorize({.account = alice});
-                            env(pay(issuer, alice, mptAsset(1000)));
-                            env.close();
-                            return mptAsset;
-                        }
-
-                        case Asset::XRP:
-                        default:
-                            return PrettyAsset{xrpIssue(), 1'000'000};
+            auto const setupAsset = [&](Account const& alice,
+                                        Account const& issuer,
+                                        Env& env) -> PrettyAsset {
+                switch (assetType)
+                {
+                    case Asset::IOU: {
+                        PrettyAsset const iouAsset = issuer["IOU"];
+                        env(trust(alice, iouAsset(1000)));
+                        env(pay(issuer, alice, iouAsset(1000)));
+                        env.close();
+                        return iouAsset;
                     }
-                }();
+                    case Asset::MPT: {
+                        MPTTester mptt{env, issuer, mptInitNoFund};
+                        mptt.create(
+                            {.flags = tfMPTCanClawback | tfMPTCanTransfer |
+                                 tfMPTCanLock});
+                        PrettyAsset const mptAsset = mptt.issuanceID();
+                        mptt.authorize({.account = alice});
+                        env(pay(issuer, alice, mptAsset(1000)));
+                        env.close();
+                        return mptAsset;
+                    }
+                    case Asset::XRP:
+                    default:
+                        return PrettyAsset{xrpIssue(), 1'000'000};
+                }
+            };
+
+            Keylet loanBrokerKeylet = keylet::amendments();
+            Preclose const createLoanBroker = [&, this](
+                                                  Account const& alice,
+                                                  Account const& issuer,
+                                                  Env& env) {
+                auto const asset = setupAsset(alice, issuer, env);
                 loanBrokerKeylet = this->createLoanBroker(alice, env, asset);
                 return BEAST_EXPECT(env.le(loanBrokerKeylet));
             };
@@ -2159,6 +2161,61 @@ class Invariants_test : public beast::unit_test::suite
                 STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}},
                 {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
                 createLoanBroker);
+
+            // Test: cover available less than pseudo-account asset balance
+            {
+                Keylet brokerKeylet = keylet::amendments();
+                Preclose const createBrokerWithCover =
+                    [&, this](
+                        Account const& alice, Account const& issuer, Env& env) {
+                        auto const asset = setupAsset(alice, issuer, env);
+                        brokerKeylet =
+                            this->createLoanBroker(alice, env, asset);
+                        if (!BEAST_EXPECT(env.le(brokerKeylet)))
+                            return false;
+                        env(loanBroker::coverDeposit(
+                            alice, brokerKeylet.key, asset(10)));
+                        env.close();
+                        return BEAST_EXPECT(env.le(brokerKeylet));
+                    };
+
+                doInvariantCheck(
+                    {{"Loan Broker cover available is less than pseudo-account "
+                      "asset balance"}},
+                    [&](Account const&, Account const&, ApplyContext& ac) {
+                        auto sle = ac.view().peek(brokerKeylet);
+                        if (!BEAST_EXPECT(sle))
+                            return false;
+                        // Pseudo-account holds 10 units, set cover to 5
+                        sle->at(sfCoverAvailable) = Number(5);
+                        ac.view().update(sle);
+                        return true;
+                    },
+                    XRPAmount{},
+                    STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}},
+                    {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
+                    createBrokerWithCover);
+            }
+
+            // Test: cover available greater than pseudo-account asset balance
+            // (requires fixSecurity3_1_3)
+            doInvariantCheck(
+                {{"Loan Broker cover available is greater than pseudo-account "
+                  "asset balance"}},
+                [&](Account const&, Account const&, ApplyContext& ac) {
+                    auto sle = ac.view().peek(loanBrokerKeylet);
+                    if (!BEAST_EXPECT(sle))
+                        return false;
+                    // Pseudo-account has no cover deposited; set cover
+                    // higher than any incidental balance
+                    sle->at(sfCoverAvailable) = Number(1'000'000);
+                    ac.view().update(sle);
+                    return true;
+                },
+                XRPAmount{},
+                STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}},
+                {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
+                createLoanBroker);
         }
     }
 

src/test/app/Loan_test.cpp

@@ -2273,7 +2273,23 @@ protected:
                     fee(XRPAmount{
                         baseFee *
                         (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}),
-                    ter(temINVALID_FLAG));
+                    ter(tecNO_PERMISSION));
+
+                {
+                    env.disableFeature(fixSecurity3_1_3);
+                    env(pay(borrower,
+                            loanKeylet.key,
+                            STAmount{
+                                broker.asset,
+                                state.periodicPayment * Number{15, -1}},
+                            tfLoanOverpayment),
+                        fee(XRPAmount{
+                            baseFee *
+                            (Number{15, -1} / loanPaymentsPerFeeIncrement +
+                             1)}),
+                        ter(temINVALID_FLAG));
+                    env.enableFeature(fixSecurity3_1_3);
+                }
             }
             // Try to send a payment marked as multiple mutually exclusive
             // payment types. Do not include `txFlags`, so we don't duplicate

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

@@ -2532,18 +2532,33 @@ ValidLoanBroker::finalize(
             return false;
         }
         auto const& vaultAsset = vault->at(sfAsset);
-        if (after->at(sfCoverAvailable) < accountHolds(
-                                              view,
-                                              after->at(sfAccount),
-                                              vaultAsset,
-                                              FreezeHandling::fhIGNORE_FREEZE,
-                                              AuthHandling::ahIGNORE_AUTH,
-                                              j))
+        auto const pseudoBalance = accountHolds(
+            view,
+            after->at(sfAccount),
+            vaultAsset,
+            FreezeHandling::fhIGNORE_FREEZE,
+            AuthHandling::ahIGNORE_AUTH,
+            j);
+        if (after->at(sfCoverAvailable) < pseudoBalance)
         {
             JLOG(j.fatal()) << "Invariant failed: Loan Broker cover available "
                                "is less than pseudo-account asset balance";
             return false;
         }
+
+        if (view.rules().enabled(fixSecurity3_1_3))
+        {
+            // Don't check the balance when LoanBroker is deleted,
+            // sfCoverAvailable is not zeroed
+            if (tx.getTxnType() != ttLOAN_BROKER_DELETE &&
+                after->at(sfCoverAvailable) > pseudoBalance)
+            {
+                JLOG(j.fatal()) << "Invariant failed: Loan Broker cover "
+                                   "available is greater "
+                                   "than pseudo-account asset balance";
+                return false;
+            }
+        }
     }
     return true;
 }

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

@@ -418,21 +418,30 @@ LoanManage::doApply()
         return tefBAD_LEDGER;  // LCOV_EXCL_LINE
     auto const vaultAsset = vaultSle->at(sfAsset);
 
-    // Valid flag combinations are checked in preflight. No flags is valid -
-    // just a noop.
-    if (tx.isFlag(tfLoanDefault))
-        return defaultLoan(view, loanSle, brokerSle, vaultSle, vaultAsset, j_);
-    if (tx.isFlag(tfLoanImpair))
-        return impairLoan(view, loanSle, vaultSle, vaultAsset, j_);
-    if (tx.isFlag(tfLoanUnimpair))
-        return unimpairLoan(view, loanSle, vaultSle, vaultAsset, j_);
-    // Noop, as described above.
+    auto const result = [&]() -> TER {
+        // Valid flag combinations are checked in preflight. No flags is valid -
+        // just a noop.
+        if (tx.isFlag(tfLoanDefault))
+            return defaultLoan(
+                view, loanSle, brokerSle, vaultSle, vaultAsset, j_);
+        if (tx.isFlag(tfLoanImpair))
+            return impairLoan(view, loanSle, vaultSle, vaultAsset, j_);
+        if (tx.isFlag(tfLoanUnimpair))
+            return unimpairLoan(view, loanSle, vaultSle, vaultAsset, j_);
+        // Noop, as described above.
+        return tesSUCCESS;
+    }();
 
-    associateAsset(*loanSle, vaultAsset);
-    associateAsset(*brokerSle, vaultAsset);
-    associateAsset(*vaultSle, vaultAsset);
+    // Pre-amendment, associateAsset was only called on the noop (no flags)
+    // path. Post-amendment, we call associateAsset on all successful paths.
+    if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result))
+    {
+        associateAsset(*loanSle, vaultAsset);
+        associateAsset(*brokerSle, vaultAsset);
+        associateAsset(*vaultSle, vaultAsset);
+    }
 
-    return tesSUCCESS;
+    return result;
 }
 
 //------------------------------------------------------------------------------

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

@@ -149,7 +149,9 @@ LoanPay::preclaim(PreclaimContext const& ctx)
     {
         JLOG(ctx.j.warn())
             << "Requested overpayment on a loan that doesn't allow it";
-        return temINVALID_FLAG;
+        return ctx.view.rules().enabled(fixSecurity3_1_3)
+            ? TER{tecNO_PERMISSION}
+            : temINVALID_FLAG;
     }
 
     auto const principalOutstanding = loanSle->at(sfPrincipalOutstanding);

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

@@ -30,6 +30,7 @@
 #include <xrpl/protocol/TER.h>
 
 #include <optional>
+#include <utility>
 
 namespace ripple {
 
@@ -257,7 +258,12 @@ VaultClawback::assetsToClawback(
     auto const mptIssuanceID = *vault->at(sfShareMPTID);
     MPTIssue const share{mptIssuanceID};
 
-    if (clawbackAmount == beast::zero)
+    // Pre-fixSecurity3_1_3: zero-amount clawback returned early without
+    // clamping to assetsAvailable, allowing more assets to be recovered
+    // than available when there was an outstanding loan. Retained for
+    // ledger replay compatibility.
+    if (!ctx_.view().rules().enabled(fixSecurity3_1_3) &&
+        clawbackAmount == beast::zero)
     {
         auto const sharesDestroyed = accountHolds(
             view(),
@@ -275,23 +281,40 @@ VaultClawback::assetsToClawback(
     }
 
     STAmount sharesDestroyed;
-    STAmount assetsRecovered = clawbackAmount;
+    STAmount assetsRecovered;
+
     try
     {
+        if (clawbackAmount == beast::zero)
         {
-            auto const maybeShares = assetsToSharesWithdraw(
-                vault, sleShareIssuance, assetsRecovered);
+            sharesDestroyed = accountHolds(
+                view(),
+                holder,
+                share,
+                FreezeHandling::fhIGNORE_FREEZE,
+                AuthHandling::ahIGNORE_AUTH,
+                j_);
+            auto const maybeAssets = sharesToAssetsWithdraw(
+                vault, sleShareIssuance, sharesDestroyed);
+            if (!maybeAssets)
+                return Unexpected(tecINTERNAL);  // LCOV_EXCL_LINE
+
+            assetsRecovered = *maybeAssets;
+        }
+        else
+        {
+            auto const maybeShares =
+                assetsToSharesWithdraw(vault, sleShareIssuance, clawbackAmount);
             if (!maybeShares)
                 return Unexpected(tecINTERNAL);  // LCOV_EXCL_LINE
             sharesDestroyed = *maybeShares;
+
+            auto const maybeAssets = sharesToAssetsWithdraw(
+                vault, sleShareIssuance, sharesDestroyed);
+            if (!maybeAssets)
+                return Unexpected(tecINTERNAL);  // LCOV_EXCL_LINE
+            assetsRecovered = *maybeAssets;
         }
-
-        auto const maybeAssets =
-            sharesToAssetsWithdraw(vault, sleShareIssuance, sharesDestroyed);
-        if (!maybeAssets)
-            return Unexpected(tecINTERNAL);  // LCOV_EXCL_LINE
-        assetsRecovered = *maybeAssets;
-
         // Clamp to maximum.
         if (assetsRecovered > *assetsAvailable)
         {

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

@@ -61,10 +61,10 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
     if (!vault)
         return tecNO_ENTRY;
 
-    auto const assets = ctx.tx[sfAmount];
+    auto const amount = ctx.tx[sfAmount];
     auto const vaultAsset = vault->at(sfAsset);
     auto const vaultShare = vault->at(sfShareMPTID);
-    if (assets.asset() != vaultAsset && assets.asset() != vaultShare)
+    if (amount.asset() != vaultAsset && amount.asset() != vaultShare)
         return tecWRONG_ASSET;
 
     auto const& vaultAccount = vault->at(sfAccount);
@@ -87,8 +87,56 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         // LCOV_EXCL_STOP
     }
 
-    if (auto const ret = canWithdraw(ctx.view, ctx.tx))
-        return ret;
+    if (ctx.view.rules().enabled(fixSecurity3_1_3) &&
+        amount.asset() == vaultShare)
+    {
+        // Post-fixSecurity3_1_3: if the user specified shares, convert
+        // to the equivalent asset amount before checking withdrawal
+        // limits. Pre-amendment the limit check was skipped for
+        // share-denominated withdrawals.
+        auto const sleIssuance = ctx.view.read(keylet::mptIssuance(vaultShare));
+        if (!sleIssuance)
+        {
+            // LCOV_EXCL_START
+            JLOG(ctx.j.error())
+                << "VaultWithdraw: missing issuance of vault shares.";
+            return tefINTERNAL;
+            // LCOV_EXCL_STOP
+        }
+
+        try
+        {
+            auto const maybeAssets =
+                sharesToAssetsWithdraw(vault, sleIssuance, amount);
+            if (!maybeAssets)
+                return tefINTERNAL;  // LCOV_EXCL_LINE
+
+            if (auto const ret = canWithdraw(
+                    ctx.view,
+                    account,
+                    dstAcct,
+                    *maybeAssets,
+                    ctx.tx.isFieldPresent(sfDestinationTag)))
+                return ret;
+        }
+        catch (std::overflow_error const&)
+        {
+            // It's easy to hit this exception from Number with large enough
+            // Scale so we avoid spamming the log and only use debug here.
+            JLOG(ctx.j.debug())  //
+                << "VaultWithdraw: overflow error with"
+                << " scale=" << (int)vault->at(sfScale)  //
+                << ", assetsTotal=" << vault->at(sfAssetsTotal)
+                << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount)
+                << ", amount=" << amount.value();
+            return tecPATH_DRY;
+        }
+    }
+    else
+    {
+        if (auto const ret = canWithdraw(ctx.view, ctx.tx))
+            return ret;
+    }
 
     // If sending to Account (i.e. not a transfer), we will also create (only
     // if authorized) a trust line or MPToken as needed, in doApply().

src/xrpld/rpc/detail/Tuning.h

@@ -49,7 +49,7 @@ static LimitRange constexpr accountOffers = {10, 200, 400};
 static LimitRange constexpr accountTx = {10, 200, 400};
 
 /** Limits for the book_offers command. */
-static LimitRange constexpr bookOffers = {1, 60, 100};
+static LimitRange constexpr bookOffers = {0, 60, 100};
 
 /** Limits for the no_ripple_check command. */
 static LimitRange constexpr noRippleCheck = {10, 300, 400};