From 73b68b6150f5f61d819c058657b1064d0835bc17 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Dec 2025 18:37:17 -0500 Subject: [PATCH] Enforce LoanBroker values keep precision when rounded to the asset type - Uncovered by associateAsset causing a test value to get rounded, causing tests which check that value to fail. --- src/test/app/LoanBroker_test.cpp | 46 +++++++++++++++----- src/xrpld/app/tx/detail/LoanBrokerSet.cpp | 53 ++++++++++++++++++----- src/xrpld/app/tx/detail/LoanBrokerSet.h | 3 ++ src/xrpld/app/tx/detail/LoanSet.cpp | 35 +++++++-------- 4 files changed, 95 insertions(+), 42 deletions(-) diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 597e2ea75b..be51d201b3 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -752,30 +752,36 @@ class LoanBroker_test : public beast::unit_test::suite // LoanBrokerID env(set(alice, vault.vaultID), loanBrokerID(nextKeylet.key), - ter(tecNO_ENTRY)); + ter(tecNO_ENTRY), + THISLINE); // VaultID env(set(alice, nextKeylet.key), loanBrokerID(broker->key()), - ter(tecNO_PERMISSION)); + ter(tecNO_ENTRY), + THISLINE); // Owner env(set(evan, vault.vaultID), loanBrokerID(broker->key()), - ter(tecNO_PERMISSION)); + ter(tecNO_PERMISSION), + THISLINE); // ManagementFeeRate env(set(alice, vault.vaultID), loanBrokerID(broker->key()), managementFeeRate(maxManagementFeeRate), - ter(temINVALID)); + ter(temINVALID), + THISLINE); // CoverRateMinimum env(set(alice, vault.vaultID), loanBrokerID(broker->key()), coverRateMinimum(maxManagementFeeRate), - ter(temINVALID)); + ter(temINVALID), + THISLINE); // CoverRateLiquidation env(set(alice, vault.vaultID), loanBrokerID(broker->key()), coverRateLiquidation(maxManagementFeeRate), - ter(temINVALID)); + ter(temINVALID), + THISLINE); // fields that can be changed testData = "Test Data 1234"; @@ -783,23 +789,43 @@ class LoanBroker_test : public beast::unit_test::suite env(set(alice, vault.vaultID), loanBrokerID(broker->key()), data(std::string(maxDataPayloadLength + 1, 'W')), - ter(temINVALID)); + ter(temINVALID), + THISLINE); // Bad debt maximum env(set(alice, vault.vaultID), loanBrokerID(broker->key()), debtMaximum(Number(-175, -1)), - ter(temINVALID)); + ter(temINVALID), + THISLINE); + Number debtMax{175, -1}; + if (vault.asset.integral()) + { + env(set(alice, vault.vaultID), + loanBrokerID(broker->key()), + data(testData), + debtMaximum(debtMax), + ter(tecPRECISION_LOSS), + THISLINE); + roundToAsset(vault.asset, debtMax); + } // Data & Debt maximum env(set(alice, vault.vaultID), loanBrokerID(broker->key()), data(testData), - debtMaximum(Number(175, -1))); + debtMaximum(debtMax), + THISLINE); }, [&](SLE::const_ref broker) { // Check the updated fields BEAST_EXPECT(checkVL(broker->at(sfData), testData)); - BEAST_EXPECT(broker->at(sfDebtMaximum) == Number(175, -1)); + Number const expected = + STAmount{vault.asset, Number(175, -1)}; + auto const actual = broker->at(sfDebtMaximum); + BEAST_EXPECTS( + actual == expected, + "Expected: " + to_string(expected) + + ", Actual: " + to_string(actual)); }); lifecycle( diff --git a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp index 49b55abd81..0c6e62f797 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp @@ -64,6 +64,15 @@ LoanBrokerSet::preflight(PreflightContext const& ctx) return tesSUCCESS; } +std::vector> const& +LoanBrokerSet::getValueFields() +{ + static std::vector> const valueFields{ + ~sfDebtMaximum}; + + return valueFields; +} + TER LoanBrokerSet::preclaim(PreclaimContext const& ctx) { @@ -72,8 +81,24 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx) auto const account = tx[sfAccount]; auto const vaultID = tx[sfVaultID]; + auto const sleVault = ctx.view.read(keylet::vault(vaultID)); + if (!sleVault) + { + JLOG(ctx.j.warn()) << "Vault does not exist."; + return tecNO_ENTRY; + } + Asset const asset = sleVault->at(sfAsset); + + if (account != sleVault->at(sfOwner)) + { + JLOG(ctx.j.warn()) << "Account is not the owner of the Vault."; + return tecNO_PERMISSION; + } + if (auto const brokerID = tx[~sfLoanBrokerID]) { + // Updating an existing Broker + auto const sleBroker = ctx.view.read(keylet::loanbroker(*brokerID)); if (!sleBroker) { @@ -94,20 +119,24 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx) } else { - auto const sleVault = ctx.view.read(keylet::vault(vaultID)); - if (!sleVault) - { - JLOG(ctx.j.warn()) << "Vault does not exist."; - return tecNO_ENTRY; - } - if (account != sleVault->at(sfOwner)) - { - JLOG(ctx.j.warn()) << "Account is not the owner of the Vault."; - return tecNO_PERMISSION; - } - if (auto const ter = canAddHolding(ctx.view, sleVault->at(sfAsset))) + if (auto const ter = canAddHolding(ctx.view, asset)) return ter; } + + // Check that relevant values can be represented as the vault asset + // type. This is mostly only relevant for integral (non-IOU) types + for (auto const& field : getValueFields()) + { + if (auto const value = tx[field]; + value && STAmount{asset, *value} != *value) + { + JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value + << ") can not be represented as a(n) " + << to_string(asset) << "."; + return tecPRECISION_LOSS; + } + } + return tesSUCCESS; } diff --git a/src/xrpld/app/tx/detail/LoanBrokerSet.h b/src/xrpld/app/tx/detail/LoanBrokerSet.h index 39ed9bcd61..4e17a0adbe 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerSet.h +++ b/src/xrpld/app/tx/detail/LoanBrokerSet.h @@ -20,6 +20,9 @@ public: static NotTEC preflight(PreflightContext const& ctx); + static std::vector> const& + getValueFields(); + static TER preclaim(PreclaimContext const& ctx); diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 8bd29debef..3222c1483f 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -291,17 +291,15 @@ LoanSet::preclaim(PreclaimContext const& ctx) // This check is almost duplicated in doApply, but that check is done after // the overall loan scale is known. This is mostly only relevant for // integral (non-IOU) types + for (auto const& field : getValueFields()) { - for (auto const& field : getValueFields()) + if (auto const value = tx[field]; + value && STAmount{asset, *value} != *value) { - if (auto const value = tx[field]; - value && STAmount{asset, *value} != *value) - { - JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value - << ") can not be represented as a(n) " - << to_string(asset) << "."; - return tecPRECISION_LOSS; - } + JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value + << ") can not be represented as a(n) " + << to_string(asset) << "."; + return tecPRECISION_LOSS; } } @@ -410,19 +408,16 @@ LoanSet::doApply() // Check that relevant values won't lose precision. This is mostly only // relevant for IOU assets. + for (auto const& field : getValueFields()) { - for (auto const& field : getValueFields()) + if (auto const value = tx[field]; + value && !isRounded(vaultAsset, *value, properties.loanScale)) { - if (auto const value = tx[field]; - value && !isRounded(vaultAsset, *value, properties.loanScale)) - { - JLOG(j_.warn()) - << field.f->getName() << " (" << *value - << ") has too much precision. Total loan value is " - << properties.totalValueOutstanding << " with a scale of " - << properties.loanScale; - return tecPRECISION_LOSS; - } + JLOG(j_.warn()) << field.f->getName() << " (" << *value + << ") has too much precision. Total loan value is " + << properties.totalValueOutstanding + << " with a scale of " << properties.loanScale; + return tecPRECISION_LOSS; } }