diff --git a/include/xrpl/ledger/helpers/RippleStateHelpers.h b/include/xrpl/ledger/helpers/RippleStateHelpers.h index a0508d074f..9ee96b0060 100644 --- a/include/xrpl/ledger/helpers/RippleStateHelpers.h +++ b/include/xrpl/ledger/helpers/RippleStateHelpers.h @@ -239,8 +239,14 @@ canTransfer(ReadView const& view, Issue const& issue, AccountID const& from, Acc //------------------------------------------------------------------------------ /** - * Any transactors that call addEmptyHolding() in doApply must call - * canAddHolding() in preflight with the same View and Asset + * After fixCleanup3_4_0, if the destination already holds this IOU, returns + * tecDUPLICATE and does not consult issuer freeze or DefaultRipple. Freeze + * and DefaultRipple still apply on the create path (DefaultRipple off is + * terNO_RIPPLE). Transactors that may create a new holding in doApply must + * call canAddHolding() in preclaim only when that destination does not + * already hold the asset. Do not call canAddHolding() merely because + * addEmptyHolding() is invoked: that helper does not check whether a + * holding already exists. */ [[nodiscard]] TER addEmptyHolding( diff --git a/include/xrpl/ledger/helpers/TokenHelpers.h b/include/xrpl/ledger/helpers/TokenHelpers.h index 5153b43cb2..2a2f1b568e 100644 --- a/include/xrpl/ledger/helpers/TokenHelpers.h +++ b/include/xrpl/ledger/helpers/TokenHelpers.h @@ -319,6 +319,12 @@ transferRate(ReadView const& view, STAmount const& amount); [[nodiscard]] TER canAddHolding(ReadView const& view, Asset const& asset); +/** + * True if the account already holds this asset (or is the issuer / XRP). + */ +[[nodiscard]] bool +holdingExists(ReadView const& view, AccountID const& account, Asset const& asset); + [[nodiscard]] TER addEmptyHolding( ApplyViewContext ctx, diff --git a/merged-prs.md b/merged-prs.md index b6c2eedafe..ee720c91eb 100644 --- a/merged-prs.md +++ b/merged-prs.md @@ -2,9 +2,11 @@ PRs merged into the `ripple/lending-protocol-fv` branch. -| PR | Title | Author | Branch | Merged | -| --------------------------------------------------- | ----------------------------------------------- | ---------- | --------------------------------------------------------- | ---------- | -| [#6383](https://github.com/XRPLF/rippled/pull/6383) | feat: Add tfVaultDonate feature | @Tapanito | `tapanito/vault-donation` | 2026-09-02 | -| [#7820](https://github.com/XRPLF/rippled/pull/7820) | feat: Split LoanSet and LoanAccept | @a1q123456 | `a1q123456/split-loan-set-and-loan-accept-implementation` | 2026-09-02 | -| [#6528](https://github.com/XRPLF/rippled/pull/6528) | feat: Make VaultID conditional on LoanBrokerSet | @Tapanito | `tapanito/loan-broker-set` | 2026-09-02 | -| [#6361](https://github.com/XRPLF/rippled/pull/6361) | Adds functionality to block vault deposits | @Tapanito | `tapanito/vault-block-deposit` | 2026-09-02 | +| PR | Title | Author | Branch | Merged | +| --------------------------------------------------- | -------------------------------------------------------------------------- | ---------- | --------------------------------------------------------- | ---------- | +| [#6383](https://github.com/XRPLF/rippled/pull/6383) | feat: Add tfVaultDonate feature | @Tapanito | `tapanito/vault-donation` | 2026-09-02 | +| [#7820](https://github.com/XRPLF/rippled/pull/7820) | feat: Split LoanSet and LoanAccept | @a1q123456 | `a1q123456/split-loan-set-and-loan-accept-implementation` | 2026-09-02 | +| [#6528](https://github.com/XRPLF/rippled/pull/6528) | feat: Make VaultID conditional on LoanBrokerSet | @Tapanito | `tapanito/loan-broker-set` | 2026-09-02 | +| [#6361](https://github.com/XRPLF/rippled/pull/6361) | Adds functionality to block vault deposits | @Tapanito | `tapanito/vault-block-deposit` | 2026-09-02 | +| [#8153](https://github.com/XRPLF/rippled/pull/8153) | fix: Allow zero-value MPT vault withdraw when the asset holding is missing | @Tapanito | `tapanito/vault-zero-delta` | 2026-09-02 | +| [#8154](https://github.com/XRPLF/rippled/pull/8154) | fix: Treat an existing IOU line as a no-op in addEmptyHolding | @Tapanito | `tapanito/empty-holding` | 2026-09-02 | diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index 1b9bb19ad4..27dcd84675 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -184,6 +184,8 @@ addEmptyHolding( auto const mpt = ctx.view.peek(keylet::mptokenIssuance(mptID)); if (!mpt) return tefINTERNAL; // LCOV_EXCL_LINE + // Unlike IOU addEmptyHolding (post-fixCleanup3_4_0), a locked issuance is + // still rejected before the "MPToken already exists" short circuit. if (mpt->isFlag(lsfMPTLocked)) return tefINTERNAL; // LCOV_EXCL_LINE if (ctx.view.peek(keylet::mptoken(mptID, accountID))) diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index 706564db6f..cc02b56305 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -652,21 +652,32 @@ addEmptyHolding( auto const& issuerId = issue.getIssuer(); auto const& currency = issue.currency; - if (isGlobalFrozen(ctx.view, issuerId)) - return tecFROZEN; // LCOV_EXCL_LINE - auto const& srcId = issuerId; auto const& dstId = accountID; auto const high = srcId > dstId; auto const index = keylet::trustLine(srcId, dstId, currency); + // Post-fixCleanup3_4_0: an existing line is a no-op. Issuer freeze and + // DefaultRipple only matter when this function has to create a line. + bool const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0); + if (fix340Enabled && ctx.view.exists(index)) + return tecDUPLICATE; + + if (isGlobalFrozen(ctx.view, issuerId)) + return tecFROZEN; // LCOV_EXCL_LINE + auto const sleSrc = ctx.view.peek(keylet::account(srcId)); auto const sleDst = ctx.view.peek(keylet::account(dstId)); if (!sleDst || !sleSrc) return tefINTERNAL; // LCOV_EXCL_LINE + // Create path: DefaultRipple is still required. terNO_RIPPLE is + // intentional so VaultWithdraw / CoverWithdraw fail in preclaim via + // canAddHolding (retryable, no fee) rather than claiming a tec* fee + // in doApply. Transactor::operator() will not apply and will not + // convert it to tefINTERNAL. if (!sleSrc->isFlag(lsfDefaultRipple)) - return tecINTERNAL; // LCOV_EXCL_LINE + return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL; // If the line already exists, don't create it again. - if (ctx.view.read(index)) + if (!fix340Enabled && ctx.view.exists(index)) return tecDUPLICATE; // A reserve sponsor only covers tx.Account's own objects. diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index aaf99a3c0a..2c2c943a9f 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -583,6 +583,32 @@ canAddHolding(ReadView const& view, Asset const& asset) asset.value()); } +[[nodiscard]] bool +holdingExists(ReadView const& view, AccountID const& account, Issue const& issue) +{ + if (issue.native() || account == issue.getIssuer()) + return true; + return view.exists(keylet::trustLine(account, issue)); +} + +[[nodiscard]] bool +holdingExists(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue) +{ + if (account == mptIssue.getIssuer()) + return true; + return view.exists(keylet::mptoken(mptIssue.getMptID(), account)); +} + +[[nodiscard]] bool +holdingExists(ReadView const& view, AccountID const& account, Asset const& asset) +{ + return std::visit( + [&](TIss const& issue) -> bool { + return holdingExists(view, account, issue); + }, + asset.value()); +} + TER addEmptyHolding( ApplyViewContext ctx, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index e914596599..88b6f8c38b 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -65,6 +65,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) { auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0); + auto const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0); auto const& tx = ctx.tx; auto const account = tx[sfAccount]; @@ -140,6 +141,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType)) return ter; + if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset)) + { + if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter)) + return ter; + } + if (fix330Enabled) { if (auto const ret = diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index 9e5384b560..a33324252f 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -965,6 +965,19 @@ LoanSet::preclaim(PreclaimContext const& ctx) ctx.view, asset, vaultPseudo, brokerPseudo, borrower, brokerOwner, ctx.j)) return ter; + // canAddHolding does not look at existing lines. After + // fixCleanup3_4_0, addEmptyHolding is a no-op when the destination + // already holds the asset, so skip this gate unless a holding would + // actually be created (borrower always; broker owner if there is an + // origination fee). + auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{}); + if (!ctx.view.rules().enabled(fixCleanup3_4_0) || !holdingExists(ctx.view, borrower, asset) || + (originationFee != beast::kZero && !holdingExists(ctx.view, brokerOwner, asset))) + { + if (auto const ter = canAddHolding(ctx.view, asset)) + return ter; + } + if (twoStepFlow) { // Reject a pending loan up front if the borrower or broker owner (the diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 697612af3e..4dc5b95c89 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -205,6 +205,15 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter)) return ter; + // Fail early when self-destination would have to create a holding. + // Skip when a holding already exists: canAddHolding does not look at that, + // and would block a no-op create (the DefaultRipple-cleared self-withdraw). + if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset)) + { + if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter)) + return ter; + } + // The checks above only establish that an account may hold the asset. A // private vault additionally restricts who may take part in it, so paying // its asset out to a third party requires both ends of that payout to be diff --git a/src/test/app/lending/LoanPay_test.cpp b/src/test/app/lending/LoanPay_test.cpp index 038ef4067b..3360e68272 100644 --- a/src/test/app/lending/LoanPay_test.cpp +++ b/src/test/app/lending/LoanPay_test.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1419,6 +1420,69 @@ private: BEAST_EXPECT(stateAfter.nextPaymentDate == exactDueDate); } + // LoanPay does not call canAddHolding. addEmptyHolding recreates the + // broker-owner holding when the borrower is also the broker owner. After + // fixCleanup3_4_0 an existing line is a no-op even if DefaultRipple is + // off; pre-fix that path dies with tecINTERNAL. + void + testLoanPaySelfBrokerExistingLineDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanPay broker-owner borrower existing line after " + "issuer clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(alice, usd(100'000))); + env.close(); + env(pay(issuer, alice, usd(50'000))); + env.close(); + + auto const broker = createVaultAndBroker(env, usd, alice); + auto const brokerSle = env.le(keylet::loanBroker(broker.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return; + auto const loanKeylet = + keylet::loan(broker.brokerID, SeqProxy::rawSequence(brokerSle->at(sfLoanSequence))); + + Number const serviceFee = usd(2).value(); + env(set(alice, broker.brokerID, usd(1'000).value()), + Sig(sfCounterpartySignature, alice), + kLoanServiceFee(serviceFee), + Fee(env.current()->fees().base * 2)); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usd.raw().get()))); + + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const payment{ + usd, + roundPeriodicPayment(usd, state.periodicPayment + serviceFee, state.loanScale)}; + + env(pay(alice, loanKeylet.key, payment), Ter(expected)); + env.close(); + }; + + run(all_ - fixCleanup3_4_0, tecINTERNAL); + run(all_, tesSUCCESS); + } + void runAmendmentIndependent() { @@ -1429,6 +1493,7 @@ private: testLoanPayCatchUpFeeAtExactDueDatePostAmendment(); testLoanPayCatchUpFeeAtExactDueDatePreAmendment(); testRepayIntoUnauthorizedVault(); + testLoanPaySelfBrokerExistingLineDefaultRipple(); } // Tests run under each entry in amendmentCombinations(). diff --git a/src/test/app/lending/LoanSet_test.cpp b/src/test/app/lending/LoanSet_test.cpp index 6652c7a840..b871ecb154 100644 --- a/src/test/app/lending/LoanSet_test.cpp +++ b/src/test/app/lending/LoanSet_test.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -1101,6 +1102,65 @@ private: }); } + // LoanSet used to call canAddHolding unconditionally, so an existing + // borrower line still failed with terNO_RIPPLE after the issuer cleared + // DefaultRipple. After fixCleanup3_4_0, skip that gate when the holding + // already exists. + void + testLoanSetExistingLineAfterIssuerClearsDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanSet existing borrower line after issuer " + "clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000), issuer, lender, borrower); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(lender, usd(100'000))); + env(trust(borrower, usd(100'000))); + env.close(); + env(pay(issuer, lender, usd(50'000))); + env(pay(issuer, borrower, usd(1'000))); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(borrower.id(), usd.raw().get()))); + + auto const broker = createVaultAndBroker(env, usd, lender); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + Number const destBefore = env.balance(borrower, usd.raw()).number(); + env(set(borrower, broker.brokerID, usd(100).value()), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 2), + Ter(expected)); + env.close(); + + Number const destAfter = env.balance(borrower, usd.raw()).number(); + if (isTesSuccess(expected)) + BEAST_EXPECT(destAfter == destBefore + Number{100}); + else + BEAST_EXPECT(destAfter == destBefore); + }; + + run(all_ - fixCleanup3_4_0, terNO_RIPPLE); + run(all_, tesSUCCESS); + } + public: void run() override @@ -1111,6 +1171,7 @@ public: testTwoStepLoanSet(); testLoanSetClosedEnded(); + testLoanSetExistingLineAfterIssuerClearsDefaultRipple(); } }; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 0ff39bd9de..adefb33629 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -16,6 +17,7 @@ #include #include +#include #include #include #include @@ -2133,6 +2135,357 @@ private: runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); } + // addEmptyHolding() used to check isGlobalFrozen(issuer) and + // !lsfDefaultRipple before the "line already exists" tecDUPLICATE + // short circuit. doWithdraw() calls addEmptyHolding() for a + // self-destination payout and only tolerates tecDUPLICATE, so + // tecINTERNAL from a missing DefaultRipple flag aborted the + // withdrawal. fixCleanup3_4_0 checks existence first and maps the + // create-path DefaultRipple miss to terNO_RIPPLE. Global freeze on an + // existing line is still rejected later by checkWithdrawFreeze. + void + testBugSelfWithdrawAfterIssuerClearsDefaultRipple() + { + using namespace test::jtx; + + auto runExistingLine = [this]( + FeatureBitset features, + TER selfExpected, + bool issuerGlobalFreeze = false) { + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(10'000), issuer, alice, bob); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + Issue const usdIssue = usd.raw().get(); + env(trust(alice, usd(10'000))); + env(trust(bob, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(1'000))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)})); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + if (issuerGlobalFreeze) + { + env(fset(issuer, asfGlobalFreeze)); + env.close(); + } + + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usdIssue))); + + // Alice's USD line is unchanged; a later deposit still succeeds + // unless the issuer is globally frozen. + if (!issuerGlobalFreeze) + { + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(10)})); + env.close(); + } + + Number const destBefore = env.balance(alice, usd.raw()).number(); + Number const vaultBefore = env.le(vaultKeylet)->at(sfAssetsTotal); + Number const withdrawAmt{50}; + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}), + Ter(selfExpected)); + env.close(); + + Number const destAfter = env.balance(alice, usd.raw()).number(); + Number const vaultAfter = env.le(vaultKeylet)->at(sfAssetsTotal); + if (isTesSuccess(selfExpected)) + { + BEAST_EXPECT(destAfter == destBefore + withdrawAmt); + BEAST_EXPECT(vaultAfter == vaultBefore - withdrawAmt); + } + else + { + BEAST_EXPECT(destAfter == destBefore); + BEAST_EXPECT(vaultAfter == vaultBefore); + } + + if (!issuerGlobalFreeze) + { + auto destTx = + vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}); + destTx[sfDestination] = bob.human(); + env(destTx); + env.close(); + } + }; + + auto runDeletedLine = [this](FeatureBitset features, TER selfExpected) { + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + Issue const usdIssue = usd.raw().get(); + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(500))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + env(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.le(keylet::trustLine(alice.id(), usdIssue))); + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}), + Ter(selfExpected)); + env.close(); + }; + + auto runCoverWithdraw = [this](FeatureBitset features, TER selfExpected) { + using namespace loan_broker; + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + Issue const usdIssue = usd.raw().get(); + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(1'000))); + env.close(); + + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded( + {.owner = alice, .asset = usd, .subscriptionOffset = std::chrono::seconds{60}}); + (void)subscriptionDate; + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice, vaultKeylet.key)); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, usd(100).value())); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usdIssue))); + + Number const destBefore = env.balance(alice, usd.raw()).number(); + Number const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + Number const withdrawAmt{50}; + + env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected)); + env.close(); + + Number const destAfter = env.balance(alice, usd.raw()).number(); + Number const coverAfter = env.le(brokerKeylet)->at(sfCoverAvailable); + if (isTesSuccess(selfExpected)) + { + BEAST_EXPECT(destAfter == destBefore + withdrawAmt); + BEAST_EXPECT(coverAfter == coverBefore - withdrawAmt); + } + else + { + BEAST_EXPECT(destAfter == destBefore); + BEAST_EXPECT(coverAfter == coverBefore); + } + }; + + auto runDeletedCoverWithdraw = [this](FeatureBitset features, TER selfExpected) { + using namespace loan_broker; + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + Issue const usdIssue = usd.raw().get(); + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(600))); + env.close(); + + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded( + {.owner = alice, .asset = usd, .subscriptionOffset = std::chrono::seconds{60}}); + (void)subscriptionDate; + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice, vaultKeylet.key)); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, usd(100).value())); + env.close(); + + env(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.le(keylet::trustLine(alice.id(), usdIssue))); + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected)); + env.close(); + }; + + auto runPrivateVault = [this](FeatureBitset features, TER selfExpected) { + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const pdOwner{"pdOwner"}; + Account const credIssuer{"credIssuer"}; + std::string const credType = "credential"; + + env.fund(XRP(10'000), issuer, alice, pdOwner, credIssuer); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(1'000))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = + vault.create({.owner = alice, .asset = usd, .flags = tfVaultPrivate}); + env(vaultTx); + env.close(); + + pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}}; + env(pdomain::setTx(pdOwner, credentials)); + auto const domainId = pdomain::getNewDomain(env.meta()); + { + auto domainTx = vault.set({.owner = alice, .id = vaultKeylet.key}); + domainTx[sfDomainID] = to_string(domainId); + env(domainTx); + env.close(); + } + + env(credentials::create(alice, credIssuer, credType)); + env(credentials::accept(alice, credIssuer, credType)); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}), + Ter(selfExpected)); + env.close(); + }; + + testcase( + "bug: VaultWithdraw to self fails with tecINTERNAL after issuer " + "clears asfDefaultRipple even though the trust line exists " + "(pre-fixCleanup3_4_0)"); + runExistingLine(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: VaultWithdraw to self succeeds after issuer clears " + "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); + runExistingLine(all_, tesSUCCESS); + + testcase( + "bug: VaultWithdraw to self with an existing line still gets " + "tecFROZEN under asfGlobalFreeze (post-fixCleanup3_4_0)"); + runExistingLine(all_, tecFROZEN, true); + + testcase( + "bug: VaultWithdraw to self fails with tecINTERNAL after issuer " + "clears asfDefaultRipple and the trust line was deleted " + "(pre-fixCleanup3_4_0)"); + runDeletedLine(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: VaultWithdraw to self fails with terNO_RIPPLE after issuer " + "clears asfDefaultRipple and the trust line was deleted " + "(post-fixCleanup3_4_0)"); + runDeletedLine(all_, terNO_RIPPLE); + + testcase( + "bug: LoanBrokerCoverWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple even though the trust line exists " + "(pre-fixCleanup3_4_0)"); + runCoverWithdraw(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: LoanBrokerCoverWithdraw to self succeeds after issuer clears " + "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); + runCoverWithdraw(all_, tesSUCCESS); + + testcase( + "bug: LoanBrokerCoverWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple and the trust line was deleted " + "(pre-fixCleanup3_4_0)"); + runDeletedCoverWithdraw(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: LoanBrokerCoverWithdraw to self fails with terNO_RIPPLE after " + "issuer clears asfDefaultRipple and the trust line was deleted " + "(post-fixCleanup3_4_0)"); + runDeletedCoverWithdraw(all_, terNO_RIPPLE); + + testcase( + "bug: private VaultWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple even though the trust line exists " + "(pre-fixCleanup3_4_0)"); + runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: private VaultWithdraw to self succeeds after issuer clears " + "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); + runPrivateVault(all_, tesSUCCESS); + } + // Bug 1: a sponsored XRP VaultWithdraw to a distinct destination is // rejected because the vault invariant treats the holder's touched // but economically unchanged AccountRoot as a second payout @@ -2468,34 +2821,369 @@ private: } public: - void - run() override - { - testVaultWithdrawEqualityEnforced(); - testBugIssuerVaultDepositAtEdge(); - testBugMakeDeltaPosteriorScale(); - testBugMakeDeltaAnteriorScale(); - testVaultDepositCanonicalizeToZero(); - testBugDepositShareTruncationSubUlp(); - testVaultWithdrawCanonicalizeToZero(); - testBugVaultDustDebitCanonicalizesToNoOp(); - testBugVaultDepositOvercreditsAcrossScaleBoundary(); - testBugVaultLockedByPartialWithdraw(); - testVaultDepositNegativeBalanceFromOppositeLimit(); - testCredentialPinsPseudoAccount(); - testCredentialPinOverflow(); - testBug6LimitBypassWithShares(); - testBugClawbackRoundTripOvershoot(); - testBugWithdrawRoundTripOvershoot(); - testBugClawbackAfterLoanImpair(); - testBugMptZeroWithdrawMissingHolding(); - testBugIouZeroWithdrawMissingTrustLine(); - testBugXrpZeroWithdrawSponsoredFee(); - testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); - testBugSponsorAsDestinationFeeMisappliedToPayout(); - testPrefundedFeeWithdraw(); - testUnsponsoredWithdrawToDistinctDestinationPreAmendment(); - } +} + +// Bug 1: a sponsored XRP VaultWithdraw to a distinct destination is +// rejected because the vault invariant treats the holder's touched +// but economically unchanged AccountRoot as a second payout +// recipient. Sequence/ticket processing still touches the holder +// while the sponsor pays the fee, so the holder's XRP delta is +// present-zero and is not normalized away. If this happens on the +// last Subscription ledger of a closed-ended vault, the holder +// cannot retry until Redemption (tecTOO_SOON during Investment). +// +// Fixed by ValidVault::deltaAssetsForParty always collapsing an +// economically-zero XRP delta to absence, regardless of who paid the +// fee. +void +testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient() +{ + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, destination, sponsor); + env.close(); + + constexpr std::uint32_t investmentPeriod = 14u * 24u * 60u * 60u; + auto const [vault, vaultKeylet, subscriptionDate, redemptionDate] = + makeClosedEndedVault(env, owner, xrpIssue(), 120u, investmentPeriod); + BEAST_EXPECT(redemptionDate - subscriptionDate == investmentPeriod); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + // Inclusive SubscriptionDate boundary: still Subscription, so an + // ordinary withdrawal is allowed. + closeToTime(env, tp{d{subscriptionDate}}); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = destination.human(); + env(withdraw, + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); + return; + } + + // Invariant rollback: the payout and share burn are undone, but + // sequence processing and the sponsored fee charge remain. + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore); + + // Once the ledger advances into Investment, the same holder + // cannot retry until Redemption. + auto retry = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + retry[sfDestination] = destination.human(); + env(retry, Ter(tecTOO_SOON)); + }; + + testcase( + "bug: sponsored XRP withdrawal to a distinct destination misreads a " + "touched-but-zero sender delta as a second recipient " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + + testcase( + "bug: sponsored XRP withdrawal to a distinct destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS); +} + +// Bug 2: a co-signed fee sponsor named as the withdrawal's own +// destination pays its fee from the same AccountRoot it is paid into, +// so its net XRP delta is (payout - fee). The invariant never fee- +// corrected the destination side at all, so this always failed the +// equal-amount check against the vault's outflow (payout). +// +// Fixed by ValidVault::deltaAssetsForParty adding the fee back onto +// whichever inspected party's AccountRoot actually paid it -- the +// sender, or a distinct destination -- not just the sender. +void +testBugSponsorAsDestinationFeeMisappliedToPayout() +{ + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, sponsor); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + // The sponsor both receives the withdrawal (as sfDestination) + // and pays its own fee (co-signed) from the same AccountRoot. + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = sponsor.human(); + env(withdraw, + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + // Paid the withdrawal, then separately debited for the fee + // it chose to cover; net effect is payout minus fee. + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore + XRP(100) - fee); + return; + } + + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + }; + + testcase( + "bug: co-signed sponsor named as withdrawal destination has its " + "own fee debit misread as breaking the payout equality " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + + testcase( + "bug: co-signed sponsor named as withdrawal destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS); +} + +// Pre-funded fee sponsorship draws the fee from ltSponsorship.sfFeeAmount, +// so feePayerAccountRoot must return nullopt rather than the sponsor's +// AccountRoot. A bystander sponsor leaves that branch unexercised: the +// result is only consulted by deltaAssetsForParty via `payer && *payer == +// id`. Naming the sponsor as sfDestination makes the early return +// load-bearing -- returning the sponsor's id instead of nullopt would add +// the fee back onto a balance that never paid it, and the equal-amount +// check against the vault outflow would fail. +// +// Contrast testBugSponsorAsDestinationFeeMisappliedToPayout, where the +// sponsor co-signs and so really does pay from its own AccountRoot. +void +testPrefundedFeeWithdraw() +{ + using namespace test::jtx; + + auto runScenario = [this]( + FeatureBitset features, TER expected, bool const sponsorIsDestination) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, destination, sponsor); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const fee = env.current()->fees().base; + env(sponsor::set_fee(sponsor, 0, fee), sponsor::SponseeAcc(holder)); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const sponsorBalanceBefore = env.balance(sponsor); + + Account const& recipient = sponsorIsDestination ? sponsor : destination; + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = recipient.human(); + env(withdraw, Fee(fee), sponsor::As(sponsor, spfSponsorFee), Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + // Holder is economically unchanged (sequence only); the fee is + // taken from the sponsorship object, not any AccountRoot. + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + if (sponsorIsDestination) + { + // The sponsor receives the payout and is not debited for + // the fee. The sponsor has to BE the destination for + // FeePayerType::SponsorPreFunded to matter. + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore + XRP(100)); + } + else + { + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore); + } + auto const sponsorship = env.le(keylet::sponsorship(sponsor, holder)); + if (!BEAST_EXPECT(sponsorship)) + return; + BEAST_EXPECT(!sponsorship->isFieldPresent(sfFeeAmount)); + return; + } + + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore); + if (!sponsorIsDestination) + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore); + }; + + testcase( + "pre-funded fee XRP withdrawal to a distinct destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS, false); + + testcase( + "bug: pre-funded sponsor named as withdrawal destination misreads " + "the sender's touched-but-zero delta as a second recipient " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED, true); + + testcase( + "bug: pre-funded sponsor named as withdrawal destination receives " + "the full payout (post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS, true); +} + +// Unsponsored third-party XRP withdrawal: the sender's AccountRoot moves +// by exactly -fee. Pre-amendment, the sender-only fee correction then +// collapses that to absence so the dual-recipient guard does not fire. +void +testUnsponsoredWithdrawToDistinctDestinationPreAmendment() +{ + using namespace test::jtx; + + testcase( + "unsponsored XRP withdrawal to a distinct destination succeeds " + "(pre-fixCleanup3_4_0)"); + + Env env{*this, all_ - fixCleanup3_4_0}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + env.fund(XRP(10'000), owner, holder, destination); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const fee = env.current()->fees().base; + + auto withdraw = + vault.withdraw({.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = destination.human(); + env(withdraw, Fee(fee), Ter(tesSUCCESS)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore - fee); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); +} + +public: +void +run() override +{ + testVaultWithdrawEqualityEnforced(); + testBugIssuerVaultDepositAtEdge(); + testBugMakeDeltaPosteriorScale(); + testBugMakeDeltaAnteriorScale(); + testVaultDepositCanonicalizeToZero(); + testBugDepositShareTruncationSubUlp(); + testVaultWithdrawCanonicalizeToZero(); + testBugVaultDustDebitCanonicalizesToNoOp(); + testBugVaultDepositOvercreditsAcrossScaleBoundary(); + testBugVaultLockedByPartialWithdraw(); + testVaultDepositNegativeBalanceFromOppositeLimit(); + testCredentialPinsPseudoAccount(); + testCredentialPinOverflow(); + testBug6LimitBypassWithShares(); + testBugClawbackRoundTripOvershoot(); + testBugWithdrawRoundTripOvershoot(); + testBugClawbackAfterLoanImpair(); + testBugMptZeroWithdrawMissingHolding(); + testBugIouZeroWithdrawMissingTrustLine(); + testBugXrpZeroWithdrawSponsoredFee(); + testBugSelfWithdrawAfterIssuerClearsDefaultRipple(); + testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); + testBugSponsorAsDestinationFeeMisappliedToPayout(); + testPrefundedFeeWithdraw(); + testUnsponsoredWithdrawToDistinctDestinationPreAmendment(); +} }; BEAST_DEFINE_TESTSUITE(VaultBugs, app, xrpl);