Verify and log LoanPay fund conservation in all builds

- A warning will be logged if there's a mismatch.

include/xrpl/protocol/STAmount.h

@@ -42,8 +42,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/test/app/Loan_test.cpp

@@ -6676,6 +6676,125 @@ 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
@@ -6684,6 +6803,11 @@ public:
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
+        for (auto const flags : {0u, tfLoanOverpayment})
+        {
+            testYieldTheftRounding(flags);
+        }
+
         testInvalidLoanSet();
 
         testCoverDepositWithdrawNonTransferableMPT();

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

@@ -1733,8 +1733,9 @@ loanMakePayment(
 
     // -------------------------------------------------------------
     // overpayment handling
+    auto const roundedAmount = roundToAsset(asset, amount, loanScale, Number::towards_zero);
     if (paymentType == LoanPaymentType::overpayment && loan->isFlag(lsfLoanOverpayment) && paymentRemainingProxy > 0 &&
-        totalPaid < amount && numPayments < loanMaximumPaymentsPerTransaction)
+        totalPaid < roundedAmount && numPayments < loanMaximumPaymentsPerTransaction)
     {
         TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
         TenthBips32 const overpaymentFeeRate{loan->at(sfOverpaymentFee)};
@@ -1742,7 +1743,7 @@ loanMakePayment(
         // It shouldn't be possible for the overpayment to be greater than
         // 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);
+        Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
 
         detail::ExtendedPaymentComponents const overpaymentComponents = detail::computeOverpaymentComponents(
             asset, loanScale, overpayment, overpaymentInterestRate, overpaymentFeeRate, managementFeeRate);

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 xrpl {
@@ -397,9 +398,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, asset, FreezeHandling::fhIGNORE_FREEZE, AuthHandling::ahIGNORE_AUTH, j_);
 
@@ -418,16 +420,6 @@ LoanPay::doApply()
         "xrpl::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
                      << ", total paid to broker: " << totalPaidToBroker << ", amount from transaction: " << amount;
@@ -450,12 +442,67 @@ 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_.warn()) << "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_, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_, SpendableHandling::shFULL_BALANCE);
     auto const vaultBalanceBefore = account_ == vaultPseudoAccount
@@ -465,7 +512,6 @@ LoanPay::doApply()
     auto const brokerBalanceBefore = account_ == brokerPayee
         ? STAmount{asset, 0}
         : accountHolds(view, brokerPayee, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_, SpendableHandling::shFULL_BALANCE);
-#endif
 
     if (totalPaidToVaultRounded != beast::zero)
     {
@@ -499,14 +545,17 @@ 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,
-        "xrpl::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_, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_, SpendableHandling::shFULL_BALANCE);
     auto const vaultBalanceAfter = account_ == vaultPseudoAccount
@@ -516,13 +565,91 @@ LoanPay::doApply()
     auto const brokerBalanceAfter = account_ == brokerPayee
         ? STAmount{asset, 0}
         : accountHolds(view, brokerPayee, asset, fhIGNORE_FREEZE, ahIGNORE_AUTH, j_, SpendableHandling::shFULL_BALANCE);
+    auto const balanceScale = [&]() {
+        // Find the maximum exponent of all the non-zero balances, before and after.
+        // This is so ugly.
+        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());
+        }
+        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,
+        totalBalanceBeforeRounded == totalBalanceAfterRounded || totalBalanceChangeRounded == beast::zero,
         "xrpl::LoanPay::doApply",
         "funds are conserved (with rounding)");
-    XRPL_ASSERT_PARTS(accountBalanceAfter >= beast::zero, "xrpl::LoanPay::doApply", "positive account balance");
+
     XRPL_ASSERT_PARTS(
         accountBalanceAfter < accountBalanceBefore || account_ == asset.getIssuer(),
         "xrpl::LoanPay::doApply",
@@ -539,7 +666,6 @@ LoanPay::doApply()
         vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore,
         "xrpl::LoanPay::doApply",
         "vault and/or broker balance increased");
-#endif
 
     return tesSUCCESS;
 }