VaultWithdraw destination account bugfix (#5572)

#5224 added (among other things) a `VaultWithdraw` transaction that allows setting the recipient of the withdrawn funds in the `Destination` transaction field. This technically turns this transaction into a payment, and in some respect the implementation does follow payment rules, e.g. enforcement of `lsfRequireDestTag` or `lsfDepositAuth`, or that MPT transfer has destination `MPToken`. However for IOUs, it missed verification that the destination account has a trust line to the asset issuer. Since the default behavior of `accountSendIOU` is to create this trust line (if missing), this is what `VaultWithdraw` currently does. This is incorrect, since the `Destination` might not be interested in holding the asset in question; this basically enables spammy transfers. This change, therefore, removes automatic creation of a trust line to the `Destination` account in `VaultWithdraw`.

include/xrpl/protocol/detail/transactions.macro

@@ -509,6 +509,7 @@ TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw, Delegation::delegatable, ({
     {sfVaultID, soeREQUIRED},
     {sfAmount, soeREQUIRED, soeMPTSupported},
     {sfDestination, soeOPTIONAL},
+    {sfDestinationTag, soeOPTIONAL},
 }))
 
 /** This transaction claws back tokens from a vault. */

src/test/app/Vault_test.cpp

@@ -234,6 +234,28 @@ class Vault_test : public beast::unit_test::suite
                 env(tx, ter{tecNO_PERMISSION});
             }
 
+            {
+                testcase(prefix + " fail to withdraw to zero destination");
+                auto tx = vault.withdraw(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(1000)});
+                tx[sfDestination] = "0";
+                env(tx, ter(temMALFORMED));
+            }
+
+            {
+                testcase(
+                    prefix +
+                    " fail to withdraw with tag but without destination");
+                auto tx = vault.withdraw(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(1000)});
+                tx[sfDestinationTag] = "0";
+                env(tx, ter(temMALFORMED));
+            }
+
             if (!asset.raw().native())
             {
                 testcase(
@@ -1335,6 +1357,7 @@ class Vault_test : public beast::unit_test::suite
         struct CaseArgs
         {
             bool enableClawback = true;
+            bool requireAuth = true;
         };
 
         auto testCase = [this](
@@ -1356,16 +1379,20 @@ class Vault_test : public beast::unit_test::suite
             Vault vault{env};
 
             MPTTester mptt{env, issuer, mptInitNoFund};
+            auto const none = LedgerSpecificFlags(0);
             mptt.create(
                 {.flags = tfMPTCanTransfer | tfMPTCanLock |
-                     (args.enableClawback ? lsfMPTCanClawback
-                                          : LedgerSpecificFlags(0)) |
-                     tfMPTRequireAuth});
+                     (args.enableClawback ? tfMPTCanClawback : none) |
+                     (args.requireAuth ? tfMPTRequireAuth : none)});
             PrettyAsset asset = mptt.issuanceID();
             mptt.authorize({.account = owner});
-            mptt.authorize({.account = issuer, .holder = owner});
             mptt.authorize({.account = depositor});
-            mptt.authorize({.account = issuer, .holder = depositor});
+            if (args.requireAuth)
+            {
+                mptt.authorize({.account = issuer, .holder = owner});
+                mptt.authorize({.account = issuer, .holder = depositor});
+            }
+
             env(pay(issuer, depositor, asset(1000)));
             env.close();
 
@@ -1514,6 +1541,100 @@ class Vault_test : public beast::unit_test::suite
             }
         });
 
+        testCase(
+            [this](
+                Env& env,
+                Account const& issuer,
+                Account const& owner,
+                Account const& depositor,
+                PrettyAsset const& asset,
+                Vault& vault,
+                MPTTester& mptt) {
+                testcase(
+                    "MPT 3rd party without MPToken cannot be withdrawal "
+                    "destination");
+
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                env(tx);
+                env.close();
+
+                tx = vault.deposit(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(100)});
+                env(tx);
+                env.close();
+
+                {
+                    // Set destination to 3rd party without MPToken
+                    Account charlie{"charlie"};
+                    env.fund(XRP(1000), charlie);
+                    env.close();
+
+                    tx = vault.withdraw(
+                        {.depositor = depositor,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    tx[sfDestination] = charlie.human();
+                    env(tx, ter(tecNO_AUTH));
+                }
+            },
+            {.requireAuth = false});
+
+        testCase(
+            [this](
+                Env& env,
+                Account const& issuer,
+                Account const& owner,
+                Account const& depositor,
+                PrettyAsset const& asset,
+                Vault& vault,
+                MPTTester& mptt) {
+                testcase("MPT depositor without MPToken cannot withdraw");
+
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                env(tx);
+                env.close();
+
+                tx = vault.deposit(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(1000)});
+                env(tx);
+                env.close();
+
+                {
+                    // Remove depositor's MPToken and withdraw will fail
+                    mptt.authorize(
+                        {.account = depositor, .flags = tfMPTUnauthorize});
+                    env.close();
+                    auto const mptoken =
+                        env.le(keylet::mptoken(mptt.issuanceID(), depositor));
+                    BEAST_EXPECT(mptoken == nullptr);
+
+                    tx = vault.withdraw(
+                        {.depositor = depositor,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx, ter(tecNO_AUTH));
+                }
+
+                {
+                    // Restore depositor's MPToken and withdraw will succeed
+                    mptt.authorize({.account = depositor});
+                    env.close();
+
+                    tx = vault.withdraw(
+                        {.depositor = depositor,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx);
+                }
+            },
+            {.requireAuth = false});
+
         testCase([this](
                      Env& env,
                      Account const& issuer,
@@ -1803,6 +1924,7 @@ class Vault_test : public beast::unit_test::suite
 
                 PrettyAsset const asset = issuer["IOU"];
                 env.trust(asset(1000), owner);
+                env.trust(asset(1000), charlie);
                 env(pay(issuer, owner, asset(200)));
                 env(rate(issuer, 1.25));
                 env.close();
@@ -2118,6 +2240,79 @@ class Vault_test : public beast::unit_test::suite
             env.close();
         });
 
+        testCase([&, this](
+                     Env& env,
+                     Account const& owner,
+                     Account const& issuer,
+                     Account const& charlie,
+                     auto,
+                     Vault& vault,
+                     PrettyAsset const& asset,
+                     auto&&...) {
+            testcase("IOU no trust line to 3rd party");
+
+            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+            env(tx);
+            env.close();
+
+            env(vault.deposit(
+                {.depositor = owner, .id = keylet.key, .amount = asset(100)}));
+            env.close();
+
+            Account const erin{"erin"};
+            env.fund(XRP(1000), erin);
+            env.close();
+
+            // Withdraw to 3rd party without trust line
+            auto const tx1 = [&](ripple::Keylet keylet) {
+                auto tx = vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(10)});
+                tx[sfDestination] = erin.human();
+                return tx;
+            }(keylet);
+            env(tx1, ter{tecNO_LINE});
+        });
+
+        testCase([&, this](
+                     Env& env,
+                     Account const& owner,
+                     Account const& issuer,
+                     Account const& charlie,
+                     auto,
+                     Vault& vault,
+                     PrettyAsset const& asset,
+                     auto&&...) {
+            testcase("IOU no trust line to depositor");
+
+            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+            env(tx);
+            env.close();
+
+            // reset limit, so deposit of all funds will delete the trust line
+            env.trust(asset(0), owner);
+            env.close();
+
+            env(vault.deposit(
+                {.depositor = owner, .id = keylet.key, .amount = asset(200)}));
+            env.close();
+
+            auto trustline =
+                env.le(keylet::line(owner, asset.raw().get<Issue>()));
+            BEAST_EXPECT(trustline == nullptr);
+
+            // Withdraw without trust line, will succeed
+            auto const tx1 = [&](ripple::Keylet keylet) {
+                auto tx = vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(10)});
+                return tx;
+            }(keylet);
+            env(tx1);
+        });
+
         testCase([&, this](
                      Env& env,
                      Account const& owner,

src/xrpld/app/tx/detail/Escrow.cpp

@@ -315,14 +315,14 @@ escrowCreatePreclaimHelper<MPTIssue>(
     // authorized
     auto const& mptIssue = amount.get<MPTIssue>();
     if (auto const ter =
-            requireAuth(ctx.view, mptIssue, account, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, account, AuthType::WeakAuth);
         ter != tesSUCCESS)
         return ter;
 
     // If the issuer has requireAuth set, check if the destination is
     // authorized
     if (auto const ter =
-            requireAuth(ctx.view, mptIssue, dest, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, dest, AuthType::WeakAuth);
         ter != tesSUCCESS)
         return ter;
 
@@ -746,7 +746,7 @@ escrowFinishPreclaimHelper<MPTIssue>(
     // authorized
     auto const& mptIssue = amount.get<MPTIssue>();
     if (auto const ter =
-            requireAuth(ctx.view, mptIssue, dest, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, dest, AuthType::WeakAuth);
         ter != tesSUCCESS)
         return ter;
 
@@ -1259,7 +1259,7 @@ escrowCancelPreclaimHelper<MPTIssue>(
     // authorized
     auto const& mptIssue = amount.get<MPTIssue>();
     if (auto const ter =
-            requireAuth(ctx.view, mptIssue, account, MPTAuthType::WeakAuth);
+            requireAuth(ctx.view, mptIssue, account, AuthType::WeakAuth);
         ter != tesSUCCESS)
         return ter;
 

src/xrpld/app/tx/detail/VaultWithdraw.cpp

@@ -52,9 +52,19 @@ VaultWithdraw::preflight(PreflightContext const& ctx)
         return temBAD_AMOUNT;
 
     if (auto const destination = ctx.tx[~sfDestination];
-        destination && *destination == beast::zero)
+        destination.has_value())
     {
-        JLOG(ctx.j.debug()) << "VaultWithdraw: zero/empty destination account.";
+        if (*destination == beast::zero)
+        {
+            JLOG(ctx.j.debug())
+                << "VaultWithdraw: zero/empty destination account.";
+            return temMALFORMED;
+        }
+    }
+    else if (ctx.tx.isFieldPresent(sfDestinationTag))
+    {
+        JLOG(ctx.j.debug()) << "VaultWithdraw: sfDestinationTag is set but "
+                               "sfDestination is not";
         return temMALFORMED;
     }
 
@@ -123,33 +133,39 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
 
     // Withdrawal to a 3rd party destination account is essentially a transfer,
     // via shares in the vault. Enforce all the usual asset transfer checks.
+    AuthType authType = AuthType::Legacy;
     if (account != dstAcct)
     {
         auto const sleDst = ctx.view.read(keylet::account(dstAcct));
         if (sleDst == nullptr)
             return tecNO_DST;
 
-        if (sleDst->getFlags() & lsfRequireDestTag)
+        if (sleDst->isFlag(lsfRequireDestTag) &&
+            !ctx.tx.isFieldPresent(sfDestinationTag))
             return tecDST_TAG_NEEDED;  // Cannot send without a tag
 
-        if (sleDst->getFlags() & lsfDepositAuth)
+        if (sleDst->isFlag(lsfDepositAuth))
         {
             if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
                 return tecNO_PERMISSION;
         }
+        // The destination account must have consented to receive the asset by
+        // creating a RippleState or MPToken
+        authType = AuthType::StrongAuth;
     }
 
-    // Destination MPToken must exist (if asset is an MPT)
-    if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct);
+    // Destination MPToken (for an MPT) or trust line (for an IOU) must exist
+    // if not sending to Account.
+    if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType);
         !isTesSuccess(ter))
         return ter;
 
     // Cannot withdraw from a Vault an Asset frozen for the destination account
-    if (isFrozen(ctx.view, dstAcct, vaultAsset))
-        return vaultAsset.holds<Issue>() ? tecFROZEN : tecLOCKED;
+    if (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset))
+        return ret;
 
-    if (isFrozen(ctx.view, account, vaultShare))
-        return tecLOCKED;
+    if (auto const ret = checkFrozen(ctx.view, account, vaultShare))
+        return ret;
 
     return tesSUCCESS;
 }

src/xrpld/ledger/View.h

@@ -175,6 +175,29 @@ isFrozen(
         asset.value());
 }
 
+[[nodiscard]] inline TER
+checkFrozen(ReadView const& view, AccountID const& account, Issue const& issue)
+{
+    return isFrozen(view, account, issue) ? (TER)tecFROZEN : (TER)tesSUCCESS;
+}
+
+[[nodiscard]] inline TER
+checkFrozen(
+    ReadView const& view,
+    AccountID const& account,
+    MPTIssue const& mptIssue)
+{
+    return isFrozen(view, account, mptIssue) ? (TER)tecLOCKED : (TER)tesSUCCESS;
+}
+
+[[nodiscard]] inline TER
+checkFrozen(ReadView const& view, AccountID const& account, Asset const& asset)
+{
+    return std::visit(
+        [&](auto const& issue) { return checkFrozen(view, account, issue); },
+        asset.value());
+}
+
 [[nodiscard]] bool
 isAnyFrozen(
     ReadView const& view,
@@ -725,19 +748,40 @@ transferXRP(
     STAmount const& amount,
     beast::Journal j);
 
-/* Check if MPToken exists:
- * - StrongAuth - before checking lsfMPTRequireAuth is set
- * - WeakAuth - after checking if lsfMPTRequireAuth is set
+/* Check if MPToken (for MPT) or trust line (for IOU) exists:
+ * - StrongAuth - before checking if authorization is required
+ * - WeakAuth
+ *    for MPT - after checking lsfMPTRequireAuth flag
+ *    for IOU - do not check if trust line exists
+ * - Legacy
+ *    for MPT - before checking lsfMPTRequireAuth flag i.e. same as StrongAuth
+ *    for IOU - do not check if trust line exists i.e. same as WeakAuth
  */
-enum class MPTAuthType : bool { StrongAuth = true, WeakAuth = false };
+enum class AuthType { StrongAuth, WeakAuth, Legacy };
 
 /** Check if the account lacks required authorization.
  *
- *   Return tecNO_AUTH or tecNO_LINE if it does
- *   and tesSUCCESS otherwise.
+ * Return tecNO_AUTH or tecNO_LINE if it does
+ * and tesSUCCESS otherwise.
+ *
+ * If StrongAuth then return tecNO_LINE if the RippleState doesn't exist. Return
+ * tecNO_AUTH if lsfRequireAuth is set on the issuer's AccountRoot, and the
+ * RippleState does exist, and the RippleState is not authorized.
+ *
+ * If WeakAuth then return tecNO_AUTH if lsfRequireAuth is set, and the
+ * RippleState exists, and is not authorized. Return tecNO_LINE if
+ * lsfRequireAuth is set and the RippleState doesn't exist. Consequently, if
+ * WeakAuth and lsfRequireAuth is *not* set, this function will return
+ * tesSUCCESS even if RippleState does *not* exist.
+ *
+ * The default "Legacy" auth type is equivalent to WeakAuth.
  */
 [[nodiscard]] TER
-requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
+requireAuth(
+    ReadView const& view,
+    Issue const& issue,
+    AccountID const& account,
+    AuthType authType = AuthType::Legacy);
 
 /** Check if the account lacks required authorization.
  *
@@ -751,32 +795,33 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
  * purely defensive, as we currently do not allow such vaults to be created.
  *
  * If StrongAuth then return tecNO_AUTH if MPToken doesn't exist or
- * lsfMPTRequireAuth is set and MPToken is not authorized. If WeakAuth then
- * return tecNO_AUTH if lsfMPTRequireAuth is set and MPToken doesn't exist or is
- * not authorized (explicitly or via credentials, if DomainID is set in
- * MPTokenIssuance). Consequently, if WeakAuth and lsfMPTRequireAuth is *not*
- * set, this function will return true even if MPToken does *not* exist.
+ * lsfMPTRequireAuth is set and MPToken is not authorized.
+ *
+ * If WeakAuth then return tecNO_AUTH if lsfMPTRequireAuth is set and MPToken
+ * doesn't exist or is not authorized (explicitly or via credentials, if
+ * DomainID is set in MPTokenIssuance). Consequently, if WeakAuth and
+ * lsfMPTRequireAuth is *not* set, this function will return true even if
+ * MPToken does *not* exist.
+ *
+ * The default "Legacy" auth type is equivalent to StrongAuth.
  */
 [[nodiscard]] TER
 requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
     AccountID const& account,
-    MPTAuthType authType = MPTAuthType::StrongAuth,
+    AuthType authType = AuthType::Legacy,
     int depth = 0);
 
 [[nodiscard]] TER inline requireAuth(
     ReadView const& view,
     Asset const& asset,
     AccountID const& account,
-    MPTAuthType authType = MPTAuthType::StrongAuth)
+    AuthType authType = AuthType::Legacy)
 {
     return std::visit(
         [&]<ValidIssueType TIss>(TIss const& issue_) {
-            if constexpr (std::is_same_v<TIss, Issue>)
-                return requireAuth(view, issue_, account);
-            else
-                return requireAuth(view, issue_, account, authType);
+            return requireAuth(view, issue_, account, authType);
         },
         asset.value());
 }

src/xrpld/ledger/detail/View.cpp

@@ -505,8 +505,8 @@ accountHolds(
         if (zeroIfUnauthorized == ahZERO_IF_UNAUTHORIZED &&
             view.rules().enabled(featureSingleAssetVault))
         {
-            if (auto const err = requireAuth(
-                    view, mptIssue, account, MPTAuthType::StrongAuth);
+            if (auto const err =
+                    requireAuth(view, mptIssue, account, AuthType::StrongAuth);
                 !isTesSuccess(err))
                 amount.clear(mptIssue);
         }
@@ -2298,15 +2298,27 @@ transferXRP(
 }
 
 TER
-requireAuth(ReadView const& view, Issue const& issue, AccountID const& account)
+requireAuth(
+    ReadView const& view,
+    Issue const& issue,
+    AccountID const& account,
+    AuthType authType)
 {
     if (isXRP(issue) || issue.account == account)
         return tesSUCCESS;
+
+    auto const trustLine =
+        view.read(keylet::line(account, issue.account, issue.currency));
+    // If account has no line, and this is a strong check, fail
+    if (!trustLine && authType == AuthType::StrongAuth)
+        return tecNO_LINE;
+
+    // If this is a weak or legacy check, or if the account has a line, fail if
+    // auth is required and not set on the line
     if (auto const issuerAccount = view.read(keylet::account(issue.account));
         issuerAccount && (*issuerAccount)[sfFlags] & lsfRequireAuth)
     {
-        if (auto const trustLine =
-                view.read(keylet::line(account, issue.account, issue.currency)))
+        if (trustLine)
             return ((*trustLine)[sfFlags] &
                     ((account > issue.account) ? lsfLowAuth : lsfHighAuth))
                 ? tesSUCCESS
@@ -2322,7 +2334,7 @@ requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
     AccountID const& account,
-    MPTAuthType authType,
+    AuthType authType,
     int depth)
 {
     auto const mptID = keylet::mptIssuance(mptIssue.getMptID());
@@ -2357,7 +2369,7 @@ requireAuth(
             if (auto const err = std::visit(
                     [&]<ValidIssueType TIss>(TIss const& issue) {
                         if constexpr (std::is_same_v<TIss, Issue>)
-                            return requireAuth(view, issue, account);
+                            return requireAuth(view, issue, account, authType);
                         else
                             return requireAuth(
                                 view, issue, account, authType, depth + 1);
@@ -2372,7 +2384,8 @@ requireAuth(
     auto const sleToken = view.read(mptokenID);
 
     // if account has no MPToken, fail
-    if (!sleToken && authType == MPTAuthType::StrongAuth)
+    if (!sleToken &&
+        (authType == AuthType::StrongAuth || authType == AuthType::Legacy))
         return tecNO_AUTH;
 
     // Note, this check is not amendment-gated because DomainID will be always