Fix touchy "funds are conserved" assertion in LoanPay #6231

ximinez · 2026-01-16T01:25:01Z

Add poorly named "Yield Theft via Rounding Manipulation" test

High Level Overview of Change

IOU rounding when a Vault and a Loan are at significantly different scales can lead to the appearance of lost funds. An assertion in LoanPay wants funds to be "conserved", but that isn't always possible.

Context of Change

The new unit test testYieldTheftRounding was submitted as a finding in ImmuneFi's attackathon. Due to earlier changes in how overpayments are calculated, the "theft" is now a non-issue, but the assertion still was causing problems.

Update the rounding to improve accuracy, and calculate the amounts in a few different ways so if one still rounds unexpectedly, one of the others will not. This sounds weird, and it is, but IOU rounding can be weird. "Sometimes you eat the bear, and sometimes the bear eats you."

Type of Change

Bug fix (non-breaking change which fixes an issue)

codecov · 2026-01-16T20:09:21Z

Codecov Report

❌ Patch coverage is 98.36066% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.3%. Comparing base (564a351) to head (08170ff).
⚠️ Report is 3 commits behind head on ximinez/release-3.1.0-rc2.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/LoanPay.cpp	98.3%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           ximinez/release-3.1.0-rc2   #6231     +/-   ##
===========================================================
- Coverage                       79.4%   79.3%   -0.0%     
===========================================================
  Files                            839     839             
  Lines                          71716   71772     +56     
  Branches                        8254    8271     +17     
===========================================================
+ Hits                           56914   56950     +36     
- Misses                         14802   14822     +20

Files with missing lines	Coverage Δ
include/xrpl/protocol/STAmount.h	`96.2% <ø> (ø)`
src/xrpld/app/misc/detail/LendingHelpers.cpp	`89.0% <100.0%> (+0.9%)`	⬆️
src/xrpld/app/tx/detail/LoanPay.cpp	`95.8% <98.3%> (+0.5%)`	⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add "Yield Theft via Rounding Manipulation" test, which used to reliably triggered it. The test now verifies that no "yield theft" occurs.

Tapanito · 2026-01-21T13:20:55Z

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

+
    XRPL_ASSERT_PARTS(
-        *assetsAvailableProxy <= *assetsTotalProxy,
+        assetsAvailableAfter <= *assetsTotalProxy,


Suggested change

assetsAvailableAfter <= *assetsTotalProxy,

assetsAvailableAfter <= assetsTotalAfter,

Tapanito · 2026-01-21T13:24:32Z

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

+        JLOG(j_.warn()) << "LoanPay: Vault assets available unchanged after "
+                           "rounding: Before: "
+                        << assetsAvailableBefore
+                        << ", After: " << assetsAvailableAfter;


nit:

Suggested change

JLOG(j_.warn()) << "LoanPay: Vault assets available unchanged after "

"rounding: Before: "

<< assetsAvailableBefore

<< ", After: " << assetsAvailableAfter;

JLOG(j_.warn())

<< "LoanPay: Vault assets available unchanged after rounding: "

<< "Before: " << assetsAvailableBefore

<< ", After: " << assetsAvailableAfter;

Tapanito · 2026-01-21T13:27:22Z

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

+    if (assetsAvailableAfter > *assetsTotalProxy)
+    {
+        // 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: " << *assetsTotalProxy;


Suggested change

if (assetsAvailableAfter > *assetsTotalProxy)

{

// 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: " << *assetsTotalProxy;

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;

Tapanito · 2026-01-21T13:32:10Z

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

+            std::minmax_element(exponents.begin(), exponents.end());
+        // IOU rounding can be interesting. Give a margin of error that reflects
+        // the orders of magnitude between the extremes.
+        if (!asset.integral() && *max < STAmount::cMaxOffset * 3 / 4)


I understand that you're using the 3 / 4 division to create a threshold that is an order of magnitude, but perhaps the magic numbers are better place somewhere else, or at least provided as a constant variable? Future generations (most likely one of us), might find this confusing.

How about using withinRelativeDistance()?

The tricky thing is how does one determine an acceptable delta?

Tapanito · 2026-01-21T13:34:41Z

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

              ahIGNORE_AUTH,
              j_,
              SpendableHandling::shFULL_BALANCE);
+    auto const balanceScale = [&]() {


Perhaps this change can be done separately, but wouldn't enforcing that there is no absolute balance change, is better served in Loan invariant?

Tapanito · 2026-01-21T13:36:31Z

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

+        JLOG(j_.warn())
+            << "LoanPay: Vault assets expected change, but unchanged after "
+               "rounding: Before: "
+            << assetsTotalBefore << ", After: " << assetsTotalAfter
+            << ", ValueChange: " << paymentParts->valueChange;


nit:

Suggested change

JLOG(j_.warn())

<< "LoanPay: Vault assets expected change, but unchanged after "

"rounding: Before: "

<< assetsTotalBefore << ", After: " << assetsTotalAfter

<< ", ValueChange: " << paymentParts->valueChange;

JLOG(j_.warn()) << "LoanPay: Vault assets total expected change, but "

"unchanged after rounding: "

<< "Before: " << assetsTotalBefore

<< ", After: " << assetsTotalAfter

<< ", ValueChange: " << paymentParts->valueChange;

Tapanito · 2026-01-21T13:37:42Z

src/test/app/Loan_test.cpp

+        log << "Periodic Payment: " << periodicPayment.value() << std::endl;
+        log << "Attack Payment:   " << attackPayment.value() << std::endl;
+        log << "Initial Vault Assets: " << initialVaultAssets << std::endl;


Can we remove these logs?

Tapanito · 2026-01-21T13:37:53Z

src/test/app/Loan_test.cpp

+                log << "[ALERT] Iteration " << i
+                    << ": YIELD THEFT CONFIRMED. Vault Delta: " << delta
+                    << std::endl;


Tapanito · 2026-01-21T13:38:01Z

src/test/app/Loan_test.cpp

+                log << "[INFO]  Iteration " << i << ": Normal Yield: " << delta
+                    << std::endl;


Tapanito · 2026-01-21T13:42:49Z

src/test/app/Loan_test.cpp

+        }();
+        Vault vault{env};
+        env(vault.deposit(
+            {.depositor = lender, .id = vaultId, .amount = iou(5'000'000)}));


I understand that this is copy pasted code, but it seems that this can be simplified.

The vault.deposit and loanBroker.coverDeposits are already performed in the createVaultAndBroker method. Thus these two operations can be removed, by passing the appropriate values to createVaultAndBroker, a few lines above.

gregtatcam · 2026-01-21T19:15:18Z

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

+            std::minmax_element(exponents.begin(), exponents.end());
+        // IOU rounding can be interesting. Give a margin of error that reflects
+        // the orders of magnitude between the extremes.
+        if (!asset.integral() && *max < STAmount::cMaxOffset * 3 / 4)


Could you please expand on the balance scale logic? Suppose min = 7 and max = 9 and it's IOU. Then new max = 9 + (9 - 7) = 11. Why is it a better scale? I would have expected the lowest scale to be the best.

shawnxie999 · 2026-01-21T19:43:33Z

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

              ahIGNORE_AUTH,
              j_,
              SpendableHandling::shFULL_BALANCE);
+    auto const balanceScale = [&]() {


currently these checks are in DEBUG mode. Do we want to bring some checks into RELEASE?

ximinez added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Jan 16, 2026

ximinez changed the title ~~Revert "Bugfix: Adds graceful peer disconnection (#5669)" (#5855)~~ [WIP] Fix touchy "funds are conserved" assertion in LoanPay Jan 16, 2026

ximinez force-pushed the ximinez/loanpay-assertion branch from 5a41799 to c1e161c Compare January 16, 2026 19:40

Fix touchy "funds are conserved" assertion in LoanPay

08170ff

- Add "Yield Theft via Rounding Manipulation" test, which used to reliably triggered it. The test now verifies that no "yield theft" occurs.

ximinez force-pushed the ximinez/loanpay-assertion branch from c1e161c to 08170ff Compare January 17, 2026 00:23

ximinez changed the title ~~[WIP] Fix touchy "funds are conserved" assertion in LoanPay~~ Fix touchy "funds are conserved" assertion in LoanPay Jan 17, 2026

ximinez marked this pull request as ready for review January 17, 2026 00:23

ximinez requested a review from a team as a code owner January 17, 2026 00:23

ximinez removed the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Jan 20, 2026

ximinez requested a review from Tapanito January 20, 2026 18:48

kennyzlei requested review from gregtatcam and shawnxie999 January 20, 2026 20:49

Tapanito requested changes Jan 21, 2026

View reviewed changes

Tapanito reviewed Jan 21, 2026

View reviewed changes

gregtatcam reviewed Jan 21, 2026

View reviewed changes

shawnxie999 reviewed Jan 21, 2026

View reviewed changes

ximinez changed the base branch from release-3.1 to ximinez/release-3.1.0-rc2 January 22, 2026 17:20

ximinez added the Blocked label Jan 22, 2026

Base automatically changed from ximinez/release-3.1.0-rc2 to release-3.1 January 22, 2026 23:41

ximinez changed the base branch from release-3.1 to ximinez/staging-3.1 January 27, 2026 23:58

	assetsAvailableAfter <= *assetsTotalProxy,
	assetsAvailableAfter <= assetsTotalAfter,

		log << "[INFO] Iteration " << i << ": Normal Yield: " << delta
		<< std::endl;

Fix touchy "funds are conserved" assertion in LoanPay #6231

Are you sure you want to change the base?

Fix touchy "funds are conserved" assertion in LoanPay #6231

Conversation

ximinez commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

codecov bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ximinez commented Jan 16, 2026 •

edited

Loading

codecov bot commented Jan 16, 2026 •

edited

Loading