fix: Unauthorized trust lines may only return funds to the issuer

[ckeshava]

High Level Overview of Change

Under fixCleanup3_4_0, a trust line whose issuer requires authorization (lsfRequireAuth) but which lacks the issuer-side lsfLowAuth/lsfHighAuth flag can no longer move funds anywhere except back to the issuer. Addresses #5450 — both halves (the invariant and the accountHolds change) ship here under the same amendment.

Enforcement happens at three layers:

• ValidTrustLineAuth invariant — backstops every current and future transactor. Balance changes are grouped per (currency, issuer) into senders and receivers. An unauthorized line gaining funds always fails; one spending funds fails whenever the group has a receiver — with no receivers the funds drained back to the issuer, the ledger shape of a holder redemption or a Clawback, the only permitted flows. Pseudo-account holders (AMM/vault/loan broker) are exempt, mirroring requireAuth; the pseudo-account marker must predate the transaction, and the auth flag is read from the pre-transaction state.
• accountHolds/accountFunds — now take a required AuthHandling parameter for IOUs. Under AuthHandling::ZeroIfUnauthorized + fixCleanup3_4_0, unauthorized balances read as zero for funding checks (offers, NFT trades, checks, escrow), so stale offers funded only by such balances are removed as unfunded. Clawback, DirectStepI redemption capacity, negative-balance guards, issuer-cashed checks, and display paths deliberately keep AuthHandling::IgnoreAuth.
• Payment engine — DirectStepI::check only lets an unauthorized balance be delivered to the issuer: spends past the issuer fail tecNO_AUTH at strand construction, and unauthorized receives fail tecNO_AUTH post-amendment, closing the historical zero-balance grandfather clause (which allowed zero-crossing mints).

Context of Change

Unauthorized trust lines with non-zero balances exist in ledger history, created through past authorization bypasses (e.g. NFT trades before fixEnforceNFTokenTrustlineV2). This PR restricts them to redemption and Clawback at every layer. Everything is gated on fixCleanup3_4_0 so both halves activate atomically; pre-amendment behavior is byte-identical (invariant violations are logged only, keeping historical replay deterministic) and pinned by tests.

Type of Change

• New feature (non-breaking change which adds functionality)
• Tests (you added tests for code that already exists, or your new feature included in this PR)

Test Plan

• InvariantsTrustLine grown to nine cases covering the invariant scenarios (unauthorized gains, third-party spends direct and via DEX offer crossing, redemption allowed, zero-crossing payments, missing issuer root, pseudo-account exemption including full AMM withdrawal), accountHolds authorization handling, DEX cash-out, zero-balance receive, own-issuance allowance, unauthorized check-cash in both eras, and an end-to-end legacy scenario: a balance acquired through the NFT gap, the amendment enabled mid-test, a stale offer removed as unfunded, spends and receives blocked, then redemption plus an issuer-cashed check drain the line.
• NFTokenAuth runs a four-configuration amendment matrix so all behaviors stay pinned.
• SetAuth, TrustSet, and PayStrand tests assert the amendment-conditional TERs; SetAuth's stale "Should be terNO_AUTH" comment is resolved and its ignored-features bug fixed.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 99.05660% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/libxrpl/ledger/helpers/TokenHelpers.cpp	92.3%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[depthfirst-app]

🟡 Severity: MEDIUM

The issuer's lsfRequireAuth flag is read from the post-transaction view, unlike the trust line's own auth flags (line 802: (before ? before : after)->isFlag(...)) and pseudo-account markers (line 827), which deliberately use pre-transaction state. A buggy transactor that clears lsfRequireAuth on the issuer while simultaneously crediting an unauthorized line would cause the invariant to see "no RequireAuth" in the post-state and continue past the entire issue group, bypassing the check entirely. The accountRoots_ map already stores pre-transaction roots and should be consulted here.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Replace the post-transaction view.read() lookup on line 852 with a lookup that consults the accountRoots_ map first (using before ? before : after for pre-transaction state), falling back to view.read() only for unmodified issuers. This mirrors the existing isPseudoHolder pattern at lines 823-830 and the trust-line auth-flag check at line 802, ensuring that a buggy transactor that clears lsfRequireAuth in the same transaction cannot suppress the invariant.

Replace:

auto const issuerSle = view.read(keylet::account(issue.account));

With an IIFE that checks accountRoots_ first:

auto const issuerSle = [&]() -> std::shared_ptr<SLE const> {
    if (auto const it = accountRoots_.find(issue.account);
        it != accountRoots_.end())
    {
        auto const& [before, after] = it->second;
        return before ? before : after;
    }
    return view.read(keylet::account(issue.account));
}();

Note: this also changes issuerKnown (line 860) to use pre-transaction state; for deleted issuers (AMM full withdrawal), the pre-state root is non-null so issuerKnown becomes true, but since AMM accounts do not set lsfRequireAuth the group is already skipped by the continue on line 854, preserving correct behavior.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change

	auto const issuerSle = view.read(keylet::account(issue.account));
	// Use pre-transaction state for the issuer's lsfRequireAuth flag,
	// consistent with trust-line auth flags and pseudo-account markers:
	// a same-transaction flag clear can only be a buggy transactor.
	auto const issuerSle = [&]() -> std::shared_ptr<SLE const> {
	if (auto const it = accountRoots_.find(issue.account);
	it != accountRoots_.end())
	{
	auto const& [before, after] = it->second;
	return before ? before : after;
	}
	return view.read(keylet::account(issue.account));
	}();

[shawnxie999]

Good work. Two things:

1. The current implementation might be over-complicated. We can simply re-use requireAuth from the helper functions to check authorization.
2. This doesn't check for the LPToken authorization bug. Whenever we observe a change in balance of an LPToken, we must also check the auth for the two underlying assets

[yinyiqian1]

We usually don't rely on invariant checks alone as a fix, because invariant checks are meant to be a final safeguard, if one is triggered, it means something has gone fundamentally wrong.
And AMM still needs to check if underlined assets are authorized.

[Copilot]

Pull request overview

This PR adds a new ledger invariant and supporting plumbing to ensure unauthorized trust lines (when an issuer has lsfRequireAuth but the line lacks issuer-side auth flags) cannot be used to move funds to third parties, permitting only “return-to-issuer” style flows (e.g. redemption / clawback). It also introduces an authorization-handling mode to accountHolds/accountFunds so funding-style reads can treat unauthorized balances as zero under fixCleanup3_4_0, and updates core transactors and tests accordingly.

Changes:

• Introduces ValidTrustLineAuth invariant to detect unauthorized trust line balance increases/spends (enforced under fixCleanup3_4_0, logged otherwise).
• Extends accountHolds/accountFunds with AuthHandling and updates transactors (NFT offers, escrow create, check cash, clawback) to use either ZeroIfUnauthorized or IgnoreAuth as appropriate.
• Updates payment engine direct-step checks and expands/adjusts unit tests to cover legacy vs post-amendment behavior and the new invariant backstop.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test/ledger/View_test.cpp	Updates accountHolds/accountFunds calls to pass AuthHandling to preserve prior test semantics.
src/test/ledger/PaymentSandbox_test.cpp	Updates sandbox balance assertions to use AuthHandling::IgnoreAuth.
src/test/jtx/impl/AMM.cpp	Ensures LP token balance queries ignore auth when reading holdings.
src/test/app/TrustSet_test.cpp	Adds a pre/post-fixCleanup3_4_0 behavior matrix for unauthorized payments.
src/test/app/SetAuth_test.cpp	Parameterizes expected TER based on fixCleanup3_4_0 and expands feature matrix.
src/test/app/PayStrand_test.cpp	Adjusts expected unauthorized-receive result based on fixCleanup3_4_0.
src/test/app/NFTokenAuth_test.cpp	Expands tests to cover legacy/invariant/transactor-fix combinations and updated TER ordering.
src/test/app/invariants/InvariantsTrustLine_test.cpp	Adds comprehensive invariant tests for unauthorized trust line flows, DEX, checks, pseudo-accounts, and legacy states.
src/libxrpl/tx/transactors/token/Clawback.cpp	Uses AuthHandling::IgnoreAuth so clawback can see/drain unauthorized balances.
src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp	Uses ZeroIfUnauthorized for funding checks; uses IgnoreAuth for negative-balance guards.
src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp	Uses ZeroIfUnauthorized so unauthorized balances don’t fund IOU escrow creation.
src/libxrpl/tx/transactors/check/CheckCash.cpp	Uses IgnoreAuth only when issuer cashes (redemption), otherwise hides unauthorized balances.
src/libxrpl/tx/paths/DirectStep.cpp	Tightens unauthorized receive/spend rules under fixCleanup3_4_0, allowing only redemption-style use.
src/libxrpl/tx/invariants/InvariantCheck.cpp	Implements ValidTrustLineAuth tracking and enforcement logic.
src/libxrpl/ledger/helpers/TokenHelpers.cpp	Adds AuthHandling to IOU accountHolds and updates accountFunds to route through it.
src/libxrpl/ledger/helpers/NFTokenHelpers.cpp	Updates NFT offer preclaim funding checks to hide unauthorized balances.
include/xrpl/tx/invariants/InvariantCheck.h	Declares ValidTrustLineAuth and wires it into the invariant tuple.
include/xrpl/ledger/helpers/TokenHelpers.h	Updates helper signatures to include AuthHandling and removes the old accountFunds overload.

Suppressed comments (3)

src/test/app/invariants/InvariantsTrustLine_test.cpp:362

• This precheck lambda takes a2 but never uses it; omitting the parameter name avoids unused-parameter warnings and matches other tests in this directory.

            [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                return setHolderBalance(a1, ac, 100);
            },

src/test/app/invariants/InvariantsTrustLine_test.cpp:371

• This preclose lambda takes a2 but never uses it; omitting the parameter name avoids unused-parameter warnings and is consistent with other invariant-test setup lambdas.

            [&](Account const& a1, Account const& a2, Env& env) {
                env.fund(XRP(1000), g1);
                env.trust(g1["USD"](10000), a1);
                env.close();
                return true;
            });

src/test/app/invariants/InvariantsTrustLine_test.cpp:289

• This new section has multiple ApplyContext lambdas that take a2 but never use it (e.g. here). Omitting the parameter name avoids unused-parameter warnings and keeps the test consistent with other invariant tests.

            [&](Account const& a1, Account const& a2, ApplyContext& ac) {
                return setHolderBalance(a1, ac, 100);
            },

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.