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.

🟠 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 👍 / 👎

if (loanSle->isFlag(lsfLoanPending))
{
    JLOG(ctx.j.warn()) << "Loan is pending acceptance. A pending loan can not be managed.";
    return tecNO_PERMISSION;
}

Missing view.update(brokerSle) — changes to sfDebtTotal and owner count are silently dropped without it:

        adjustLoanBrokerOwnerCount(view, brokerSle, -1, j_);
        view.update(brokerSle);

adjustLoanBrokerOwnerCount calls view.update

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:

1. The method is read-only, thus all parameters should be read only. I.e. ReadView instead of ApplyView, SLE::const_ref instead of SLE::ref.
2. The method requires a bunch of redundant arguments. LoanProperites already contain LoanState, which in-turn already contains principalRequested.

Since the function requires so many parameters, it is probably doing too many things at once. I'd recommend to split it up further:

1. A method to validate valueFieldsare rounded at Loan scale.
2. Method to validate that the Loan would not exceed Vault and Broker asset limits.
3. Method to ensure there are sufficient funds for the FLC.
4. Method to check LoanGuards and that the computed loan properties are valid.

Good idea. Will create a separate branch and a PR for it. Thanks, Vito!

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

1. isGlobalFrozen
2. checkIndividualFrozen(vaultPseudo)
3. `checkDeepFrozen(brokerPseudo)
4. checkIndividualFrozen(borrower)
5. checkDeepFrozen(brokerOwner)

This is copied from LoanSet. I guess it'll be a good idea to put the changes in a separate branch. How do you think?

Isn't this check used across both, LoanSet and LoanAccept transactions? 🤔

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.

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.

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.

Same as checkLoanLimits I suggest to move this refactoring to a separate PR to keep this PR smaller.

This function is only called from the LoanSet transaction, it's an encapsulation for a single call, perhaps it's not needed?

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;

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);
}

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.

Please add comments to the added functions referencing if applicable Lending Protocol specifications.

This check contradicts the one above for "Special case for Batch inner transactions". According to the check above, batch inner transaction is valid if Counterparty is present and CounterpartySignature is not. This added check fails for exactly the same condition.

I think this check belongs in LoanSet::checkExtraFeatures.

Doesn't have to be a lambda.

Don't need featureEnabled since it's only used once.