Enforce max recursion depth in VaultCreate, improve VaultDeposit checks

Also fix off-by-one error in recursive checks isFrozen and requireAuth

include/xrpl/protocol/Protocol.h

@@ -122,6 +122,8 @@ std::size_t constexpr maxDataPayloadLength = 256;
 /** Vault withdrawal policies */
 std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1;
 
+/** Maximum recursion depth for vault shares being put as an asset inside
+ * another vault; counted from 0 */
 std::uint8_t constexpr maxFreezeCheckDepth = 5;
 
 /** A ledger index. */

src/test/app/Vault_test.cpp

@@ -38,6 +38,7 @@
 #include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Issue.h>
 #include <xrpl/protocol/MPTIssue.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
@@ -1062,6 +1063,124 @@ class Vault_test : public beast::unit_test::suite
             tx = vault.del({.owner = owner, .id = keylet.key});
             env(tx);
         });
+
+        {
+            testcase("MPT maximum asset recursion");
+
+            Env env{*this, supported_amendments() | featureSingleAssetVault};
+            Account owner{"owner"};
+            Account issuer{"issuer"};
+            env.fund(XRP(1000000), owner, issuer);
+            env.close();
+            Vault vault{env};
+
+            struct Vault
+            {
+                Keylet keylet;
+                PrettyAsset shares;
+                PrettyAsset assets;
+            };
+            std::vector<Vault> vaults;
+
+            MPTTester mptt{env, issuer, mptInitNoFund};
+            mptt.create(
+                {.flags = tfMPTCanTransfer | tfMPTCanLock | lsfMPTCanClawback |
+                     tfMPTRequireAuth});
+            mptt.authorize({.account = owner});
+            mptt.authorize({.account = issuer, .holder = owner});
+            {
+                for (int i = 0; i < 5; ++i)
+                    vaults.emplace_back(
+                        keylet::vault(beast::zero), xrpIssue(), xrpIssue());
+
+                PrettyAsset asset = mptt.issuanceID();
+                env(pay(issuer, owner, asset(100)));
+                for (auto& v : vaults)
+                {
+                    auto [tx, k] =
+                        vault.create({.owner = owner, .asset = asset});
+                    env(tx);
+                    env.close();
+
+                    v.keylet = k;
+                    v.assets = asset;
+                    v.shares = [&env, keylet = k, this]() -> Asset {
+                        auto const vault = env.le(keylet);
+                        BEAST_EXPECT(vault != nullptr);
+                        return MPTIssue(vault->at(sfShareMPTID));
+                    }();
+                    asset = v.shares;
+                }
+            }
+
+            env(std::get<Json::Value>(vault.create(
+                    {.owner = owner, .asset = vaults.back().shares})),
+                ter{tecKILLED});  // exceeded maximum recursion depth
+
+            for (auto& v : vaults)
+            {
+                env(vault.deposit(
+                    {.depositor = owner,
+                     .id = v.keylet.key,
+                     .amount = v.assets(100)}));
+                env.close();
+            }
+
+            {
+                testcase("MPT recursive authorization and lock check");
+                auto& v = vaults.back();
+
+                mptt.authorize(
+                    {.account = issuer,
+                     .holder = owner,
+                     .flags = tfMPTUnauthorize});
+
+                env(vault.deposit(
+                        {.depositor = owner,
+                         .id = v.keylet.key,
+                         .amount = v.assets(10)}),
+                    ter{tecNO_AUTH});
+                env(vault.withdraw(
+                        {.depositor = owner,
+                         .id = v.keylet.key,
+                         .amount = v.assets(10)}),
+                    ter{tecNO_AUTH});
+                env.close();
+
+                mptt.authorize({.account = issuer, .holder = owner});
+                mptt.set(
+                    {.account = issuer, .holder = owner, .flags = tfMPTLock});
+
+                env(vault.deposit(
+                        {.depositor = owner,
+                         .id = v.keylet.key,
+                         .amount = v.assets(10)}),
+                    ter{tecLOCKED});
+                env(vault.withdraw(
+                        {.depositor = owner,
+                         .id = v.keylet.key,
+                         .amount = v.assets(10)}),
+                    ter{tecLOCKED});
+                env.close();
+
+                mptt.set(
+                    {.account = issuer, .holder = owner, .flags = tfMPTUnlock});
+            }
+
+            for (auto i = vaults.rbegin(); i != vaults.rend(); ++i)
+            {
+                auto v = *i;
+                env(vault.withdraw(
+                    {.depositor = owner,
+                     .id = v.keylet.key,
+                     .amount = v.shares(100)}));
+            }
+
+            for (auto& v : vaults)
+            {
+                env(vault.del({.owner = owner, .id = v.keylet.key}));
+            }
+        }
     }
 
     void

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

@@ -110,6 +110,18 @@ VaultCreate::preclaim(PreclaimContext const& ctx)
             return tecNO_AUTH;
     }
 
+    // Check for excessive vault shares recursion, which is reported by
+    // requireAuth as tecKILLED. The vault owner might not be permissioned to
+    // hold assets but that's OK, it only means that the owner won't be able to
+    // withdraw from or deposit into the vault (but other users would be fine).
+    if (asset.holds<MPTIssue>())
+    {
+        auto const mptIssue = asset.get<MPTIssue>();
+        if (auto const ter = requireAuth(ctx.view, mptIssue, account, 1);
+            ter == tecKILLED)
+            return ter;
+    }
+
     // Cannot create Vault for an Asset frozen for the vault owner
     if (isFrozen(ctx.view, account, asset))
         return tecFROZEN;

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

@@ -66,18 +66,17 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (assets.asset() != vaultAsset)
         return tecWRONG_ASSET;
 
+    auto const share = MPTIssue(vault->at(sfShareMPTID));
+    if (share == assets.asset())
+        return tefINTERNAL;
+
     // Cannot deposit inside Vault an Asset frozen for the depositor
     if (isFrozen(ctx.view, account, vaultAsset))
-        return tecFROZEN;
+        return vaultAsset.holds<Issue>() ? tecFROZEN : tecLOCKED;
 
-    auto const share = MPTIssue(vault->at(sfShareMPTID));
     // Cannot deposit if the shares of the vault are frozen
     if (isFrozen(ctx.view, account, share))
-        return tecFROZEN;
+        return tecLOCKED;
-
-    // Defensive check, given above `if (asset.asset() != vaultAsset)`
-    if (share == assets.asset())
-        return tecWRONG_ASSET;
 
     if ((vault->getFlags() & tfVaultPrivate) && account != vault->at(sfOwner))
     {
@@ -94,6 +93,14 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
             return ter;
     }
 
+    if (auto const ter = std::visit(
+            [&]<ValidIssueType TIss>(TIss const& issue) -> TER {
+                return requireAuth(ctx.view, issue, account);
+            },
+            vaultAsset.value());
+        !isTesSuccess(ter))
+        return ter;
+
     if (assets.holds<MPTIssue>())
     {
         auto mptID = assets.get<MPTIssue>().getMptID();

src/xrpld/ledger/View.h

@@ -177,17 +177,6 @@ isAnyFrozen(
     MPTIssue const& mptIssue,
     int depth = 0);
 
-/*
-We do not have a use case for these (yet ?)
-
-[[nodiscard]] bool
-isAnyFrozen(
-    ReadView const& view,
-    AccountID const& account1,
-    AccountID const& account2,
-    Currency const& currency,
-    AccountID const& issuer);
-
 [[nodiscard]] inline bool
 isAnyFrozen(
     ReadView const& view,
@@ -195,7 +184,8 @@ isAnyFrozen(
     AccountID const& account2,
     Issue const& issue)
 {
-    return isAnyFrozen(view, account1, account2, issue.currency, issue.account);
+    return isFrozen(view, account1, issue.currency, issue.account) ||
+        isFrozen(view, account2, issue.currency, issue.account);
 }
 
 [[nodiscard]] inline bool
@@ -203,15 +193,18 @@ isAnyFrozen(
     ReadView const& view,
     AccountID const& account1,
     AccountID const& account2,
-    Asset const& asset)
+    Asset const& asset,
+    int depth = 0)
 {
     return std::visit(
-        [&](auto const& issue) {
+        [&]<ValidIssueType TIss>(TIss const& issue) {
-            return isAnyFrozen(view, account1, account2, issue);
+            if constexpr (std::is_same_v<TIss, Issue>)
+                return isAnyFrozen(view, account1, account2, issue);
+            else
+                return isAnyFrozen(view, account1, account2, issue, depth);
         },
         asset.value());
 }
-*/
 
 [[nodiscard]] bool
 isDeepFrozen(

src/xrpld/ledger/detail/View.cpp

@@ -328,7 +328,7 @@ isVaultPseudoAccountFrozen(
     if (!mptIssuer->isFieldPresent(sfVaultID))
         return false;  // not a Vault pseudo-account, common case
 
-    if (depth > maxFreezeCheckDepth)
+    if (depth >= maxFreezeCheckDepth)
         return true;  // fail at maximum 2^maxFreezeCheckDepth checks
 
     auto const vault =
@@ -340,9 +340,7 @@ isVaultPseudoAccountFrozen(
         return false;  // LCOV_EXCL_LINE
     }
 
-    Asset const asset{vault->at(sfAsset)};
+    return isAnyFrozen(view, issuer, account, vault->at(sfAsset), depth + 1);
-    return isFrozen(view, issuer, asset, depth + 1) ||
-        isFrozen(view, account, asset, depth + 1);
 }
 
 bool
@@ -2273,8 +2271,8 @@ requireAuth(
             if (!sleVault)
                 return tefINTERNAL;  // LCOV_EXCL_LINE
 
-            if (depth > maxFreezeCheckDepth)
+            if (depth >= maxFreezeCheckDepth)
-                return tecKILLED;  // TODO: consider different code
+                return tecKILLED;  // VaultCreate looks for this error code
 
             auto const asset = sleVault->at(sfAsset);
             return std::visit(