diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 1d9fb024eb..3ea111eda8 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -17,6 +17,8 @@ */ //============================================================================== +#include +#include #include #include #include @@ -761,21 +763,71 @@ class Vault_test : public beast::unit_test::suite testCreateFailIOU() { using namespace test::jtx; - Env env{*this, supported_amendments() | featureSingleAssetVault}; - Account issuer{"issuer"}; - Account owner{"owner"}; - Account depositor{"depositor"}; - env.fund(XRP(1000), issuer, owner, depositor); - env.close(); - Vault vault{env}; - Asset asset = issuer["IOU"]; + { + testcase("IOU fail create frozen"); + Env env{*this, supported_amendments() | featureSingleAssetVault}; + Account issuer{"issuer"}; + Account owner{"owner"}; + Account depositor{"depositor"}; + env.fund(XRP(1000), issuer, owner, depositor); + env.close(); + Vault vault{env}; + Asset asset = issuer["IOU"]; - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(fset(issuer, asfGlobalFreeze)); - env.close(); - env(tx, ter(tecFROZEN)); - env.close(); + env(fset(issuer, asfGlobalFreeze)); + env.close(); + env(tx, ter(tecFROZEN)); + env.close(); + } + + { + testcase("IOU fail create vault for AMM LPToken"); + Env env{*this, supported_amendments() | featureSingleAssetVault}; + Account const gw("gateway"); + Account const alice("alice"); + Account const carol("carol"); + IOU const USD = gw["USD"]; + + auto const [asset1, asset2] = + std::pair(XRP(10000), USD(10000)); + auto tofund = [&](STAmount const& a) -> STAmount { + if (a.native()) + { + auto const defXRP = XRP(30000); + if (a <= defXRP) + return defXRP; + return a + XRP(1000); + } + auto const defIOU = STAmount{a.issue(), 30000}; + if (a <= defIOU) + return defIOU; + return a + STAmount{a.issue(), 1000}; + }; + auto const toFund1 = tofund(asset1); + auto const toFund2 = tofund(asset2); + BEAST_EXPECT(asset1 <= toFund1 && asset2 <= toFund2); + + if (!asset1.native() && !asset2.native()) + fund(env, gw, {alice, carol}, {toFund1, toFund2}, Fund::All); + else if (asset1.native()) + fund(env, gw, {alice, carol}, toFund1, {toFund2}, Fund::All); + else if (asset2.native()) + fund(env, gw, {alice, carol}, toFund2, {toFund1}, Fund::All); + + AMM ammAlice( + env, alice, asset1, asset2, CreateArg{.log = false, .tfee = 0}); + + Account const owner{"owner"}; + env.fund(XRP(1000000), owner); + + Vault vault{env}; + auto [tx, k] = + vault.create({.owner = owner, .asset = ammAlice.lptIssue()}); + env(tx, ter{tecWRONG_ASSET}); + env.close(); + } } void @@ -1065,7 +1117,7 @@ class Vault_test : public beast::unit_test::suite }); { - testcase("MPT maximum asset recursion"); + testcase("MPT shares to a vault"); Env env{*this, supported_amendments() | featureSingleAssetVault}; Account owner{"owner"}; @@ -1074,119 +1126,27 @@ class Vault_test : public beast::unit_test::suite env.close(); Vault vault{env}; - struct VaultInfo - { - Keylet keylet; - PrettyAsset shares; - PrettyAsset assets; - VaultInfo( - Keylet const& keylet_, - PrettyAsset const& shares_, - PrettyAsset const& assets_) - : keylet(keylet_), shares(shares_), assets(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))); + auto [tx1, k1] = vault.create({.owner = owner, .asset = asset}); + env(tx1); + env.close(); - 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(); + auto const shares = [&env, keylet = k1, this]() -> Asset { + auto const vault = env.le(keylet); + BEAST_EXPECT(vault != nullptr); + return MPTIssue(vault->at(sfShareMPTID)); + }(); - 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{tecLIMIT_EXCEEDED}); // 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})); - } + auto [tx2, k2] = vault.create({.owner = owner, .asset = shares}); + env(tx2, ter{tecWRONG_ASSET}); + env.close(); } } diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 1c228b2819..024455aff7 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -110,16 +110,13 @@ VaultCreate::preclaim(PreclaimContext const& ctx) return tecNO_AUTH; } - // Check for excessive vault shares recursion, which is reported by - // requireAuth as tecLIMIT_EXCEEDED. 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, or hold shares. - if (asset.holds()) + // Check for pseudo-account issuers - we do not want a vault to hold such + // assets (e.g. MPT shares to other vaults or AMM LPTokens) as they would be + // impossible to clawback (should the need arise) + if (!asset.native()) { - auto const mptIssue = asset.get(); - if (auto const ter = requireAuth(ctx.view, mptIssue, account, 1); - ter == tecLIMIT_EXCEEDED) - return ter; + if (isPseudoAccount(ctx.view, asset.getIssuer())) + return tecWRONG_ASSET; } // Cannot create Vault for an Asset frozen for the vault owner diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index da42998c42..dbdabccf37 100644 --- a/src/xrpld/ledger/View.h +++ b/src/xrpld/ledger/View.h @@ -156,10 +156,9 @@ isFrozen( int depth = 0); /** - * isFrozen check is recursive for MPT shares in a vault, descending to assets - * in the vault. These assets could be themselves MPT shares in another vault. - * For this reason we limit depth of check, up to maxAssetCheckDepth. If this - * depth is exceeded, we return true (i.e. the asset is considered frozen). + * isFrozen check is recursive for MPT shares in a vault, descending to + * assets in the vault, up to maxAssetCheckDepth recursion depth. This is + * purely defensive, as we currently do not allow such vaults to be created. */ [[nodiscard]] inline bool isFrozen( @@ -718,11 +717,8 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account); * doApply. This will ensure that any expired credentials are deleted. * * requireAuth check is recursive for MPT shares in a vault, descending to - * assets in the vault. These assets could be themselves MPT shares in another - * vault. For this reason we limit depth of check, up to maxAssetCheckDepth. - * This function will return tecLIMIT_EXCEEDED if maximum depth is exceeded. - * Transaction VaultCreate checks for this error code, to prevent such vaults - * being created. + * assets in the vault, up to maxAssetCheckDepth recursion depth. This is + * purely defensive, as we currently do not allow such vaults to be created. */ [[nodiscard]] TER requireAuth( diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index cd2281ffde..22d5d0d893 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -328,7 +328,7 @@ isVaultPseudoAccountFrozen( return false; // not a Vault pseudo-account, common case if (depth >= maxAssetCheckDepth) - return true; // fail at maximum 2^maxAssetCheckDepth checks + return true; // LCOV_EXCL_LINE auto const vault = view.read(keylet::vault(mptIssuer->getFieldH256(sfVaultID))); @@ -2298,7 +2298,7 @@ requireAuth( return tefINTERNAL; // LCOV_EXCL_LINE if (depth >= maxAssetCheckDepth) - return tecLIMIT_EXCEEDED; // VaultCreate looks for this code + return tecINTERNAL; // LCOV_EXCL_LINE auto const asset = sleVault->at(sfAsset); if (auto const err = std::visit(