fix: Add assorted Lending Protocol fixes#6678
Conversation
Tapanito
added
Amendment
AI Triage
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| doInvariantCheck( | ||
| {{"Loan Broker cover available is greater than pseudo-account asset balance"}}, | ||
| [&](Account const&, Account const&, ApplyContext& ac) { | ||
| auto sle = ac.view().peek(loanBrokerKeylet); |
There was a problem hiding this comment.
Test silently passes if fixSecurity3_1_3 isn't enabled — invariant block is never entered. Explicitly enable the amendment before this doInvariantCheck call.
The existing invariant only checks that sfCoverAvailable is not less than the pseudo-account balance. Add a complementary check, gated behind fixSecurity3_1_3, that sfCoverAvailable is also not greater than the balance. This ensures exact equality between the two values after every transaction. Add invariant tests for both the lower-bound and upper-bound checks across all three asset types (XRP, IOU, MPT).
Tapanito
force-pushed
the
tapanito/lending-assorted-fixes
branch
from
5153b12 to
4ea3e45
Compare
There was a problem hiding this comment.
Went over the changes
Four issues flagged inline: unconditional associateAsset on failed TER (LoanManage.cpp), a financial invariant only enforced post-amendment (LoanInvariant.cpp), a fee-semantic change from tem→tec that needs documentation (LoanPay.cpp), and a test state leak from a non-RAII feature-flag pattern (Loan_test.cpp). Also raised a question about whether the noop-path behavior change on the non-amended code path is intentional.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6678 +/- ##
=========================================
- Coverage 81.4% 81.4% -0.0%
=========================================
Files 999 999
Lines 74439 74450 +11
Branches 7564 7558 -6
=========================================
+ Hits 60621 60626 +5
- Misses 13818 13824 +6
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses several Lending Protocol correctness issues by aligning transaction outcomes and invariants with intended semantics, while adding regression tests and amendment-gated behavior for backward compatibility.
Changes:
- LoanManage: restructure
doApplyto always runassociateAssetafter flag-specific actions whenfixSecurity3_1_3is enabled. - LoanPay: return
tecNO_PERMISSION(amendment-gated) whentfLoanOverpaymentis requested on a loan that doesn’t allow overpayment. - LoanBroker invariant: add an amendment-gated upper-bound check to enforce
sfCoverAvailablematches the pseudo-account’s actual holdings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libxrpl/tx/transactors/lending/LoanManage.cpp | Refactors control flow so post-action asset association runs consistently under fixSecurity3_1_3. |
| src/libxrpl/tx/transactors/lending/LoanPay.cpp | Adjusts preclaim TER for non-permitted overpayment requests with amendment gating. |
| src/libxrpl/tx/invariants/LoanInvariant.cpp | Adds amendment-gated upper-bound invariant check for sfCoverAvailable vs pseudo-account holdings. |
| src/test/app/Loan_test.cpp | Updates/extends tests to validate new TER behavior and backward compatibility when disabling fixSecurity3_1_3. |
| src/test/app/Invariants_test.cpp | Adds invariant regression tests for cover-available imbalance cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
There was a problem hiding this comment.
Two high-severity issues flagged inline: a pre-amendment behavioral regression in LoanManage.cpp where the noop path silently drops associateAsset calls when fixSecurity3_1_3 is off, and a related concern about state mutation on failed transactions in the legacy branch. A medium-severity pre-amendment DoS vector via fee-free tem* submissions in LoanPay.cpp is noted (fix is correct, defence-in-depth suggested). Lower-severity items: the ttLOAN_BROKER_DELETE carve-out in the invariant needs explicit justification, and the invariant test suite is missing a negative case for the amendment-disabled path.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
Two issues flagged inline: a pre-amendment behavioral regression in LoanManage.cpp where associateAsset calls are silently dropped (potential security impact), and a test state leak in Loan_test.cpp where a missing RAII guard can corrupt subsequent test cases.
Review by Claude Opus 4.6 · Prompt: V12
shawnxie999
left a comment
shawnxie999
left a comment
There was a problem hiding this comment.
LGTM - left minor comment
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
| if (!BEAST_EXPECT(sle)) | ||
| return false; | ||
| // Pseudo-account holds 10 units, set cover to 5 | ||
| sle->at(sfCoverAvailable) = Number(5); |
There was a problem hiding this comment.
Maybe move the constants to beginning of the test and describe the pre-conditions, tests and testing strategy?
There was a problem hiding this comment.
Invariant test constants are inline, I think refactoring this for all tests is out of scope for this PR, and refactoring only some would make it confusing "why are some inline and other not"
| }(); | ||
|
|
||
| return tesSUCCESS; | ||
| if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result)) |
There was a problem hiding this comment.
The summary says:
LoanManage: call associateAsset on all flag paths — The previous code returned early from defaultLoan/impairLoan/unimpairLoan, skipping the associateAsset calls. Restructured doApply to capture the result and always call associateAsset on the Loan, LoanBroker, and Vault SLEs afterward (gated behind fixSecurity3_1_3).
The old code always called associateAsset on the path where no flags are set. The new code calls them only when amendment is set and no flags are set. Is that correct? I believe AI flagged the same thing. The summary is a bit confusing to me. It says call associateAsset on all flag paths, does that mean, call associateAsset on the path where all flags are set?
This changes behavior for amendment inactive flows. Maybe document that this is intentional?
There was a problem hiding this comment.
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;
}();
// Pre-amendment, associateAsset was only called on the noop (no flags)
// path. The flag paths returned early from defaultLoan/impairLoan/
// unimpairLoan before reaching the calls. Post-amendment, we call
// associateAsset on all successful paths — including the flag paths that
// previously skipped it.
if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result))
{
associateAsset(*loanSle, vaultAsset);
associateAsset(*brokerSle, vaultAsset);
associateAsset(*vaultSle, vaultAsset);
}
```
Now `associateAsset` is called an all successful paths.
There was a problem hiding this comment.
What is a "successful" path?
There was a problem hiding this comment.
A tesSUCCESS return code. I don't think that we need to call associateAsset on failure conditions as a failure does not modify the ledger state.
There was a problem hiding this comment.
Right. I believe I had some confusion because of the no-op comment. Since it wasn't clear to me that all of these defaultLoan/impairLoan/unimpairLoan functions can return tesSUCCESS. So, that means we are now calling associateAsset even when these functions return success (earlier we weren't, as flagged by AI as well). But the no-op comment followed by return tesSUCCESS suggested that the lambda returns tesSUCCESS when there's no-op (no flag set) only.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Left a comment otherwise LGTM
@pratikmankawde I see your comment received a response - are any follow-ups needed or is this PR good to go? |
bthomee
changed the title
Approved. |
Tapanito
changed the title
Tapanito
added
the
Ready to merge
bthomee
changed the title
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
ximinez
added
Needs 2 PRs (staging / develop)
and removed
Needs 2 PRs (staging / develop)
labels
##Summary
LoanManage: call
associateAsseton all flag paths — The previous code returned early fromdefaultLoan/impairLoan/unimpairLoan, skipping theassociateAssetcalls. RestructureddoApplyto capture the result and always callassociateAsseton the Loan, LoanBroker, and Vault SLEs afterward (gated behindfixSecurity3_1_3).LoanPay: return
tecNO_PERMISSIONinstead oftemINVALID_FLAG— The overpayment-on-non-overpayable-loan check inpreclaimreturnedtemINVALID_FLAG, but the flag itself is valid — the loan just doesn't permit it. Changed totecNO_PERMISSIONunderfixSecurity3_1_3to preserve backward compatibility.LoanBroker invariant: add upper-bound check on cover available — The existing invariant only verified
sfCoverAvailable >= accountHolds(pseudo-account). Added a complementarysfCoverAvailable <= accountHoldscheck (gated behindfixSecurity3_1_3) to enforce exact equality. The check is skipped forLoanBrokerDeletetransactions because the broker SLE is erased without zeroingsfCoverAvailable. Also cached theaccountHoldsresult to avoid a duplicate call.High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)