fix: Enforce reserve when creating trust line or MPToken in VaultWithdraw (#5857)

Similarly to other transaction typed that can create a trust line or MPToken for the transaction submitter (e.g. CashCheck #5285, EscrowFinish #5185 ), VaultWithdraw should enforce reserve before creating a new object. Additionally, the lsfRequireDestTag account flag should be enforced for the transaction submitter.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>

include/xrpl/protocol/detail/transactions.macro

@@ -909,7 +909,7 @@ TRANSACTION(ttVAULT_DEPOSIT, 68, VaultDeposit,
 TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw,
     Delegation::delegatable,
     featureSingleAssetVault,
-    mayDeleteMPT | mustModifyVault,
+    mayDeleteMPT | mayAuthorizeMPT | mustModifyVault,
     ({
     {sfVaultID, soeREQUIRED},
     {sfAmount, soeREQUIRED, soeMPTSupported},

src/libxrpl/ledger/View.cpp

@@ -1242,6 +1242,12 @@ addEmptyHolding(
     // If the line already exists, don't create it again.
     if (view.read(index))
         return tecDUPLICATE;
+
+    // Can the account cover the trust line reserve ?
+    std::uint32_t const ownerCount = sleDst->at(sfOwnerCount);
+    if (priorBalance < view.fees().accountReserve(ownerCount + 1))
+        return tecNO_LINE_INSUF_RESERVE;
+
     return trustCreate(
         view,
         high,

src/test/app/Vault_test.cpp

@@ -59,14 +59,15 @@ class Vault_test : public beast::unit_test::suite
     testSequences()
     {
         using namespace test::jtx;
+        Account issuer{"issuer"};
+        Account owner{"owner"};
+        Account depositor{"depositor"};
+        Account charlie{"charlie"};  // authorized 3rd party
+        Account dave{"dave"};
 
-        auto const testSequence = [this](
+        auto const testSequence = [&, this](
                                       std::string const& prefix,
                                       Env& env,
-                                      Account const& issuer,
-                                      Account const& owner,
-                                      Account const& depositor,
-                                      Account const& charlie,
                                       Vault& vault,
                                       PrettyAsset const& asset) {
             auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
@@ -104,11 +105,9 @@ class Vault_test : public beast::unit_test::suite
 
             // Several 3rd party accounts which cannot receive funds
             Account alice{"alice"};
-            Account dave{"dave"};
             Account erin{"erin"};  // not authorized by issuer
-            env.fund(XRP(1000), alice, dave, erin);
+            env.fund(XRP(1000), alice, erin);
             env(fset(alice, asfDepositAuth));
-            env(fset(dave, asfRequireDest));
             env.close();
 
             {
@@ -328,19 +327,6 @@ class Vault_test : public beast::unit_test::suite
                 env.close();
             }
 
-            {
-                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));
-                env.close();
-            }
-
             if (!asset.raw().native())
             {
                 testcase(
@@ -368,12 +354,49 @@ class Vault_test : public beast::unit_test::suite
                 env.close();
             }
 
+            {
+                testcase(prefix + " withdraw to 3rd party lsfRequireDestTag");
+                auto tx = vault.withdraw(
+                    {.depositor = depositor,
+                     .id = keylet.key,
+                     .amount = asset(50)});
+                tx[sfDestination] = dave.human();
+                tx[sfDestinationTag] = "0";
+                env(tx);
+                env.close();
+            }
+
+            {
+                testcase(prefix + " deposit again");
+                auto tx = vault.deposit(
+                    {.depositor = dave, .id = keylet.key, .amount = asset(50)});
+                env(tx);
+                env.close();
+            }
+
+            {
+                testcase(prefix + " fail to withdraw lsfRequireDestTag");
+                auto tx = vault.withdraw(
+                    {.depositor = dave, .id = keylet.key, .amount = asset(50)});
+                env(tx, ter{tecDST_TAG_NEEDED});
+                env.close();
+            }
+
+            {
+                testcase(prefix + " withdraw with tag");
+                auto tx = vault.withdraw(
+                    {.depositor = dave, .id = keylet.key, .amount = asset(50)});
+                tx[sfDestinationTag] = "0";
+                env(tx);
+                env.close();
+            }
+
             {
                 testcase(prefix + " withdraw to authorized 3rd party");
                 auto tx = vault.withdraw(
                     {.depositor = depositor,
                      .id = keylet.key,
-                     .amount = asset(100)});
+                     .amount = asset(50)});
                 tx[sfDestination] = charlie.human();
                 env(tx);
                 env.close();
@@ -523,80 +546,56 @@ class Vault_test : public beast::unit_test::suite
             }
         };
 
-        auto testCases = [this, &testSequence](
+        auto testCases = [&, this](
                              std::string prefix,
-                             std::function<PrettyAsset(
+                             std::function<PrettyAsset(Env & env)> setup) {
-                                 Env & env,
-                                 Account const& issuer,
-                                 Account const& owner,
-                                 Account const& depositor,
-                                 Account const& charlie)> setup) {
             Env env{*this, testable_amendments() | featureSingleAssetVault};
-            Account issuer{"issuer"};
+
-            Account owner{"owner"};
-            Account depositor{"depositor"};
-            Account charlie{"charlie"};  // authorized 3rd party
             Vault vault{env};
-            env.fund(XRP(1000), issuer, owner, depositor, charlie);
+            env.fund(XRP(1000), issuer, owner, depositor, charlie, dave);
             env.close();
             env(fset(issuer, asfAllowTrustLineClawback));
             env(fset(issuer, asfRequireAuth));
+            env(fset(dave, asfRequireDest));
             env.close();
             env.require(flags(issuer, asfAllowTrustLineClawback));
             env.require(flags(issuer, asfRequireAuth));
 
-            PrettyAsset asset = setup(env, issuer, owner, depositor, charlie);
+            PrettyAsset asset = setup(env);
-            testSequence(
+            testSequence(prefix, env, vault, asset);
-                prefix, env, issuer, owner, depositor, charlie, vault, asset);
         };
 
-        testCases(
+        testCases("XRP", [&](Env& env) -> PrettyAsset {
-            "XRP",
+            return {xrpIssue(), 1'000'000};
-            [](Env& env,
+        });
-               Account const& issuer,
-               Account const& owner,
-               Account const& depositor,
-               Account const& charlie) -> PrettyAsset {
-                return {xrpIssue(), 1'000'000};
-            });
 
-        testCases(
+        testCases("IOU", [&](Env& env) -> Asset {
-            "IOU",
+            PrettyAsset asset = issuer["IOU"];
-            [](Env& env,
+            env(trust(owner, asset(1000)));
-               Account const& issuer,
+            env(trust(depositor, asset(1000)));
-               Account const& owner,
+            env(trust(charlie, asset(1000)));
-               Account const& depositor,
+            env(trust(dave, asset(1000)));
-               Account const& charlie) -> Asset {
+            env(trust(issuer, asset(0), owner, tfSetfAuth));
-                PrettyAsset asset = issuer["IOU"];
+            env(trust(issuer, asset(0), depositor, tfSetfAuth));
-                env(trust(owner, asset(1000)));
+            env(trust(issuer, asset(0), charlie, tfSetfAuth));
-                env(trust(depositor, asset(1000)));
+            env(trust(issuer, asset(0), dave, tfSetfAuth));
-                env(trust(charlie, asset(1000)));
+            env(pay(issuer, depositor, asset(1000)));
-                env(trust(issuer, asset(0), owner, tfSetfAuth));
+            env.close();
-                env(trust(issuer, asset(0), depositor, tfSetfAuth));
+            return asset;
-                env(trust(issuer, asset(0), charlie, tfSetfAuth));
+        });
-                env(pay(issuer, depositor, asset(1000)));
-                env.close();
-                return asset;
-            });
 
-        testCases(
+        testCases("MPT", [&](Env& env) -> Asset {
-            "MPT",
+            MPTTester mptt{env, issuer, mptInitNoFund};
-            [](Env& env,
+            mptt.create(
-               Account const& issuer,
+                {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock});
-               Account const& owner,
+            PrettyAsset asset = mptt.issuanceID();
-               Account const& depositor,
+            mptt.authorize({.account = depositor});
-               Account const& charlie) -> Asset {
+            mptt.authorize({.account = charlie});
-                MPTTester mptt{env, issuer, mptInitNoFund};
+            mptt.authorize({.account = dave});
-                mptt.create(
+            env(pay(issuer, depositor, asset(1000)));
-                    {.flags =
+            env.close();
-                         tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock});
+            return asset;
-                PrettyAsset asset = mptt.issuanceID();
+        });
-                mptt.authorize({.account = depositor});
-                mptt.authorize({.account = charlie});
-                env(pay(issuer, depositor, asset(1000)));
-                env.close();
-                return asset;
-            });
     }
 
     void
@@ -1672,6 +1671,7 @@ class Vault_test : public beast::unit_test::suite
         {
             bool enableClawback = true;
             bool requireAuth = true;
+            int initialXRP = 1000;
         };
 
         auto testCase = [this](
@@ -1688,7 +1688,7 @@ class Vault_test : public beast::unit_test::suite
             Account issuer{"issuer"};
             Account owner{"owner"};
             Account depositor{"depositor"};
-            env.fund(XRP(1000), issuer, owner, depositor);
+            env.fund(XRP(args.initialXRP), issuer, owner, depositor);
             env.close();
             Vault vault{env};
 
@@ -1868,9 +1868,7 @@ class Vault_test : public beast::unit_test::suite
                 PrettyAsset const& asset,
                 Vault& vault,
                 MPTTester& mptt) {
-                testcase(
+                testcase("MPT depositor without MPToken, auth required");
-                    "MPT 3rd party without MPToken cannot be withdrawal "
-                    "destination");
 
                 auto [tx, keylet] =
                     vault.create({.owner = owner, .asset = asset});
@@ -1880,10 +1878,32 @@ class Vault_test : public beast::unit_test::suite
                 tx = vault.deposit(
                     {.depositor = depositor,
                      .id = keylet.key,
-                     .amount = asset(100)});
+                     .amount = asset(1000)});
                 env(tx);
                 env.close();
 
+                {
+                    // Remove depositor MPToken and it will not be re-created
+                    mptt.authorize(
+                        {.account = depositor, .flags = tfMPTUnauthorize});
+                    env.close();
+
+                    auto const mptoken =
+                        keylet::mptoken(mptt.issuanceID(), depositor);
+                    auto const sleMPT1 = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT1 == nullptr);
+
+                    tx = vault.withdraw(
+                        {.depositor = depositor,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx, ter{tecNO_AUTH});
+                    env.close();
+
+                    auto const sleMPT2 = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT2 == nullptr);
+                }
+
                 {
                     // Set destination to 3rd party without MPToken
                     Account charlie{"charlie"};
@@ -1898,7 +1918,7 @@ class Vault_test : public beast::unit_test::suite
                     env(tx, ter(tecNO_AUTH));
                 }
             },
-            {.requireAuth = false});
+            {.requireAuth = true});
 
         testCase(
             [this](
@@ -1909,7 +1929,7 @@ class Vault_test : public beast::unit_test::suite
                 PrettyAsset const& asset,
                 Vault& vault,
                 MPTTester& mptt) {
-                testcase("MPT depositor without MPToken cannot withdraw");
+                testcase("MPT depositor without MPToken, no auth required");
 
                 auto [tx, keylet] =
                     vault.create({.owner = owner, .asset = asset});
@@ -1917,7 +1937,6 @@ class Vault_test : public beast::unit_test::suite
                 env.close();
                 auto v = env.le(keylet);
                 BEAST_EXPECT(v);
-                MPTID share = (*v)[sfShareMPTID];
 
                 tx = vault.deposit(
                     {.depositor = depositor,
@@ -1927,41 +1946,120 @@ class Vault_test : public beast::unit_test::suite
                 env.close();
 
                 {
-                    // Remove depositor's MPToken and withdraw will fail
+                    // Remove depositor's MPToken and it will be re-created
                     mptt.authorize(
                         {.account = depositor, .flags = tfMPTUnauthorize});
                     env.close();
+
                     auto const mptoken =
-                        env.le(keylet::mptoken(mptt.issuanceID(), depositor));
+                        keylet::mptoken(mptt.issuanceID(), depositor);
-                    BEAST_EXPECT(mptoken == nullptr);
+                    auto const sleMPT1 = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT1 == nullptr);
 
                     tx = vault.withdraw(
                         {.depositor = depositor,
                          .id = keylet.key,
                          .amount = asset(100)});
-                    env(tx, ter(tecNO_AUTH));
+                    env(tx);
+                    env.close();
+
+                    auto const sleMPT2 = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT2 != nullptr);
+                    BEAST_EXPECT(sleMPT2->at(sfMPTAmount) == 100);
                 }
 
                 {
-                    // Restore depositor's MPToken and withdraw will succeed
+                    // Remove 3rd party MPToken and it will not be re-created
-                    mptt.authorize({.account = depositor});
+                    mptt.authorize(
+                        {.account = owner, .flags = tfMPTUnauthorize});
                     env.close();
 
+                    auto const mptoken =
+                        keylet::mptoken(mptt.issuanceID(), owner);
+                    auto const sleMPT1 = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT1 == nullptr);
+
                     tx = vault.withdraw(
                         {.depositor = depositor,
                          .id = keylet.key,
-                         .amount = asset(1000)});
+                         .amount = asset(100)});
-                    env(tx);
+                    tx[sfDestination] = owner.human();
+                    env(tx, ter(tecNO_AUTH));
                     env.close();
 
-                    // Withdraw removed shares MPToken
+                    auto const sleMPT2 = env.le(mptoken);
-                    auto const mptSle =
+                    BEAST_EXPECT(sleMPT2 == nullptr);
-                        env.le(keylet::mptoken(share, depositor.id()));
-                    BEAST_EXPECT(mptSle == nullptr);
                 }
             },
             {.requireAuth = false});
 
+        auto const [acctReserve, incReserve] = [this]() -> std::pair<int, int> {
+            Env env{*this, testable_amendments()};
+            return {
+                env.current()->fees().accountReserve(0).drops() /
+                    DROPS_PER_XRP.drops(),
+                env.current()->fees().increment.drops() /
+                    DROPS_PER_XRP.drops()};
+        }();
+
+        testCase(
+            [&, this](
+                Env& env,
+                Account const& issuer,
+                Account const& owner,
+                Account const& depositor,
+                PrettyAsset const& asset,
+                Vault& vault,
+                MPTTester& mptt) {
+                testcase("MPT failed reserve to re-create MPToken");
+
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                env(tx);
+                env.close();
+                auto v = env.le(keylet);
+                BEAST_EXPECT(v);
+
+                env(pay(depositor, owner, asset(1000)));
+                env.close();
+
+                tx = vault.deposit(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(1000)});  // all assets held by owner
+                env(tx);
+                env.close();
+
+                {
+                    // Remove owners's MPToken and it will not be re-created
+                    mptt.authorize(
+                        {.account = owner, .flags = tfMPTUnauthorize});
+                    env.close();
+
+                    auto const mptoken =
+                        keylet::mptoken(mptt.issuanceID(), owner);
+                    auto const sleMPT = env.le(mptoken);
+                    BEAST_EXPECT(sleMPT == nullptr);
+
+                    // No reserve to create MPToken for asset in VaultWithdraw
+                    tx = vault.withdraw(
+                        {.depositor = owner,
+                         .id = keylet.key,
+                         .amount = asset(100)});
+                    env(tx, ter{tecINSUFFICIENT_RESERVE});
+                    env.close();
+
+                    env(pay(depositor, owner, XRP(incReserve)));
+                    env.close();
+
+                    // Withdraw can now create asset MPToken, tx will succeed
+                    env(tx);
+                    env.close();
+                }
+            },
+            {.requireAuth = false,
+             .initialXRP = acctReserve + incReserve * 4 - 1});
+
         testCase([this](
                      Env& env,
                      Account const& issuer,
@@ -2320,23 +2418,30 @@ class Vault_test : public beast::unit_test::suite
     {
         using namespace test::jtx;
 
+        struct CaseArgs
+        {
+            int initialXRP = 1000;
+            double transferRate = 1.0;
+        };
+
         auto testCase =
-            [&,
+            [&, this](
-             this](std::function<void(
+                std::function<void(
-                       Env & env,
+                    Env & env,
-                       Account const& owner,
+                    Account const& owner,
-                       Account const& issuer,
+                    Account const& issuer,
-                       Account const& charlie,
+                    Account const& charlie,
-                       std::function<Account(ripple::Keylet)> vaultAccount,
+                    std::function<Account(ripple::Keylet)> vaultAccount,
-                       Vault& vault,
+                    Vault& vault,
-                       PrettyAsset const& asset,
+                    PrettyAsset const& asset,
-                       std::function<MPTID(ripple::Keylet)> issuanceId)> test) {
+                    std::function<MPTID(ripple::Keylet)> issuanceId)> test,
+                CaseArgs args = {}) {
                 Env env{*this, testable_amendments() | featureSingleAssetVault};
                 Account const owner{"owner"};
                 Account const issuer{"issuer"};
                 Account const charlie{"charlie"};
                 Vault vault{env};
-                env.fund(XRP(1000), issuer, owner, charlie);
+                env.fund(XRP(args.initialXRP), issuer, owner, charlie);
                 env(fset(issuer, asfAllowTrustLineClawback));
                 env.close();
 
@@ -2344,7 +2449,7 @@ class Vault_test : public beast::unit_test::suite
                 env.trust(asset(1000), owner);
                 env.trust(asset(1000), charlie);
                 env(pay(issuer, owner, asset(200)));
-                env(rate(issuer, 1.25));
+                env(rate(issuer, args.transferRate));
                 env.close();
 
                 auto const vaultAccount =
@@ -2505,73 +2610,81 @@ class Vault_test : public beast::unit_test::suite
             env.close();
         });
 
-        testCase([&, this](
+        testCase(
-                     Env& env,
+            [&, this](
-                     Account const& owner,
+                Env& env,
-                     Account const& issuer,
+                Account const& owner,
-                     Account const& charlie,
+                Account const& issuer,
-                     auto vaultAccount,
+                Account const& charlie,
-                     Vault& vault,
+                auto vaultAccount,
-                     PrettyAsset const& asset,
+                Vault& vault,
-                     auto issuanceId) {
+                PrettyAsset const& asset,
-            testcase("IOU transfer fees not applied");
+                auto issuanceId) {
+                testcase("IOU transfer fees not applied");
 
-            auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
+                auto [tx, keylet] =
-            env(tx);
+                    vault.create({.owner = owner, .asset = asset});
-            env.close();
-
-            env(vault.deposit(
-                {.depositor = owner, .id = keylet.key, .amount = asset(100)}));
-            env.close();
-
-            auto const issue = asset.raw().get<Issue>();
-            Asset const share = Asset(issuanceId(keylet));
-
-            // transfer fees ignored on deposit
-            BEAST_EXPECT(env.balance(owner, issue) == asset(100));
-            BEAST_EXPECT(
-                env.balance(vaultAccount(keylet), issue) == asset(100));
-
-            {
-                auto tx = vault.clawback(
-                    {.issuer = issuer,
-                     .id = keylet.key,
-                     .holder = owner,
-                     .amount = asset(50)});
                 env(tx);
                 env.close();
-            }
 
-            // transfer fees ignored on clawback
+                env(vault.deposit(
-            BEAST_EXPECT(env.balance(owner, issue) == asset(100));
-            BEAST_EXPECT(env.balance(vaultAccount(keylet), issue) == asset(50));
-
-            env(vault.withdraw(
-                {.depositor = owner,
-                 .id = keylet.key,
-                 .amount = share(20'000'000)}));
-
-            // transfer fees ignored on withdraw
-            BEAST_EXPECT(env.balance(owner, issue) == asset(120));
-            BEAST_EXPECT(env.balance(vaultAccount(keylet), issue) == asset(30));
-
-            {
-                auto tx = vault.withdraw(
                     {.depositor = owner,
                      .id = keylet.key,
-                     .amount = share(30'000'000)});
+                     .amount = asset(100)}));
-                tx[sfDestination] = charlie.human();
+                env.close();
-                env(tx);
-            }
 
-            // transfer fees ignored on withdraw to 3rd party
+                auto const issue = asset.raw().get<Issue>();
-            BEAST_EXPECT(env.balance(owner, issue) == asset(120));
+                Asset const share = Asset(issuanceId(keylet));
-            BEAST_EXPECT(env.balance(charlie, issue) == asset(30));
-            BEAST_EXPECT(env.balance(vaultAccount(keylet), issue) == asset(0));
 
-            env(vault.del({.owner = owner, .id = keylet.key}));
+                // transfer fees ignored on deposit
-            env.close();
+                BEAST_EXPECT(env.balance(owner, issue) == asset(100));
-        });
+                BEAST_EXPECT(
+                    env.balance(vaultAccount(keylet), issue) == asset(100));
+
+                {
+                    auto tx = vault.clawback(
+                        {.issuer = issuer,
+                         .id = keylet.key,
+                         .holder = owner,
+                         .amount = asset(50)});
+                    env(tx);
+                    env.close();
+                }
+
+                // transfer fees ignored on clawback
+                BEAST_EXPECT(env.balance(owner, issue) == asset(100));
+                BEAST_EXPECT(
+                    env.balance(vaultAccount(keylet), issue) == asset(50));
+
+                env(vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = share(20'000'000)}));
+
+                // transfer fees ignored on withdraw
+                BEAST_EXPECT(env.balance(owner, issue) == asset(120));
+                BEAST_EXPECT(
+                    env.balance(vaultAccount(keylet), issue) == asset(30));
+
+                {
+                    auto tx = vault.withdraw(
+                        {.depositor = owner,
+                         .id = keylet.key,
+                         .amount = share(30'000'000)});
+                    tx[sfDestination] = charlie.human();
+                    env(tx);
+                }
+
+                // transfer fees ignored on withdraw to 3rd party
+                BEAST_EXPECT(env.balance(owner, issue) == asset(120));
+                BEAST_EXPECT(env.balance(charlie, issue) == asset(30));
+                BEAST_EXPECT(
+                    env.balance(vaultAccount(keylet), issue) == asset(0));
+
+                env(vault.del({.owner = owner, .id = keylet.key}));
+                env.close();
+            },
+            CaseArgs{.transferRate = 1.25});
 
         testCase([&, this](
                      Env& env,
@@ -2713,6 +2826,103 @@ class Vault_test : public beast::unit_test::suite
             env(tx1);
         });
 
+        auto const [acctReserve, incReserve] = [this]() -> std::pair<int, int> {
+            Env env{*this, testable_amendments()};
+            return {
+                env.current()->fees().accountReserve(0).drops() /
+                    DROPS_PER_XRP.drops(),
+                env.current()->fees().increment.drops() /
+                    DROPS_PER_XRP.drops()};
+        }();
+
+        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 no reserve");
+                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);
+
+                // Fail because not enough reserve to create trust line
+                tx = vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(10)});
+                env(tx, ter{tecNO_LINE_INSUF_RESERVE});
+                env.close();
+
+                env(pay(charlie, owner, XRP(incReserve)));
+                env.close();
+
+                // Withdraw can now create trust line, will succeed
+                env(tx);
+                env.close();
+            },
+            CaseArgs{.initialXRP = acctReserve + incReserve * 4 - 1});
+
+        testCase(
+            [&, this](
+                Env& env,
+                Account const& owner,
+                Account const& issuer,
+                Account const& charlie,
+                auto,
+                Vault& vault,
+                PrettyAsset const& asset,
+                auto&&...) {
+                testcase("IOU no reserve for share MPToken");
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                env(tx);
+                env.close();
+
+                env(pay(owner, charlie, asset(100)));
+                env.close();
+
+                // Use up some reserve on tickets
+                env(ticket::create(charlie, 2));
+                env.close();
+
+                // Fail because not enough reserve to create MPToken for shares
+                tx = vault.deposit(
+                    {.depositor = charlie,
+                     .id = keylet.key,
+                     .amount = asset(100)});
+                env(tx, ter{tecINSUFFICIENT_RESERVE});
+                env.close();
+
+                env(pay(issuer, charlie, XRP(incReserve)));
+                env.close();
+
+                // Deposit can now create MPToken, will succeed
+                env(tx);
+                env.close();
+            },
+            CaseArgs{.initialXRP = acctReserve + incReserve * 4 - 1});
+
         testCase([&, this](
                      Env& env,
                      Account const& owner,

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

@@ -202,8 +202,7 @@ VaultDeposit::doApply()
     else  // !vault->isFlag(lsfVaultPrivate) || account_ == vault->at(sfOwner)
     {
         // No authorization needed, but must ensure there is MPToken
-        auto sleMpt = view().read(keylet::mptoken(mptIssuanceID, account_));
+        if (!view().exists(keylet::mptoken(mptIssuanceID, account_)))
-        if (!sleMpt)
         {
             if (auto const err = authorizeMPToken(
                     view(),

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

@@ -52,12 +52,6 @@ VaultWithdraw::preflight(PreflightContext const& ctx)
             return temMALFORMED;
         }
     }
-    else if (ctx.tx.isFieldPresent(sfDestinationTag))
-    {
-        JLOG(ctx.j.debug()) << "VaultWithdraw: sfDestinationTag is set but "
-                               "sfDestination is not";
-        return temMALFORMED;
-    }
 
     return tesSUCCESS;
 }
@@ -116,37 +110,28 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
     }
 
     auto const account = ctx.tx[sfAccount];
-    auto const dstAcct = [&]() -> AccountID {
+    auto const dstAcct = ctx.tx[~sfDestination].value_or(account);
-        if (ctx.tx.isFieldPresent(sfDestination))
+    auto const sleDst = ctx.view.read(keylet::account(dstAcct));
-            return ctx.tx.getAccountID(sfDestination);
+    if (sleDst == nullptr)
-        return account;
+        return account == dstAcct ? tecINTERNAL : tecNO_DST;
-    }();
+
+    if (sleDst->isFlag(lsfRequireDestTag) &&
+        !ctx.tx.isFieldPresent(sfDestinationTag))
+        return tecDST_TAG_NEEDED;  // Cannot send without a tag
 
     // 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 && sleDst->isFlag(lsfDepositAuth))
-    if (account != dstAcct)
     {
-        auto const sleDst = ctx.view.read(keylet::account(dstAcct));
+        if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
-        if (sleDst == nullptr)
+            return tecNO_PERMISSION;
-            return tecNO_DST;
-
-        if (sleDst->isFlag(lsfRequireDestTag) &&
-            !ctx.tx.isFieldPresent(sfDestinationTag))
-            return tecDST_TAG_NEEDED;  // Cannot send without a tag
-
-        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 (for an MPT) or trust line (for an IOU) must exist
+    // If sending to Account (i.e. not a transfer), we will also create (only
-    // if not sending to Account.
+    // if authorized) a trust line or MPToken as needed, in doApply().
+    // Destination MPToken or trust line must exist if _not_ sending to Account.
+    AuthType const authType =
+        account == dstAcct ? AuthType::WeakAuth : AuthType::StrongAuth;
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType);
         !isTesSuccess(ter))
         return ter;
@@ -307,11 +292,16 @@ VaultWithdraw::doApply()
         // else quietly ignore, account balance is not zero
     }
 
-    auto const dstAcct = [&]() -> AccountID {
+    auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);
-        if (ctx_.tx.isFieldPresent(sfDestination))
+    if (!vaultAsset.native() &&               //
-            return ctx_.tx.getAccountID(sfDestination);
+        dstAcct != vaultAsset.getIssuer() &&  //
-        return account_;
+        dstAcct == account_)
-    }();
+    {
+        if (auto const ter = addEmptyHolding(
+                view(), account_, mPriorBalance, vaultAsset, j_);
+            !isTesSuccess(ter) && ter != tecDUPLICATE)
+            return ter;
+    }
 
     // Transfer assets from vault to depositor or destination account.
     if (auto const ter = accountSend(