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

[ckeshava]

High Level Overview of Change

Adds a ValidTrustLineAuth invariant: a trust line whose issuer requires authorization (lsfRequireAuth) but which lacks the issuer-side lsfLowAuth/lsfHighAuth flag may only move funds back to the issuer.

Partially addresses #5450 (the invariants half; the accountHolds change will follow separately, ideally under the same amendment).

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 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.
• An unauthorized line 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-accounts (AMM/vault/loan broker) are implicitly authorized, mirroring requireAuth; the marker must predate the transaction, and a pseudo-account deleted together with its lines (full AMM withdrawal) stays exempt.
• Enforcement is gated on fixCleanup3_4_0 (violations become tecINVARIANT_FAILED); without the amendment violations are logged only, keeping historical replay deterministic.

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

• New InvariantsTrustLine cases: unauthorized gains, third-party spends (direct and via DEX offer crossing), redemption allowed, zero-crossing payments, missing issuer root (fail closed), pseudo-account exemption including full AMM withdrawal — each documenting both pre- and post-fixCleanup3_4_0 behavior.
• NFTokenAuth now runs a three-configuration matrix (legacy / invariant backstop / transactor fix) so all behaviors stay pinned.
• Full unit test suite passes (247 suites, 1,276,027 tests, 0 failures).

🤖 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.