Merge branch 'ripple/staging-313' into ximinez/loanpay-assertion

Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com>

.github/workflows/publish-docs.yml

@@ -4,6 +4,17 @@ 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"
@@ -22,21 +33,26 @@ defaults:
     shell: bash
 
 env:
-  BUILD_DIR: .build
-  NPROC_SUBTRACT: 2
+  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' }}
 
 jobs:
-  publish:
+  build:
     runs-on: ubuntu-latest
     container: ghcr.io/xrplf/ci/tools-rippled-documentation:sha-a8c7be1
-    permissions:
-      contents: write
     steps:
       - name: Checkout repository
-        uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+
+      - name: Prepare runner
+        uses: XRPLF/actions/prepare-runner@90f11ee655d1687824fb8793db770477d52afbab
+        with:
+          enable_ccache: false
 
       - name: Get number of processors
-        uses: XRPLF/actions/.github/actions/get-nproc@046b1620f6bfd6cd0985dc82c3df02786801fe0a
+        uses: XRPLF/actions/get-nproc@cf0433aa74563aead044a1e395610c96d65a37cf
         id: nproc
         with:
           subtract: ${{ env.NPROC_SUBTRACT }}
@@ -64,9 +80,23 @@ jobs:
           cmake -Donly_docs=ON ..
           cmake --build . --target docs --parallel ${BUILD_NPROC}
 
-      - 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
+      - name: Create documentation artifact
+        if: ${{ github.event.repository.visibility == 'public' && github.event_name == 'push' }}
+        uses: actions/upload-pages-artifact@7b1f4a764d45c48632c6b24a0339c27f5614fb0b # v4.0.0
         with:
-          github_token: ${{ secrets.GITHUB_TOKEN }}
-          publish_dir: ${{ env.BUILD_DIR }}/docs/html
+          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

.pre-commit-config.yaml

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

include/xrpl/protocol/STAmount.h

@@ -62,8 +62,8 @@ private:
 public:
     using value_type = STAmount;
 
-    static int const cMinOffset = -96;
-    static int const cMaxOffset = 80;
+    static constexpr int cMinOffset = -96;
+    static constexpr int cMaxOffset = 80;
 
     // Maximum native value supported by the code
     constexpr static std::uint64_t cMinValue = 1'000'000'000'000'000ull;

src/libxrpl/ledger/View.cpp

@@ -1559,9 +1559,7 @@ authorizeMPToken(
         {
             auto const mptokenKey = keylet::mptoken(mptIssuanceID, account);
             auto const sleMpt = view.peek(mptokenKey);
-            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0 ||
-                (view.rules().enabled(fixSecurity3_1_3) &&
-                 (*sleMpt)[~sfLockedAmount].value_or(0) != 0))
+            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
                 return tecINTERNAL;  // LCOV_EXCL_LINE
 
             if (!view.dirRemove(
@@ -1870,9 +1868,7 @@ 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 ||
-        (view.rules().enabled(fixSecurity3_1_3) &&
-         (*mptoken)[~sfLockedAmount].value_or(0) != 0))
+    if (mptoken->at(sfMPTAmount) != 0)
         return tecHAS_OBLIGATIONS;
 
     return authorizeMPToken(

src/test/app/Invariants_test.cpp

@@ -1939,41 +1939,39 @@ class Invariants_test : public beast::unit_test::suite
         {
             // Initialize with a placeholder value because there's no default
             // ctor
-            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);
+            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};
+                    }
+                }();
                 loanBrokerKeylet = this->createLoanBroker(alice, env, asset);
                 return BEAST_EXPECT(env.le(loanBrokerKeylet));
             };
@@ -2161,61 +2159,6 @@ 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

@@ -314,17 +314,11 @@ protected:
                         env.balance(vaultPseudo, broker.asset).number());
                     if (ownerCount == 0)
                     {
-                        // Allow some slop for rounding IOUs
-
-                        // TODO: This needs to be an exact match once all the
-                        // other rounding issues are worked out.
+                        // The Vault must be perfectly balanced if there
+                        // are no loans outstanding
                         auto const total = vaultSle->at(sfAssetsTotal);
                         auto const available = vaultSle->at(sfAssetsAvailable);
-                        env.test.BEAST_EXPECT(
-                            total == available ||
-                            (!broker.asset.integral() && available != 0 &&
-                             ((total - available) / available <
-                              Number(1, -6))));
+                        env.test.BEAST_EXPECT(total == available);
                         env.test.BEAST_EXPECT(
                             vaultSle->at(sfLossUnrealized) == 0);
                     }
@@ -2273,23 +2267,7 @@ protected:
                     fee(XRPAmount{
                         baseFee *
                         (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}),
-                    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);
-                }
+                    ter(temINVALID_FLAG));
             }
             // Try to send a payment marked as multiple mutually exclusive
             // payment types. Do not include `txFlags`, so we don't duplicate
@@ -7660,6 +7638,133 @@ protected:
         BEAST_EXPECT(afterSecondCoverAvailable == 0);
     }
 
+    void
+    testYieldTheftRounding(std::uint32_t flags)
+    {
+        testcase("Yield Theft via Rounding Manipulation");
+        using namespace jtx;
+        using namespace loan;
+
+        // 1. Setup Environment
+        Env env(*this, all);
+        Account const issuer{"issuer"};
+        Account const lender{"lender"};
+        Account const borrower{"borrower"};
+
+        env.fund(XRP(1000), issuer, lender, borrower);
+        env.close();
+
+        // 2. Asset Selection
+        PrettyAsset const iou = issuer["USD"];
+        env(trust(lender, iou(100'000'000)));
+        env(trust(borrower, iou(100'000'000)));
+        env(pay(issuer, lender, iou(100'000'000)));
+        env(pay(issuer, borrower, iou(100'000'000)));
+        env.close();
+
+        // 3. Create Vault and Broker with High Debt Limit (100M)
+        auto const brokerInfo = createVaultAndBroker(
+            env,
+            iou,
+            lender,
+            {
+                .vaultDeposit = 5'000'000,
+                .debtMax = Number{100'000'000},
+                .coverDeposit = 500'000,
+            });
+        auto const [currentSeq, vaultId, vaultKeylet] = [&]() {
+            auto const brokerSle =
+                env.le(keylet::loanbroker(brokerInfo.brokerID));
+            auto const currentSeq = brokerSle->at(sfLoanSequence);
+            auto const vaultKeylet = keylet::vault(brokerSle->at(sfVaultID));
+            auto const vaultId = brokerSle->at(sfVaultID);
+            return std::make_tuple(currentSeq, vaultId, vaultKeylet);
+        }();
+
+        // 4. Loan Parameters (Attack Vector)
+        Number const principal = 1'000'000;
+        TenthBips32 const interestRate = TenthBips32{1};  // 0.001%
+        std::uint32_t const paymentInterval = 86400;
+        std::uint32_t const paymentTotal = 3650;
+
+        auto const loanSetFee = fee(env.current()->fees().base * 2);
+        env(set(borrower, brokerInfo.brokerID, iou(principal).value(), flags),
+            sig(sfCounterpartySignature, lender),
+            loan::interestRate(interestRate),
+            loan::paymentInterval(paymentInterval),
+            loan::paymentTotal(paymentTotal),
+            fee(loanSetFee));
+        env.close();
+
+        // --- RETRIEVE OBJECTS & SETUP ATTACK ---
+
+        auto const loanKeylet = keylet::loan(brokerInfo.brokerID, currentSeq);
+        auto const [periodicPayment, loanScale] = [&]() {
+            auto const loanSle = env.le(loanKeylet);
+            // Construct Payment
+            return std::make_tuple(
+                STAmount{iou, loanSle->at(sfPeriodicPayment)},
+                loanSle->at(sfLoanScale));
+        }();
+        auto const roundedPayment =
+            roundToScale(periodicPayment, loanScale, Number::upward);
+
+        // ATTACK: Add dust buffer (1e-9) to force 'excess' logic execution
+        STAmount const paymentBuffer{iou, Number(1, -9)};
+        STAmount const attackPayment = periodicPayment + paymentBuffer;
+
+        auto const initialVaultAssets = env.le(vaultKeylet)->at(sfAssetsTotal);
+
+        // 5. Execution Loop
+        int yieldTheftCount = 0;
+        auto previousAssetsTotal = initialVaultAssets;
+
+        auto borrowerBalance = [&]() { return env.balance(borrower, iou); };
+
+        for (int i = 0; i < 100; ++i)
+        {
+            auto const balanceBefore = borrowerBalance();
+            env(pay(borrower, loanKeylet.key, attackPayment, flags));
+            env.close();
+            auto const borrowerDelta = borrowerBalance() - balanceBefore;
+
+            auto const loanSle = env.le(loanKeylet);
+            if (!BEAST_EXPECT(loanSle))
+                break;
+            auto const updatedPayment =
+                STAmount{iou, loanSle->at(sfPeriodicPayment)};
+            BEAST_EXPECT(
+                (roundToScale(updatedPayment, loanScale, Number::upward) ==
+                 roundedPayment));
+            BEAST_EXPECT(
+                (updatedPayment == periodicPayment) ||
+                (flags == tfLoanOverpayment && i >= 2 &&
+                 updatedPayment < periodicPayment));
+
+            auto const currentVaultSle = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(currentVaultSle))
+                break;
+
+            auto const currentAssetsTotal = currentVaultSle->at(sfAssetsTotal);
+            auto const delta = currentAssetsTotal - previousAssetsTotal;
+
+            BEAST_EXPECT(
+                (delta == beast::zero && borrowerDelta <= roundedPayment) ||
+                (delta > beast::zero && borrowerDelta > roundedPayment));
+
+            // If tx succeeded but Assets Total didn't change, interest was
+            // stolen.
+            if (delta == beast::zero && borrowerDelta > roundedPayment)
+            {
+                yieldTheftCount++;
+            }
+
+            previousAssetsTotal = currentAssetsTotal;
+        }
+
+        BEAST_EXPECTS(yieldTheftCount == 0, std::to_string(yieldTheftCount));
+    }
+
 public:
     void
     run() override
@@ -7668,6 +7773,11 @@ public:
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
+        for (auto const flags : {0u, tfLoanOverpayment})
+        {
+            testYieldTheftRounding(flags);
+        }
+
         testInvalidLoanSet();
 
         testCoverDepositWithdrawNonTransferableMPT();

src/xrpld/app/misc/detail/LendingHelpers.cpp

@@ -1927,10 +1927,20 @@ loanMakePayment(
         "no value change");
 
     // -------------------------------------------------------------
-    // overpayment handling
+    //
+    // If the "fixSecurity3_1_3" amendment is enabled, truncate "amount",
+    // at the loan scale. If the raw value is used, the overpayment
+    // amount could be meaningless dust. Trying to process such a small
+    // amount will, at best, waste time when all the result values round
+    // to zero. At worst, it can cause logical errors with tiny amounts
+    // of interest that don't add up correctly.
+    auto const roundedAmount = view.rules().enabled(fixSecurity3_1_3)
+        ? roundToAsset(asset, amount, loanScale, Number::towards_zero)
+        : amount;
     if (paymentType == LoanPaymentType::overpayment &&
         loan->isFlag(lsfLoanOverpayment) && paymentRemainingProxy > 0 &&
-        totalPaid < amount && numPayments < loanMaximumPaymentsPerTransaction)
+        totalPaid < roundedAmount &&
+        numPayments < loanMaximumPaymentsPerTransaction)
     {
         TenthBips32 const overpaymentInterestRate{
             loan->at(sfOverpaymentInterestRate)};
@@ -1940,7 +1950,7 @@ loanMakePayment(
         // totalValueOutstanding, because that would have been processed as
         // another normal payment. But cap it just in case.
         Number const overpayment =
-            std::min(amount - totalPaid, *totalValueOutstandingProxy);
+            std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
 
         detail::ExtendedPaymentComponents const overpaymentComponents =
             detail::computeOverpaymentComponents(

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

@@ -2532,33 +2532,18 @@ ValidLoanBroker::finalize(
             return false;
         }
         auto const& vaultAsset = vault->at(sfAsset);
-        auto const pseudoBalance = accountHolds(
-            view,
-            after->at(sfAccount),
-            vaultAsset,
-            FreezeHandling::fhIGNORE_FREEZE,
-            AuthHandling::ahIGNORE_AUTH,
-            j);
-        if (after->at(sfCoverAvailable) < pseudoBalance)
+        if (after->at(sfCoverAvailable) < accountHolds(
+                                              view,
+                                              after->at(sfAccount),
+                                              vaultAsset,
+                                              FreezeHandling::fhIGNORE_FREEZE,
+                                              AuthHandling::ahIGNORE_AUTH,
+                                              j))
         {
             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,30 +418,21 @@ LoanManage::doApply()
         return tefBAD_LEDGER;  // LCOV_EXCL_LINE
     auto const vaultAsset = vaultSle->at(sfAsset);
 
-    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;
-    }();
+    // 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.
 
-    // 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);
-    }
+    associateAsset(*loanSle, vaultAsset);
+    associateAsset(*brokerSle, vaultAsset);
+    associateAsset(*vaultSle, vaultAsset);
 
-    return result;
+    return tesSUCCESS;
 }
 
 //------------------------------------------------------------------------------

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

@@ -8,6 +8,7 @@
 #include <xrpl/protocol/STTakesAsset.h>
 #include <xrpl/protocol/TxFlags.h>
 
+#include <algorithm>
 #include <bit>
 
 namespace ripple {
@@ -149,9 +150,7 @@ LoanPay::preclaim(PreclaimContext const& ctx)
     {
         JLOG(ctx.j.warn())
             << "Requested overpayment on a loan that doesn't allow it";
-        return ctx.view.rules().enabled(fixSecurity3_1_3)
-            ? TER{tecNO_PERMISSION}
-            : temINVALID_FLAG;
+        return temINVALID_FLAG;
     }
 
     auto const principalOutstanding = loanSle->at(sfPrincipalOutstanding);
@@ -432,9 +431,10 @@ LoanPay::doApply()
     // Vault object state changes
     view.update(vaultSle);
 
+    Number const assetsAvailableBefore = *assetsAvailableProxy;
+    Number const assetsTotalBefore = *assetsTotalProxy;
 #if !NDEBUG
     {
-        Number const assetsAvailableBefore = *assetsAvailableProxy;
         Number const pseudoAccountBalanceBefore = accountHolds(
             view,
             vaultPseudoAccount,
@@ -458,17 +458,6 @@ LoanPay::doApply()
         "rippled::LoanPay::doApply",
         "assets available must not be greater than assets outstanding");
 
-    if (*assetsAvailableProxy > *assetsTotalProxy)
-    {
-        // LCOV_EXCL_START
-        JLOG(j_.fatal()) << "Vault assets available must not be greater "
-                            "than assets outstanding. Available: "
-                         << *assetsAvailableProxy
-                         << ", Total: " << *assetsTotalProxy;
-        return tecINTERNAL;
-        // LCOV_EXCL_STOP
-    }
-
     JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw
                      << ", total paid to vault rounded: "
                      << totalPaidToVaultRounded
@@ -495,12 +484,74 @@ LoanPay::doApply()
     associateAsset(*vaultSle, asset);
 
     // Duplicate some checks after rounding
+    Number const assetsAvailableAfter = *assetsAvailableProxy;
+    Number const assetsTotalAfter = *assetsTotalProxy;
+
     XRPL_ASSERT_PARTS(
-        *assetsAvailableProxy <= *assetsTotalProxy,
+        assetsAvailableAfter <= assetsTotalAfter,
         "xrpl::LoanPay::doApply",
         "assets available must not be greater than assets outstanding");
+    if (assetsAvailableAfter == assetsAvailableBefore)
+    {
+        // An unchanged assetsAvailable indicates that the amount paid to the
+        // vault was zero, or rounded to zero. That should be impossible, but I
+        // can't rule it out for extreme edge cases, so fail gracefully if it
+        // happens.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.warn())
+            << "LoanPay: Vault assets available unchanged after rounding: "  //
+            << "Before: " << assetsAvailableBefore                           //
+            << ", After: " << assetsAvailableAfter;
+        return tecPRECISION_LOSS;
+        // LCOV_EXCL_STOP
+    }
+    if (paymentParts->valueChange != beast::zero &&
+        assetsTotalAfter == assetsTotalBefore)
+    {
+        // Non-zero valueChange with an unchanged assetsTotal indicates that the
+        // actual value change rounded to zero. That should be impossible, but I
+        // can't rule it out for extreme edge cases, so fail gracefully if it
+        // happens.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.warn()) << "LoanPay: Vault assets expected change, but "
+                           "unchanged after rounding: "     //
+                        << "Before: " << assetsTotalBefore  //
+                        << ", After: " << assetsTotalAfter  //
+                        << ", ValueChange: " << paymentParts->valueChange;
+        return tecPRECISION_LOSS;
+        // LCOV_EXCL_STOP
+    }
+    if (paymentParts->valueChange == beast::zero &&
+        assetsTotalAfter != assetsTotalBefore)
+    {
+        // A change in assetsTotal when there was no valueChange indicates that
+        // something really weird happened. That should be flat out impossible.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.fatal())
+            << "LoanPay: Vault assets changed unexpectedly after rounding: "  //
+            << "Before: " << assetsTotalBefore                                //
+            << ", After: " << assetsTotalAfter                                //
+            << ", ValueChange: " << paymentParts->valueChange;
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
+    }
+    if (assetsAvailableAfter > assetsTotalAfter)
+    {
+        // Assets available are not allowed to be larger than assets total.
+        // LCOV_EXCL_START
+        JLOG(j_.fatal())
+            << "LoanPay: Vault assets available must not be greater "
+               "than assets outstanding. Available: "
+            << assetsAvailableAfter << ", Total: " << assetsTotalAfter;
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
+    }
 
-#if !NDEBUG
+    // These three values are used to check that funds are conserved after the
+    // transfers
     auto const accountBalanceBefore = accountHolds(
         view,
         account_,
@@ -529,7 +580,6 @@ LoanPay::doApply()
               ahIGNORE_AUTH,
               j_,
               SpendableHandling::shFULL_BALANCE);
-#endif
 
     if (totalPaidToVaultRounded != beast::zero)
     {
@@ -570,19 +620,22 @@ LoanPay::doApply()
         return ter;
 
 #if !NDEBUG
-    Number const assetsAvailableAfter = *assetsAvailableProxy;
-    Number const pseudoAccountBalanceAfter = accountHolds(
-        view,
-        vaultPseudoAccount,
-        asset,
-        FreezeHandling::fhIGNORE_FREEZE,
-        AuthHandling::ahIGNORE_AUTH,
-        j_);
-    XRPL_ASSERT_PARTS(
-        assetsAvailableAfter == pseudoAccountBalanceAfter,
-        "ripple::LoanPay::doApply",
-        "vault pseudo balance agrees after");
+    {
+        Number const pseudoAccountBalanceAfter = accountHolds(
+            view,
+            vaultPseudoAccount,
+            asset,
+            FreezeHandling::fhIGNORE_FREEZE,
+            AuthHandling::ahIGNORE_AUTH,
+            j_);
+        XRPL_ASSERT_PARTS(
+            assetsAvailableAfter == pseudoAccountBalanceAfter,
+            "xrpl::LoanPay::doApply",
+            "vault pseudo balance agrees after");
+    }
+#endif
 
+    // Check that funds are conserved
     auto const accountBalanceAfter = accountHolds(
         view,
         account_,
@@ -611,16 +664,132 @@ LoanPay::doApply()
               ahIGNORE_AUTH,
               j_,
               SpendableHandling::shFULL_BALANCE);
+    auto const balanceScale = [&]() {
+        // Find a reasonable scale to use for the balance comparisons.
+        //
+        // First find the minimum and maximum exponent of all the non-zero
+        // balances, before and after. If min and max are equal, use that value.
+        // If they are not, use "max + 1" to reduce rounding discrepancies
+        // without making the result meaningless. Cap the scale at
+        // STAmount::cMaxOffset, just in case the numbers are all very large.
+        std::vector<int> exponents;
 
+        for (auto const& a : {
+                 accountBalanceBefore,
+                 vaultBalanceBefore,
+                 brokerBalanceBefore,
+                 accountBalanceAfter,
+                 vaultBalanceAfter,
+                 brokerBalanceAfter,
+             })
+        {
+            // Exclude zeroes
+            if (a != beast::zero)
+                exponents.push_back(a.exponent());
+        }
+        if (exponents.empty())
+        {
+            UNREACHABLE("xrpl::LoanPay::doApply : all zeroes");
+            return 0;
+        }
+        auto const [minItr, maxItr] =
+            std::minmax_element(exponents.begin(), exponents.end());
+        auto const min = *minItr;
+        auto const max = *maxItr;
+        JLOG(j_.trace()) << "Min scale: " << min << ", max scale: " << max;
+        // IOU rounding can be interesting. We want all the balance checks to
+        // agree, but don't want to round to such an extreme that it becomes
+        // meaningless.  e.g. Everything rounds to one digit. So add 1 to the
+        // max (reducing the number of digits after the decimal point by 1) if
+        // the scales are not already all the same.
+        return std::min(min == max ? max : max + 1, STAmount::cMaxOffset);
+    }();
+
+    auto const accountBalanceBeforeRounded =
+        roundToScale(accountBalanceBefore, balanceScale);
+    auto const vaultBalanceBeforeRounded =
+        roundToScale(vaultBalanceBefore, balanceScale);
+    auto const brokerBalanceBeforeRounded =
+        roundToScale(brokerBalanceBefore, balanceScale);
+
+    auto const totalBalanceBefore =
+        accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore;
+    auto const totalBalanceBeforeRounded =
+        roundToScale(totalBalanceBefore, balanceScale);
+
+    JLOG(j_.trace()) << "Before: "  //
+                     << "account " << Number(accountBalanceBeforeRounded)
+                     << " (" << Number(accountBalanceBefore) << ")"
+                     << ", vault " << Number(vaultBalanceBeforeRounded) << " ("
+                     << Number(vaultBalanceBefore) << ")"
+                     << ", broker " << Number(brokerBalanceBeforeRounded)
+                     << " (" << Number(brokerBalanceBefore) << ")"
+                     << ", total " << Number(totalBalanceBeforeRounded) << " ("
+                     << Number(totalBalanceBefore) << ")";
+
+    auto const accountBalanceAfterRounded =
+        roundToScale(accountBalanceAfter, balanceScale);
+    auto const vaultBalanceAfterRounded =
+        roundToScale(vaultBalanceAfter, balanceScale);
+    auto const brokerBalanceAfterRounded =
+        roundToScale(brokerBalanceAfter, balanceScale);
+
+    auto const totalBalanceAfter =
+        accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter;
+    auto const totalBalanceAfterRounded =
+        roundToScale(totalBalanceAfter, balanceScale);
+
+    JLOG(j_.trace()) << "After: "  //
+                     << "account " << Number(accountBalanceAfterRounded) << " ("
+                     << Number(accountBalanceAfter) << ")"
+                     << ", vault " << Number(vaultBalanceAfterRounded) << " ("
+                     << Number(vaultBalanceAfter) << ")"
+                     << ", broker " << Number(brokerBalanceAfterRounded) << " ("
+                     << Number(brokerBalanceAfter) << ")"
+                     << ", total " << Number(totalBalanceAfterRounded) << " ("
+                     << Number(totalBalanceAfter) << ")";
+
+    auto const accountBalanceChange =
+        accountBalanceAfter - accountBalanceBefore;
+    auto const vaultBalanceChange = vaultBalanceAfter - vaultBalanceBefore;
+    auto const brokerBalanceChange = brokerBalanceAfter - brokerBalanceBefore;
+
+    auto const totalBalanceChange =
+        accountBalanceChange + vaultBalanceChange + brokerBalanceChange;
+    auto const totalBalanceChangeRounded =
+        roundToScale(totalBalanceChange, balanceScale);
+
+    JLOG(j_.trace()) << "Changes: "                                    //
+                     << "account " << to_string(accountBalanceChange)  //
+                     << ", vault " << to_string(vaultBalanceChange)    //
+                     << ", broker " << to_string(brokerBalanceChange)  //
+                     << ", total " << to_string(totalBalanceChangeRounded)
+                     << " (" << Number(totalBalanceChange) << ")";
+
+    if (totalBalanceBeforeRounded != totalBalanceAfterRounded)
+    {
+        JLOG(j_.warn()) << "Total rounded balances don't match"
+                        << (totalBalanceChangeRounded == beast::zero
+                                ? ", but total changes do"
+                                : "");
+    }
+    if (totalBalanceChangeRounded != beast::zero)
+    {
+        JLOG(j_.warn()) << "Total balance changes don't match"
+                        << (totalBalanceBeforeRounded ==
+                                    totalBalanceAfterRounded
+                                ? ", but total balances do"
+                                : "");
+    }
+
+    // Rounding for IOUs can be weird, so check a few different ways to show
+    // that funds are conserved.
     XRPL_ASSERT_PARTS(
-        accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore ==
-            accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter,
-        "ripple::LoanPay::doApply",
+        totalBalanceBeforeRounded == totalBalanceAfterRounded ||
+            totalBalanceChangeRounded == beast::zero,
+        "xrpl::LoanPay::doApply",
         "funds are conserved (with rounding)");
-    XRPL_ASSERT_PARTS(
-        accountBalanceAfter >= beast::zero,
-        "ripple::LoanPay::doApply",
-        "positive account balance");
+
     XRPL_ASSERT_PARTS(
         accountBalanceAfter < accountBalanceBefore ||
             account_ == asset.getIssuer(),
@@ -643,7 +812,6 @@ LoanPay::doApply()
             brokerBalanceAfter > brokerBalanceBefore,
         "ripple::LoanPay::doApply",
         "vault and/or broker balance increased");
-#endif
 
     return tesSUCCESS;
 }

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

@@ -30,7 +30,6 @@
 #include <xrpl/protocol/TER.h>
 
 #include <optional>
-#include <utility>
 
 namespace ripple {
 
@@ -258,12 +257,7 @@ VaultClawback::assetsToClawback(
     auto const mptIssuanceID = *vault->at(sfShareMPTID);
     MPTIssue const share{mptIssuanceID};
 
-    // 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)
+    if (clawbackAmount == beast::zero)
     {
         auto const sharesDestroyed = accountHolds(
             view(),
@@ -281,40 +275,23 @@ VaultClawback::assetsToClawback(
     }
 
     STAmount sharesDestroyed;
-    STAmount assetsRecovered;
-
+    STAmount assetsRecovered = clawbackAmount;
     try
     {
-        if (clawbackAmount == beast::zero)
         {
-            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);
+            auto const maybeShares = assetsToSharesWithdraw(
+                vault, sleShareIssuance, assetsRecovered);
             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 amount = ctx.tx[sfAmount];
+    auto const assets = ctx.tx[sfAmount];
     auto const vaultAsset = vault->at(sfAsset);
     auto const vaultShare = vault->at(sfShareMPTID);
-    if (amount.asset() != vaultAsset && amount.asset() != vaultShare)
+    if (assets.asset() != vaultAsset && assets.asset() != vaultShare)
         return tecWRONG_ASSET;
 
     auto const& vaultAccount = vault->at(sfAccount);
@@ -87,56 +87,8 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         // LCOV_EXCL_STOP
     }
 
-    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 (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 = {0, 60, 100};
+static LimitRange constexpr bookOffers = {1, 60, 100};
 
 /** Limits for the no_ripple_check command. */
 static LimitRange constexpr noRippleCheck = {10, 300, 400};