From d0bc792885c15d747a82023c2575eef2d6cd36a5 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 25 Jul 2025 14:53:25 +0100 Subject: [PATCH] VaultWithdraw destination account bugfix (#5572) --- .../xrpl/protocol/detail/transactions.macro | 1 + src/test/app/Vault_test.cpp | 205 +++++++++++++++++- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 36 ++- src/xrpld/ledger/View.h | 81 +++++-- src/xrpld/ledger/detail/View.cpp | 29 ++- 5 files changed, 311 insertions(+), 41 deletions(-) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 21bb81b3c..ea17dadb0 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -544,6 +544,7 @@ TRANSACTION(ttVAULT_WITHDRAW, 78, VaultWithdraw, Delegation::delegatable, ({ {sfVaultID, soeREQUIRED}, {sfAmount, soeREQUIRED, soeMPTSupported}, {sfDestination, soeOPTIONAL}, + {sfDestinationTag, soeOPTIONAL}, })) /** This transaction claws back tokens from a vault. */ diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 44ef31494..422503b8a 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -233,6 +233,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( @@ -1334,6 +1356,7 @@ class Vault_test : public beast::unit_test::suite struct CaseArgs { bool enableClawback = true; + bool requireAuth = true; }; auto testCase = [this]( @@ -1355,16 +1378,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(); @@ -1513,6 +1540,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, @@ -1800,6 +1921,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(); @@ -2115,6 +2237,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())); + 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, diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 7a8605cdb..09a9fd14e 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/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() ? 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; } diff --git a/src/xrpld/ledger/View.h b/src/xrpld/ledger/View.h index 9c644d429..9777c2a40 100644 --- a/src/xrpld/ledger/View.h +++ b/src/xrpld/ledger/View.h @@ -182,6 +182,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, @@ -1369,19 +1392,40 @@ trustTransferLockedBalance( return tesSUCCESS; } -/* 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. * @@ -1395,32 +1439,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( [&](TIss const& issue_) { - if constexpr (std::is_same_v) - return requireAuth(view, issue_, account); - else - return requireAuth(view, issue_, account, authType); + return requireAuth(view, issue_, account, authType); }, asset.value()); } diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 6233061e9..6b781ddbd 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -518,8 +518,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); } @@ -2381,15 +2381,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 @@ -2405,7 +2417,7 @@ requireAuth( ReadView const& view, MPTIssue const& mptIssue, AccountID const& account, - MPTAuthType authType, + AuthType authType, int depth) { auto const mptID = keylet::mptIssuance(mptIssue.getMptID()); @@ -2440,7 +2452,7 @@ requireAuth( if (auto const err = std::visit( [&](TIss const& issue) { if constexpr (std::is_same_v) - return requireAuth(view, issue, account); + return requireAuth(view, issue, account, authType); else return requireAuth( view, issue, account, authType, depth + 1); @@ -2455,7 +2467,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