From 52f61678781ff99610c3fe42ce1a174bfb09bd48 Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Fri, 7 Aug 2026 18:35:24 +0100 Subject: [PATCH 1/8] fix: Check permissioned domain on private vault withdrawal A private vault restricts who may take part in it, but VaultWithdraw only ever checked that the destination was allowed to hold the underlying asset. A participant could therefore withdraw to an account the domain owner never admitted, and the funds left the domain. Under fixCleanup3_4_0, a withdrawal from a private vault to a third party now requires both the submitter and the destination to be members of the vault's permissioned domain, read from the share issuance as VaultDeposit does. Withdrawing to self is not checked, so that losing vault access cannot strand funds already deposited. The asset issuer is always allowed to receive, which keeps the return path for frozen assets open even for a submitter who lost access. Public vaults, VaultClawback and LoanBrokerCoverWithdraw are unaffected. --- .../tx/transactors/vault/VaultWithdraw.cpp | 45 ++++- src/test/app/Vault_test.cpp | 162 ++++++++++++++++++ 2 files changed, 206 insertions(+), 1 deletion(-) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 353b72c30d1..cbc1690eaea 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -68,6 +69,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) auto const fix313Enabled = ctx.view.rules().enabled(fixCleanup3_1_3); 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 vault = ctx.view.read(keylet::vault(ctx.tx[sfVaultID])); if (!vault) @@ -163,6 +165,45 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter)) return ter; + // Post-fixCleanup3_4_0: 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 inside the vault's permissioned domain. + // VaultDeposit applies the same domain check on the way in. + // + // Two cases deliberately skip the check. Withdrawing to self is never + // restricted: losing vault access must not strand funds already deposited. + // The asset issuer is always allowed to receive, which keeps the return + // path for frozen assets open even for a submitter who lost access. + if (fix340Enabled && vault->isFlag(lsfVaultPrivate) && dstAcct != account && + dstAcct != vaultAsset.getIssuer()) + { + auto const sleIssuance = ctx.view.read(keylet::mptokenIssuance(vaultShare)); + if (!sleIssuance) + { + // LCOV_EXCL_START + JLOG(ctx.j.error()) << "VaultWithdraw: missing issuance of vault shares."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + // The domain is read from the share issuance rather than the vault, to + // stay consistent with VaultDeposit. A private vault with no domain + // set has no authorized participants to withdraw to. + auto const maybeDomainID = sleIssuance->at(~sfDomainID); + if (!maybeDomainID) + return tecNO_AUTH; + + for (auto const& subject : {account, dstAcct}) + { + // Unlike VaultDeposit we do not suppress tecEXPIRED: there is no + // doApply step here that would clean up the expired credential. + if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, subject); + !isTesSuccess(ter)) + return ter; + } + } + if (fix330Enabled) { // checkWithdrawFreeze checks the underlying asset on the source @@ -211,7 +252,9 @@ VaultWithdraw::doApply() // Note, we intentionally do not check lsfVaultPrivate flag on the Vault. If // you have a share in the vault, it means you were at some point authorized // to deposit into it, and this means you are also indefinitely authorized - // to withdraw from it. + // to withdraw it to yourself. Sending the proceeds to somebody else is a + // different matter: post-fixCleanup3_4_0 preclaim checks such a withdrawal + // against the vault's permissioned domain. auto const amount = ctx_.tx[sfAmount]; Asset const vaultAsset = vault->at(sfAsset); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 773ce289632..a88337f84aa 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -8016,6 +8016,165 @@ class Vault_test : public beast::unit_test::Suite env.enableFeature(fixCleanup3_3_0); } + // Withdrawing out of a private vault to a third party requires both the + // submitter and the destination to be members of the vault's permissioned + // domain. Withdrawal to self is exempt: revoking vault access must not + // trap already deposited funds. The asset issuer is exempt as a + // destination, so that frozen assets can always be returned. + void + testVaultWithdrawPrivateDestinationDomain(FeatureBitset features) + { + using namespace test::jtx; + + bool const withFix = features[fixCleanup3_4_0]; + testcase( + std::string{"VaultWithdraw private vault destination domain check"} + + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const depositor{"depositor"}; + Account const beneficiary{"beneficiary"}; + Account const outsider{"outsider"}; + Account const pdOwner{"pdOwner"}; + Account const credIssuer{"credIssuer"}; + std::string const credType = "credential"; + + Env env{*this, features}; + Vault const vault{env}; + + env.fund( + XRP(100'000), issuer, owner, depositor, beneficiary, outsider, pdOwner, credIssuer); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + // Everyone holds Layer 1 (asset) permission, so anything blocked below + // is blocked by the Layer 2 (vault) check alone. + for (auto const& account : {owner, depositor, beneficiary, outsider}) + { + env.trust(asset(1'000'000), account); + env(pay(issuer, account, asset(10'000))); + } + env.close(); + + auto const domainId = [&]() { + pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}}; + env(pdomain::setTx(pdOwner, credentials)); + env.close(); + return pdomain::getNewDomain(env.meta()); + }(); + + auto const joinDomain = [&](Account const& account) { + env(credentials::create(account, credIssuer, credType)); + env(credentials::accept(account, credIssuer, credType)); + env.close(); + }; + joinDomain(depositor); + joinDomain(beneficiary); + + auto [createTx, keylet] = + vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate}); + env(createTx); + env.close(); + + { + auto tx = vault.set({.owner = owner, .id = keylet.key}); + tx[sfDomainID] = to_string(domainId); + env(tx); + env.close(); + } + + env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)})); + env.close(); + + auto const withdrawTo = [&, keylet = keylet](Account const& destination) { + auto tx = + vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)}); + tx[sfDestination] = destination.human(); + return tx; + }; + + { + // Destination holds both layers of permission. + env(withdrawTo(beneficiary)); + env.close(); + } + + { + // Destination may hold the asset but was never let into the vault. + env(withdrawTo(outsider), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS))); + env.close(); + } + + { + // The asset issuer can always receive, to keep the recovery path + // for frozen assets open. + env(withdrawTo(issuer)); + env.close(); + } + + { + // The vault owner gets no special treatment as a destination: it + // is a third party like any other and needs domain membership. + env(withdrawTo(owner), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS))); + env.close(); + } + + { + // Withdrawal to self needs no Destination and stays unaffected. + env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)})); + env.close(); + } + + { + // Naming yourself as the Destination is still a withdrawal to self. + env(withdrawTo(depositor)); + env.close(); + } + + { + testcase( + std::string{"VaultWithdraw private vault submitter lost vault access"} + + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + env(credentials::deleteCred(credIssuer, depositor, credIssuer, credType)); + env.close(); + + // The exit of last resort: the submitter lost vault access but + // must still be able to redeem its own shares. + env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)})); + env.close(); + + // Moving funds to anyone else is not allowed any more, even to a + // destination that is itself a domain member. + env(withdrawTo(beneficiary), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS))); + env.close(); + + // Returning assets to the issuer stays open regardless. + env(withdrawTo(issuer)); + env.close(); + } + + { + testcase( + std::string{"VaultWithdraw public vault destination unaffected"} + + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + auto [publicTx, publicKeylet] = vault.create({.owner = owner, .asset = asset}); + env(publicTx); + env.close(); + + env(vault.deposit({.depositor = owner, .id = publicKeylet.key, .amount = asset(100)})); + env.close(); + + auto tx = + vault.withdraw({.depositor = owner, .id = publicKeylet.key, .amount = asset(1)}); + tx[sfDestination] = outsider.human(); + env(tx); + env.close(); + } + } + void testVaultWithdrawFreezeIOU() { @@ -8411,6 +8570,9 @@ class Vault_test : public beast::unit_test::Suite testVaultWithdrawFreezeMPT(); testVaultSelfWithdrawWhileFrozen(); + testVaultWithdrawPrivateDestinationDomain(all_ - fixCleanup3_4_0); + testVaultWithdrawPrivateDestinationDomain(all_); + testReferenceHolding(); testHoldingDeletionBlocked(); } From a123c510701f0adb86f42fe2e0267448e46746a2 Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Fri, 7 Aug 2026 18:49:56 +0100 Subject: [PATCH 2/8] review: Drop amendment names from comments and unroll the domain loop Comments now describe the rule rather than the gate that carries it: the rules.enabled condition already tells the reader which amendment applies, and naming it in prose only rots once the amendment activates. The two domain checks are also spelled out instead of looping over an initializer list of the two accounts. With a three-line body and exactly two elements the loop saved nothing and asked the reader to think about temporaries. --- .../tx/transactors/vault/VaultWithdraw.cpp | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index cbc1690eaea..31741335b40 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -165,11 +165,11 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter)) return ter; - // Post-fixCleanup3_4_0: 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 inside the vault's permissioned domain. - // VaultDeposit applies the same domain check on the way in. + // 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 + // inside the vault's permissioned domain. VaultDeposit applies the same + // domain check on the way in. // // Two cases deliberately skip the check. Withdrawing to self is never // restricted: losing vault access must not strand funds already deposited. @@ -194,14 +194,15 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (!maybeDomainID) return tecNO_AUTH; - for (auto const& subject : {account, dstAcct}) - { - // Unlike VaultDeposit we do not suppress tecEXPIRED: there is no - // doApply step here that would clean up the expired credential. - if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, subject); - !isTesSuccess(ter)) - return ter; - } + // Unlike VaultDeposit we do not suppress tecEXPIRED: there is no + // doApply step here that would clean up the expired credential. + if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, account); + !isTesSuccess(ter)) + return ter; + + if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, dstAcct); + !isTesSuccess(ter)) + return ter; } if (fix330Enabled) @@ -253,8 +254,8 @@ VaultWithdraw::doApply() // you have a share in the vault, it means you were at some point authorized // to deposit into it, and this means you are also indefinitely authorized // to withdraw it to yourself. Sending the proceeds to somebody else is a - // different matter: post-fixCleanup3_4_0 preclaim checks such a withdrawal - // against the vault's permissioned domain. + // different matter, and preclaim checks such a withdrawal against the + // vault's permissioned domain. auto const amount = ctx_.tx[sfAmount]; Asset const vaultAsset = vault->at(sfAsset); From 050dbc628f7b22759406896a05cb717ce5b1e1f1 Mon Sep 17 00:00:00 2001 From: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:41:30 +0000 Subject: [PATCH 3/8] refactor: Drop dead associateAsset calls from loan delete paths (#7986) Co-authored-by: Cursor --- src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp | 3 --- src/libxrpl/tx/transactors/lending/LoanDelete.cpp | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp index b36977d2256..433d77806a4 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include @@ -198,8 +197,6 @@ LoanBrokerDelete::doApply() view().erase(broker); - associateAsset(*broker, vaultAsset); - return tesSUCCESS; } diff --git a/src/libxrpl/tx/transactors/lending/LoanDelete.cpp b/src/libxrpl/tx/transactors/lending/LoanDelete.cpp index 1a77489b4bc..bc8e974d105 100644 --- a/src/libxrpl/tx/transactors/lending/LoanDelete.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanDelete.cpp @@ -130,9 +130,6 @@ LoanDelete::doApply() // Decrement the borrower's owner count decreaseOwnerCountForObject(view, borrowerSle, loanSle, 1, j_); - // These associations shouldn't do anything, but do them just to be safe - associateAsset(*loanSle, vaultAsset); - associateAsset(*brokerSle, vaultAsset); associateAsset(*vaultSle, vaultAsset); return tesSUCCESS; From d06a03baa6fce02614005226e5791ece9f2e9569 Mon Sep 17 00:00:00 2001 From: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:42:02 +0000 Subject: [PATCH 4/8] test: Verify private-vault DEX permissions survive domain loss (#7937) --- src/test/app/Vault_test.cpp | 188 ++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 70527f570dc..6b6c4eb875f 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3017,6 +3017,192 @@ class Vault_test : public beast::unit_test::Suite } } + void + testDomainLossAfterAcquisition() + { + using namespace test::jtx; + + testcase("private vault share transfer after depositor loses domain"); + + // The "Private Vault - Access Control Rules" spec requires that a holder who + // loses Layer 2 (Permissioned Domain membership) after acquiring shares be + // blocked from sending them onward, by P2P transfer or DEX offer, the same + // way a brand-new never-authorized holder is blocked. Only withdrawal to + // self is meant to stay open. + // + // For a domain-gated share MPToken, requireAuth()'s escape hatch for + // holders who already have an MPToken (MPTokenHelpers.cpp) only applies to + // the classic explicit-issuer-authorization flag, which + // enforceMPTokenAuthorization documents as "meaningless" for + // domain-authorized holders and never sets. So a stale MPToken does not + // carry authorization forward once the account's domain credential is + // gone, and both actions below are correctly blocked. + + Env env{*this, testableAmendments()}; + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const depositor{"depositor"}; + Account const bob{"bob"}; + Account const pdOwner{"pdOwner"}; + Account const credIssuer{"credIssuer"}; + std::string const credType = "credential"; + Vault const vault{env}; + env.fund(XRP(1000), issuer, owner, depositor, bob, pdOwner, credIssuer); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1000), owner); + env(pay(issuer, owner, asset(500))); + env.trust(asset(1000), depositor); + env(pay(issuer, depositor, asset(500))); + env.trust(asset(1000), bob); + env(pay(issuer, bob, asset(500))); + env.close(); + + // Transferable shares (no tfVaultShareNonTransferable): sections 3.3/3.4 of + // the spec (DEX trading / P2P transfer) only apply to transferable shares. + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate}); + env(tx); + env.close(); + + pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}}; + env(pdomain::setTx(pdOwner, credentials)); + auto const domainId = [&]() { + auto tx = env.tx()->getJson(JsonOptions::Values::None); + return pdomain::getNewDomain(env.meta()); + }(); + { + auto domainTx = vault.set({.owner = owner, .id = keylet.key}); + domainTx[sfDomainID] = to_string(domainId); + env(domainTx); + env.close(); + } + + // Both depositor and bob acquire domain membership and deposit, so each + // ends up with an authorized share MPToken. + env(credentials::create(depositor, credIssuer, credType)); + env(credentials::accept(depositor, credIssuer, credType)); + env(credentials::create(bob, credIssuer, credType)); + env(credentials::accept(bob, credIssuer, credType)); + env.close(); + + env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(100)})); + env(vault.deposit({.depositor = bob, .id = keylet.key, .amount = asset(100)})); + env.close(); + + auto const shares = [&env, keylet = keylet, this]() -> PrettyAsset { + auto const sle = env.le(keylet); + BEAST_EXPECT(sle != nullptr); + return MPTIssue(sle->at(sfShareMPTID)); + }(); + + // Depositor loses Layer 2: their Permissioned Domain credential is revoked. + auto const credKeylet = credentials::keylet(depositor, credIssuer, credType); + env(credentials::deleteCred(credIssuer, depositor, credIssuer, credType)); + env.close(); + BEAST_EXPECT(env.le(credKeylet) == nullptr); + + // Sanity check, mirrors testWithDomainCheck's "not authorized yet" case: a + // brand-new depositor with no MPToken yet is still correctly blocked. The + // gap below is specific to holders who already hold shares. + { + Account const charlie{"charlie"}; + env.fund(XRP(1000), charlie); + env.close(); + auto depTx = + vault.deposit({.depositor = charlie, .id = keylet.key, .amount = asset(1)}); + env(depTx, Ter{tecNO_AUTH}); + } + + // P2P transfer: spec section 3.4 requires this blocked once Layer 2 is + // lost, and it is. + env(pay(depositor, bob, shares(1)), Ter{tecNO_AUTH}); + env.close(); + + // DEX/CLOB: spec section 3.3 requires the seller leg blocked the same way. + // The offer can't even be created: preclaim treats the seller as + // unfunded once their share balance reads as zero for auth purposes. + env(offer(depositor, XRP(1), shares(1)), Ter{tecUNFUNDED_OFFER}); + env.close(); + BEAST_EXPECT(expectOffers(env, depositor, 0)); + } + + void + testDomainCheckBuyerSideOffer() + { + using namespace test::jtx; + + testcase("private vault share purchase via DEX requires buyer domain membership"); + + // The "Private Vault - Access Control Rules" spec requires the buyer leg + // of a DEX trade in private-vault shares to hold Layer 1 and Layer 2 as + // well, not just the seller. + + Env env{*this, testableAmendments()}; + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const bob{"bob"}; + Account const charlie{"charlie"}; + Account const pdOwner{"pdOwner"}; + Account const credIssuer{"credIssuer"}; + std::string const credType = "credential"; + Vault const vault{env}; + env.fund(XRP(1000), issuer, owner, bob, charlie, pdOwner, credIssuer); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1000), owner); + env(pay(issuer, owner, asset(500))); + env.trust(asset(1000), bob); + env(pay(issuer, bob, asset(500))); + env.close(); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate}); + env(tx); + env.close(); + + pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}}; + env(pdomain::setTx(pdOwner, credentials)); + auto const domainId = [&]() { + auto tx = env.tx()->getJson(JsonOptions::Values::None); + return pdomain::getNewDomain(env.meta()); + }(); + { + auto domainTx = vault.set({.owner = owner, .id = keylet.key}); + domainTx[sfDomainID] = to_string(domainId); + env(domainTx); + env.close(); + } + + // Only bob joins the domain and deposits; charlie never does. + env(credentials::create(bob, credIssuer, credType)); + env(credentials::accept(bob, credIssuer, credType)); + env.close(); + env(vault.deposit({.depositor = bob, .id = keylet.key, .amount = asset(100)})); + env.close(); + + auto const shares = [&env, keylet = keylet, this]() -> PrettyAsset { + auto const sle = env.le(keylet); + BEAST_EXPECT(sle != nullptr); + return MPTIssue(sle->at(sfShareMPTID)); + }(); + + // Bob (domain member, holds shares) rests a sell offer. + env(offer(bob, XRP(1), shares(1))); + env.close(); + BEAST_EXPECT(expectOffers(env, bob, 1)); + + // Charlie never held the domain credential. Buying shares via a + // crossing offer must be blocked the same way a direct MPTokenAuthorize + // + pay attempt already is (see testWithDomainChecXRP's "cannot pay + // shares to 3rd party"): checkAcceptAsset() rejects the offer outright + // in preclaim, before any funding check is even reached. + env(offer(charlie, shares(1), XRP(1)), Ter{tecNO_AUTH}); + env.close(); + BEAST_EXPECT(expectOffers(env, bob, 1)); + BEAST_EXPECT(expectOffers(env, charlie, 0)); + } + void testWithDomainChecXRP() { @@ -8396,6 +8582,8 @@ class Vault_test : public beast::unit_test::Suite testWithMPT(); testWithIOU(); testWithDomainCheck(); + testDomainLossAfterAcquisition(); + testDomainCheckBuyerSideOffer(); testWithDomainChecXRP(); testNonTransferableShares(); testFailedPseudoAccount(); From dc8973053eeb2bfc1aac2ed61878d18690dab028 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:42:55 +0000 Subject: [PATCH 5/8] test: Fix LoanBatch broker cover rates and schedule overflow (#7967) --- src/test/app/lending/LoanMisc_test.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/app/lending/LoanMisc_test.cpp b/src/test/app/lending/LoanMisc_test.cpp index 2cb4f38ecf3..c5a7d543110 100644 --- a/src/test/app/lending/LoanMisc_test.cpp +++ b/src/test/app/lending/LoanMisc_test.cpp @@ -473,14 +473,21 @@ class LoanBatch_test : public LoanTestBase TenthBips16 const managementFeeRate{managementFeeRateDist_(engine_)}; auto const serviceFee = serviceFeeDist_(engine_); TenthBips32 interest{interestRateDist_(engine_)}; - auto const payTotal = paymentTotalDist_(engine_); + auto payTotal = paymentTotalDist_(engine_); auto const payInterval = paymentIntervalDist_(engine_); + // The end of the last payment's grace period must fit in a 32-bit + // ripple-epoch timestamp, or LoanSet fails with tecKILLED. Cap the + // schedule well below that horizon (2e9 seconds is roughly 63 years, + // leaving ample headroom over the ledger start date). + constexpr std::uint32_t kMaxScheduleSeconds = 2'000'000'000; + payTotal = std::min(payTotal, static_cast(kMaxScheduleSeconds / payInterval)); BrokerParameters const brokerParams{ .vaultDeposit = principalRequest * 10, .debtMax = 0, .coverRateMin = TenthBips32{0}, - .managementFeeRate = managementFeeRate}; + .managementFeeRate = managementFeeRate, + .coverRateLiquidation = TenthBips32{0}}; LoanParameters const loanParams{ .account = lender, .counter = borrower, From cfc9164b373b1c99becc93f6d882e7c649ffb45b Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Wed, 12 Aug 2026 15:05:59 +0100 Subject: [PATCH 6/8] review: Extract the vault permissioned-domain check into a helper VaultWithdraw repeated VaultDeposit's domain lookup almost line for line. Both now call checkVaultDomain, which reads the domain from the share issuance and reports a missing domain as tecNO_AUTH. The callers disagree only about expired credentials, so that stays a parameter: deposit tolerates them because doApply deletes them, withdrawal keeps the error because nothing in its path would clean them up. This also covers the missing-domain branch, which no test reached before. A private vault whose domain has been cleared refuses a third-party destination, while withdrawal to self and to the issuer keep working. Co-authored-by: Cursor --- include/xrpl/ledger/helpers/VaultHelpers.h | 37 +++++++++++++++++++ src/libxrpl/ledger/helpers/VaultHelpers.cpp | 24 ++++++++++++ .../tx/transactors/vault/VaultDeposit.cpp | 24 +++--------- .../tx/transactors/vault/VaultWithdraw.cpp | 12 +----- src/test/app/Vault_test.cpp | 23 ++++++++++++ 5 files changed, 91 insertions(+), 29 deletions(-) diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index 5681cc57e86..208625662f9 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -5,6 +5,7 @@ #include #include #include +#include #include @@ -123,4 +124,40 @@ isSoleShareholder(ReadView const& view, AccountID const& account, SLE::const_ref [[nodiscard]] VaultVersion getVaultVersion(SLE::const_ref vault); +/** + * Controls whether checkVaultDomain reports an expired credential as an + * error. A caller that deletes expired credentials later, in doApply, passes + * Yes and treats the subject as authorized; a caller with no such cleanup + * step must keep the error. + */ +enum class SuppressExpired : bool { No = false, Yes = true }; + +/** + * Checks that subject belongs to the permissioned domain governing a vault's + * shares. + * + * The domain is read from the share issuance rather than from the vault. Vault + * shares are issued by the vault's pseudo-account, which cannot grant an + * authorization explicitly, so domain membership is the only route to being + * authorized: a vault with no domain set has no authorized participants at + * all, and every subject fails with tecNO_AUTH. + * + * Which accounts to check, and whether to check at all, is left to the caller. + * This says nothing about vault privacy or about the roles of the accounts. + * + * @param view The ledger view. + * @param issuance The MPTokenIssuance SLE for the vault's shares. + * @param subject The account whose domain membership is checked. + * @param suppressExpired Whether an expired credential counts as authorized. + * + * @return tesSUCCESS if the subject is a domain member, otherwise the reason + * it is not. + */ +[[nodiscard]] TER +checkVaultDomain( + ReadView const& view, + SLE::const_ref issuance, + AccountID const& subject, + SuppressExpired suppressExpired); + } // namespace xrpl diff --git a/src/libxrpl/ledger/helpers/VaultHelpers.cpp b/src/libxrpl/ledger/helpers/VaultHelpers.cpp index 78f64d20774..a5494dc1408 100644 --- a/src/libxrpl/ledger/helpers/VaultHelpers.cpp +++ b/src/libxrpl/ledger/helpers/VaultHelpers.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include // IWYU pragma: keep @@ -11,6 +12,7 @@ #include #include #include // IWYU pragma: keep +#include #include #include @@ -157,4 +159,26 @@ getVaultVersion(SLE::const_ref vault) return static_cast(version); } +[[nodiscard]] TER +checkVaultDomain( + ReadView const& view, + SLE::const_ref issuance, + AccountID const& subject, + SuppressExpired suppressExpired) +{ + XRPL_ASSERT( + issuance && issuance->getType() == ltMPTOKEN_ISSUANCE, + "xrpl::checkVaultDomain : valid issuance SLE"); + + auto const maybeDomainID = issuance->at(~sfDomainID); + if (!maybeDomainID) + return tecNO_AUTH; + + auto const err = credentials::validDomain(view, *maybeDomainID, subject); + if (err == tecEXPIRED && suppressExpired == SuppressExpired::Yes) + return tesSUCCESS; + + return err; +} + } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp index aa9cfc8537b..db180800315 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -127,26 +126,13 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) return tecLOCKED; } + // The vault owner is authorized to deposit unconditionally. An expired + // credential is tolerated here because doApply deletes it. if (vault->isFlag(lsfVaultPrivate) && account != vault->at(sfOwner)) { - auto const maybeDomainID = sleIssuance->at(~sfDomainID); - // Since this is a private vault and the account is not its owner, we - // perform authorization check based on DomainID read from sleIssuance. - // Had the vault shares been a regular MPToken, we would allow - // authorization granted by the Issuer explicitly, but Vault uses Issuer - // pseudo-account, which cannot grant an authorization. - if (maybeDomainID) - { - // As per validDomain documentation, we suppress tecEXPIRED error - // here, so we can delete any expired credentials inside doApply. - if (auto const err = credentials::validDomain(ctx.view, *maybeDomainID, account); - !isTesSuccess(err) && err != tecEXPIRED) - return err; - } - else - { - return tecNO_AUTH; - } + if (auto const err = checkVaultDomain(ctx.view, sleIssuance, account, SuppressExpired::Yes); + !isTesSuccess(err)) + return err; } // Source MPToken must exist (if asset is an MPT) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 31741335b40..c7cabcc0a4f 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -187,20 +186,13 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - // The domain is read from the share issuance rather than the vault, to - // stay consistent with VaultDeposit. A private vault with no domain - // set has no authorized participants to withdraw to. - auto const maybeDomainID = sleIssuance->at(~sfDomainID); - if (!maybeDomainID) - return tecNO_AUTH; - // Unlike VaultDeposit we do not suppress tecEXPIRED: there is no // doApply step here that would clean up the expired credential. - if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, account); + if (auto const ter = checkVaultDomain(ctx.view, sleIssuance, account, SuppressExpired::No); !isTesSuccess(ter)) return ter; - if (auto const ter = credentials::validDomain(ctx.view, *maybeDomainID, dstAcct); + if (auto const ter = checkVaultDomain(ctx.view, sleIssuance, dstAcct, SuppressExpired::No); !isTesSuccess(ter)) return ter; } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index dd12e11e535..bccf7baf38f 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -8170,6 +8170,29 @@ class Vault_test : public beast::unit_test::Suite env.close(); } + { + testcase( + std::string{"VaultWithdraw private vault with no domain set"} + + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + auto tx = vault.set({.owner = owner, .id = keylet.key}); + tx[sfDomainID] = "0"; + env(tx); + env.close(); + + // Clearing the domain leaves the vault with nobody it considers + // authorized, so a third-party destination cannot qualify. + env(withdrawTo(beneficiary), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS))); + env.close(); + + // The two exempt paths survive the domain going away. + env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)})); + env.close(); + + env(withdrawTo(issuer)); + env.close(); + } + { testcase( std::string{"VaultWithdraw public vault destination unaffected"} + From f63cc0fc0d070d9af0462793161949dd1245b26e Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Wed, 12 Aug 2026 15:06:45 +0100 Subject: [PATCH 7/8] fix: Reject pseudo-account destinations on vault withdrawal A pseudo-account is owned by a ledger object and cannot take part in a user-initiated payout, but VaultWithdraw never checked for one. The withdrawal was already refused, because every pseudo-account is created with deposit authorization set, and the resulting tecNO_PERMISSION named the wrong reason. It now returns tecPSEUDO_ACCOUNT, which is what LoanBrokerCoverWithdraw has always done for the same case. The check sits ahead of the domain check so that a private vault gives the same answer as a public one. Otherwise a pseudo-account destination would be reported as lacking domain membership that it can never hold. Co-authored-by: Cursor --- .../tx/transactors/vault/VaultWithdraw.cpp | 12 ++ src/test/app/Vault_test.cpp | 110 ++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index c7cabcc0a4f..9a08485cc0c 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -104,6 +105,17 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } + // A pseudo-account belongs to a ledger object rather than to a person and + // must never receive funds from a user-initiated transaction. Deposit + // authorization, which every pseudo-account carries, already refuses the + // payout, but it reports only that the destination declines deposits and + // leaves the real reason unsaid. + if (fix340Enabled && isPseudoAccount(ctx.view, dstAcct)) + { + JLOG(ctx.j.debug()) << "VaultWithdraw: cannot withdraw into a pseudo-account."; + return tecPSEUDO_ACCOUNT; + } + if (fix313Enabled && amount.asset() == vaultShare) { // Post-fixCleanup3_1_3: if the user specified shares, convert diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index bccf7baf38f..f6a117ff9c1 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -8213,6 +8213,113 @@ class Vault_test : public beast::unit_test::Suite } } + // A pseudo-account belongs to a ledger object, so it must never be the + // destination of a withdrawal. The payout is refused either way, by the + // deposit authorization every pseudo-account carries, so the only change + // is a misleading tecNO_PERMISSION becoming tecPSEUDO_ACCOUNT. The check + // runs ahead of the private-vault domain check, which would otherwise + // report a domain problem against an account that can never join one. + void + testVaultWithdrawPseudoAccountDestination(FeatureBitset features) + { + using namespace test::jtx; + + bool const withFix = features[fixCleanup3_4_0]; + testcase( + std::string{"VaultWithdraw pseudo-account destination"} + + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const depositor{"depositor"}; + Account const pdOwner{"pdOwner"}; + Account const credIssuer{"credIssuer"}; + std::string const credType = "credential"; + + Env env{*this, features}; + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, owner, depositor, pdOwner, credIssuer); + // Rippling plays no part in what is being tested here, and would + // otherwise stop the payout before it reaches the check under test. + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + for (auto const& account : {owner, depositor}) + { + env.trust(asset(1'000'000), account); + env(pay(issuer, account, asset(10'000))); + } + env.close(); + + // Another vault over the same asset supplies the destination. Its + // pseudo-account holds a trust line for the asset from creation, so + // the payout is refused for being a pseudo-account and nothing else. + auto const pseudoDestination = [&]() { + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + return Account("otherVault", env.le(keylet)->at(sfAccount)); + }(); + + TER const expected = withFix ? TER(tecPSEUDO_ACCOUNT) : TER(tecNO_PERMISSION); + + auto const withdrawToPseudo = [&](uint256 const& vaultId) { + auto tx = vault.withdraw({.depositor = depositor, .id = vaultId, .amount = asset(1)}); + tx[sfDestination] = pseudoDestination.human(); + return tx; + }; + + { + auto [createTx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(createTx); + env.close(); + + env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)})); + env.close(); + + env(withdrawToPseudo(keylet.key), Ter(expected)); + env.close(); + + // Withdrawing to self out of the same vault stays unaffected. + env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)})); + env.close(); + } + + { + auto const domainId = [&]() { + pdomain::Credentials const credentials{ + {.issuer = credIssuer, .credType = credType}}; + env(pdomain::setTx(pdOwner, credentials)); + env.close(); + return pdomain::getNewDomain(env.meta()); + }(); + + env(credentials::create(depositor, credIssuer, credType)); + env(credentials::accept(depositor, credIssuer, credType)); + env.close(); + + auto [createTx, keylet] = + vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate}); + env(createTx); + env.close(); + + auto setTx = vault.set({.owner = owner, .id = keylet.key}); + setTx[sfDomainID] = to_string(domainId); + env(setTx); + env.close(); + + env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)})); + env.close(); + + // The domain check never gets a say: the destination is rejected + // for what it is, not for the domain it is missing. + env(withdrawToPseudo(keylet.key), Ter(expected)); + env.close(); + } + } + void testVaultWithdrawFreezeIOU() { @@ -8611,6 +8718,9 @@ class Vault_test : public beast::unit_test::Suite testVaultWithdrawPrivateDestinationDomain(all_ - fixCleanup3_4_0); testVaultWithdrawPrivateDestinationDomain(all_); + testVaultWithdrawPseudoAccountDestination(all_ - fixCleanup3_4_0); + testVaultWithdrawPseudoAccountDestination(all_); + testReferenceHolding(); testHoldingDeletionBlocked(); } From a6fc84a59d447859d02831a5364234654614728f Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Wed, 12 Aug 2026 15:33:54 +0100 Subject: [PATCH 8/8] review: Isolate the cause in the missing-domain withdrawal test The case cleared the vault's domain while the submitter was still without a credential, left over from the preceding case, so tecNO_AUTH could have come from either the submitter or the absent domain. The submitter now regains its credential first, which leaves the missing domain as the only thing that can refuse the payout. --- src/test/app/Vault_test.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index f6a117ff9c1..c9c70085198 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -8175,13 +8175,20 @@ class Vault_test : public beast::unit_test::Suite std::string{"VaultWithdraw private vault with no domain set"} + (withFix ? " (fixCleanup3_4_0)" : " (pre-fix)")); + // Give the submitter its vault access back first, so that the + // vault having no domain is the only reason left to refuse. + env(credentials::create(depositor, credIssuer, credType)); + env(credentials::accept(depositor, credIssuer, credType)); + env.close(); + auto tx = vault.set({.owner = owner, .id = keylet.key}); tx[sfDomainID] = "0"; env(tx); env.close(); // Clearing the domain leaves the vault with nobody it considers - // authorized, so a third-party destination cannot qualify. + // authorized, so a third-party destination cannot qualify even + // though both ends of the payout hold a credential. env(withdrawTo(beneficiary), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS))); env.close();