fix: Check permissioned domain on private vault withdrawal

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

@@ -6,6 +6,7 @@
 #include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/ledger/helpers/VaultHelpers.h>
 #include <xrpl/protocol/AccountID.h>
@@ -68,6 +69,7 @@
     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,46 @@
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !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
+    // 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;
+
+        // 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)
     {
         // checkWithdrawFreeze checks the underlying asset on the source
@@ -211,7 +253,9 @@
     // 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, and preclaim checks such a withdrawal against the
+    // vault's permissioned domain.
 
     auto const amount = ctx_.tx[sfAmount];
     Asset const vaultAsset = vault->at(sfAsset);

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