feat: Split LoanSet and LoanAccept#7820
Conversation
a1q123456
changed the title
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two issues flagged inline: a debug test narrowing that skips most amendment combinations (high — must revert before merge), and an auth bypass in LoanSet::checkSign where counterparty signature validation can be skipped by crafting transaction fields to satisfy isTwoStepFlow regardless of the on-ledger loan's origination flow.
Review by Claude Sonnet 4.6 · Prompt: V15
| runAmendmentIndependent(); | ||
| for (auto const& features : jtx::amendmentCombinations( | ||
| {fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2}, all_)) | ||
| // runAmendmentIndependent(); |
There was a problem hiding this comment.
Debug narrowing left in — runAmendmentIndependent() is commented out and the amendment combination matrix was reduced to {featureLendingProtocolV1_1} only, skipping all existing fixCleanup3_1_3/fixCleanup3_2_0/featureMPTokensV2 combinations. Restore before merging:
runAmendmentIndependent();
for (auto const& features : jtx::amendmentCombinations(
{fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2, featureLendingProtocolV1_1}, all_))
runAmendmentSensitive(features);
|
|
||
| // In the two-step (Borrower) flow introduced by V1.1 there is no | ||
| // counterparty, so there is no CounterpartySignature to check. | ||
| if (isTwoStepFlowEnabled(ctx.view.rules()) && isTwoStepFlow(ctx.tx)) |
There was a problem hiding this comment.
Auth bypass: isTwoStepFlow() checks only tx fields the submitter controls, not the on-ledger loan object. A malicious actor can craft a tx with sfBorrower+sfStartDate targeting a one-step loan and skip counterparty signature validation. Verify the origination flow against the ledger object, not solely the incoming transaction fields.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| loan->at(sfNextPaymentDueDate) = startDate + paymentInterval; | ||
| loan->at(sfPaymentRemaining) = paymentTotal; | ||
| if (twoStepFlow) | ||
| loan->setFlag(lsfLoanPending); |
There was a problem hiding this comment.
🟠 Severity: HIGH
LoanManage::preclaim does not check lsfLoanPending, allowing the broker owner to default a pending loan via LoanManage. This zeroes PrincipalOutstanding and liquidates cover. Subsequent LoanAccept or pending-path LoanDelete then double-reverses vault/broker accounting, corrupting ledger state.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a guard in LoanManage::preclaim (in LoanManage.cpp) to reject transactions when the loan has the lsfLoanPending flag set. Insert the following check after the lsfLoanDefault check (around line 88 in LoanManage.cpp):
if (loanSle->isFlag(lsfLoanPending))
{
JLOG(ctx.j.warn()) << "Loan is pending acceptance. A pending loan can not be managed.";
return tecNO_PERMISSION;
}This prevents a broker owner from calling LoanManage with tfLoanDefault (or tfLoanImpair/tfLoanUnimpair) on a loan that has not yet been accepted by the borrower via LoanAccept. Without this guard, a pending loan can be defaulted, which improperly liquidates first-loss capital (cover) for a loan where principal was never disbursed, permanently inflates sfAssetsReserved, and corrupts vault state.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7820 +/- ##
========================================
Coverage 82.5% 82.5%
========================================
Files 1039 1042 +3
Lines 80446 80713 +267
Branches 9080 9086 +6
========================================
+ Hits 66332 66573 +241
- Misses 14105 14131 +26
Partials 9 9
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
7849e48 to
14a1bb2
Compare
| -(principalOutstanding + state.interestDue), | ||
| vaultAsset, | ||
| vaultScale); | ||
| adjustLoanBrokerOwnerCount(view, brokerSle, -1, j_); |
There was a problem hiding this comment.
Missing view.update(brokerSle) — changes to sfDebtTotal and owner count are silently dropped without it:
adjustLoanBrokerOwnerCount(view, brokerSle, -1, j_);
view.update(brokerSle);
14a1bb2 to
e32b303
Compare
| } | ||
|
|
||
| TER | ||
| checkLoanLimits( |
There was a problem hiding this comment.
While this method is a nice refactoring, at the moment it's called only in LoanSet. I suggest to make this refactoring in a separate PR.
There are a few things that should be improved with this:
- The method is read-only, thus all parameters should be read only. I.e.
ReadViewinstead ofApplyView,SLE::const_refinstead ofSLE::ref. - The method requires a bunch of redundant arguments.
LoanProperitesalready containLoanState, which in-turn already containsprincipalRequested.
Since the function requires so many parameters, it is probably doing too many things at once. I'd recommend to split it up further:
- A method to validate
valueFieldsare rounded at Loan scale. - Method to validate that the Loan would not exceed Vault and Broker asset limits.
- Method to ensure there are sufficient funds for the FLC.
- Method to check LoanGuards and that the computed loan properties are valid.
| { | ||
| if (auto const ter = canAddHolding(view, asset)) | ||
| return ter; | ||
|
|
There was a problem hiding this comment.
There are redundant checks in this method:
checkFrozen checks whether an asset is globally frozen, and then checks if a recursion is needed of the asset is an MPT.
I suggest to refactor this code as
isGlobalFrozencheckIndividualFrozen(vaultPseudo)- `checkDeepFrozen(brokerPseudo)
checkIndividualFrozen(borrower)checkDeepFrozen(brokerOwner)
| if (auto const ter = | ||
| reserveLoanOwner(view, brokerOwner, brokerOwnerSle, accountID_, preFeeBalance_, j_)) | ||
| return ter; | ||
| } |
There was a problem hiding this comment.
Unlike the immediate flow, LoanSet::doApply's twoStepFlow branch never calls requireAuth for the named borrower against vaultAsset — that check only happens later, inside disburseLoan during LoanAccept. So a broker owner can propose a two-step loan to a borrower who isn't (or never becomes) authorized to hold the vault asset; the loan sits pending, holding the broker's reserve and the vault's committed principal, until it either expires or someone runs LoanDelete.
| } | ||
|
|
||
| TER | ||
| LoanSet::doApply() |
There was a problem hiding this comment.
separate one-step / two-step in LoanSet::doApply
doApply interleaves the two flows as four separate if (twoStepFlow) branches — borrower resolution (479-487), reserve/disburse (555-586), pending-flag/vault bookkeeping (630, 639-640), and directory link (656-660). Reading any one flow means filtering out the other inline.
Proposal: extract only the two branches with real behavior — borrower resolution and reserve/disburse — into anonymous-namespace helpers:
LoanParticipants resolveParticipants(tx, twoStepFlow, brokerOwner, accountID); // {borrower, counterparty}
TER createPendingLoan(view, brokerOwner, brokerOwnerSle, accountID, preFeeBalance, j);
TER createImmediateLoan(viewContext, participants, borrowerSle, brokerOwner, brokerOwnerSle,
vaultPseudo, vaultAsset, loanAssetsToBorrower, originationFee,
accountID, preFeeBalance, j);
doApply becomes: common setup → resolveParticipants → twoStepFlow ? createPendingLoan(...) : createImmediateLoan(...) → unchanged common loan/vault construction.
Leave as-is: the 3 single-line if (twoStepFlow) after loan construction (pending flag, reserved bucket, directory link). Splitting those into two parallel builder functions would duplicate ~40 lines of identical field-setting to save 3 lines of branching — worse trade.
Why free functions, not private methods: none touch this; keeping them anonymous-namespace matches the existing currentLedgerCloseTime helper and avoids header churn.
| } | ||
|
|
||
| NotTEC | ||
| LoanSet::preflight(PreflightContext const& ctx) |
There was a problem hiding this comment.
borrower resolution + authorization interleaved in one lambda (377-395)
std::expected<AccountID, TER> const maybeBorrower = & -> std::expected<AccountID, TER> {
if (twoStepFlow) { ... return tx[sfBorrower]; }
auto const counterparty = tx[~sfCounterparty].value_or(brokerOwner);
if (account != brokerOwner && counterparty != brokerOwner) { ... }
return counterparty == brokerOwner ? account : counterparty;
}();
Same shape as the doApply borrower-resolution lambda — worth extracting as a matching pair of named helpers for consistency and symmetry with the doApply refactor:
std::expected<AccountID, TER> resolveTwoStepBorrower(tx, account, brokerOwner, j);
std::expected<AccountID, TER> resolveOneStepBorrower(tx, account, brokerOwner, j);
called as twoStepFlow ? resolveTwoStepBorrower(...) : resolveOneStepBorrower(...). This is the one piece of branching logic in preclaim; naming it removes the need to read into the lambda body to know which flow's permission rule applies.
| } | ||
|
|
||
| TER | ||
| updateLoanBroker( |
There was a problem hiding this comment.
Same as checkLoanLimits I suggest to move this refactoring to a separate PR to keep this PR smaller.
| { | ||
| // Put the loan into the pseudo-account's directory | ||
| return dirLink(view, brokerPseudo, loan, sfLoanBrokerNode); | ||
| } |
There was a problem hiding this comment.
This function is only called from the LoanSet transaction, it's an encapsulation for a single call, perhaps it's not needed?
| auto vaultAvailableProxy = vaultSle->at(sfAssetsAvailable); | ||
| auto vaultReservedProxy = vaultSle->at(sfAssetsReserved); | ||
| auto vaultTotalProxy = vaultSle->at(sfAssetsTotal); | ||
| vaultAvailableProxy += principalOutstanding; | ||
| vaultReservedProxy -= principalOutstanding; | ||
| vaultTotalProxy -= state.interestDue; | ||
| view.update(vaultSle); |
There was a problem hiding this comment.
The proxy values aren't needed, they're good if we're modifying SLE somewhere in a separate method, but in this case, you can simplify the code by directly modifying the values in place:
e.g.
vaultSle->at(sfAssetsTotal) -= state.interestDue;
| // time and releases the owner reserve charged to the LoanBroker owner. It is | ||
| // only linked into the broker pseudo-account's directory, and the borrower | ||
| // was never charged a reserve. | ||
| if (loanSle->isFlag(lsfLoanPending)) |
There was a problem hiding this comment.
What do you think about splitting the LoanDelete::doApply into two distinct methods deletePendingLoan and deleteActiveLoan?
namespace {
// Two-step (pending) loan: reverse the vault bookkeeping and broker-owner
// reserve charged at proposal time; the loan was never linked to the Borrower.
TER
deletePendingLoan(
ApplyView& view,
SLE::ref loanSle,
SLE::ref brokerSle,
SLE::ref vaultSle,
AccountID const& brokerPseudoAccount,
Asset const& vaultAsset,
uint256 const& loanID,
beast::Journal j);
// Active (fully-paid) loan: unlink from both directories, forgive any
// dust debt if this was the broker's last loan, release the Borrower's
// reserve.
TER
deleteActiveLoan(
ApplyView& view,
SLE::ref loanSle,
SLE::ref brokerSle,
SLE::ref vaultSle,
AccountID const& brokerPseudoAccount,
Asset const& vaultAsset,
uint256 const& loanID,
beast::Journal j);
}
| static bool | ||
| isTwoStepFlowEnabled(Rules const& rules); | ||
| /* Returns true if the transaction is using the two-step flow. */ | ||
| static bool | ||
| isTwoStepFlow(STTx const& tx); | ||
| /* Returns true if the transaction is using the one-step flow. */ | ||
| static bool | ||
| isOneStepFlow(STTx const& tx); |
There was a problem hiding this comment.
These methods are only used in the LoanSet.cpp implementation, what about moving them to an anonymous namespace in LoanSet? That way the header won't have to be modified.
2 participants
High Level Overview of Change
This PR adds
LoanAccept, and modifiesLoanDeleteandLoanSetto support the two step flow introduced by XRPLF/XRPL-Standards#570.LoanAcceptContext of Change
XRPLF/XRPL-Standards#570
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)