From f839049de76b940a0d7b71f90cecc87ed8a35e47 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 7 Apr 2025 16:15:36 +0100 Subject: [PATCH] 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 | 2 + src/test/app/Vault_test.cpp | 119 +++++++++++++++++++++++ src/xrpld/app/tx/detail/VaultCreate.cpp | 12 +++ src/xrpld/app/tx/detail/VaultDeposit.cpp | 21 ++-- src/xrpld/ledger/View.h | 25 ++--- src/xrpld/ledger/detail/View.cpp | 10 +- 6 files changed, 160 insertions(+), 29 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index b9cc7df4b5..08c7a291a5 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/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. */ diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index d06b6774d5..b650d1a037 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -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 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(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 diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index aec4a389b8..aa3fc0b541 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/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()) + { + auto const mptIssue = asset.get(); + 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; diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index e9c3aa053e..2226b85330 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/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() ? 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; - - // Defensive check, given above `if (asset.asset() != vaultAsset)` - if (share == assets.asset()) - return tecWRONG_ASSET; + return tecLOCKED; 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( + [&](TIss const& issue) -> TER { + return requireAuth(ctx.view, issue, account); + }, + vaultAsset.value()); + !isTesSuccess(ter)) + return ter; + if (assets.holds()) { auto mptID = assets.get().getMptID(); diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index d62698e949..8e925bedaf 100644 --- a/src/xrpld/ledger/View.h +++ b/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) { - return isAnyFrozen(view, account1, account2, issue); + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return isAnyFrozen(view, account1, account2, issue); + else + return isAnyFrozen(view, account1, account2, issue, depth); }, asset.value()); } -*/ [[nodiscard]] bool isDeepFrozen( diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index f613fd3ba7..68b5c3671c 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/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 isFrozen(view, issuer, asset, depth + 1) || - isFrozen(view, account, asset, depth + 1); + return isAnyFrozen(view, issuer, account, vault->at(sfAsset), depth + 1); } bool @@ -2273,8 +2271,8 @@ requireAuth( if (!sleVault) return tefINTERNAL; // LCOV_EXCL_LINE - if (depth > maxFreezeCheckDepth) - return tecKILLED; // TODO: consider different code + if (depth >= maxFreezeCheckDepth) + return tecKILLED; // VaultCreate looks for this error code auto const asset = sleVault->at(sfAsset); return std::visit(