From 2b86a1a5577c9eb7599b802beca44837a93aa737 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 1 Apr 2025 13:39:26 +0100 Subject: [PATCH] Enforce Destination checks on VaultWithdraw --- src/test/app/Vault_test.cpp | 130 +++++++++++++++++----- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 54 +++++---- 2 files changed, 138 insertions(+), 46 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 435da8021a..063508f80e 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -68,6 +68,7 @@ class Vault_test : public beast::unit_test::suite 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}); @@ -75,6 +76,15 @@ class Vault_test : public beast::unit_test::suite env.close(); BEAST_EXPECT(env.le(keylet)); + // 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(fset(alice, asfDepositAuth)); + env(fset(dave, asfRequireDest)); + env.close(); + { testcase(prefix + " fail to set negative maximum"); auto tx = vault.set({.owner = owner, .id = keylet.key}); @@ -317,11 +327,57 @@ class Vault_test : public beast::unit_test::suite } { - testcase(prefix + " withdraw non-zero assets"); + testcase( + prefix + " fail to withdraw to 3rd party lsfDepositAuth"); auto tx = vault.withdraw( {.depositor = depositor, .id = keylet.key, - .amount = asset(200)}); + .amount = asset(100)}); + tx[sfDestination] = alice.human(); + env(tx, ter{tecNO_PERMISSION}); + } + + if (!asset.raw().native()) + { + testcase( + prefix + " fail to withdraw to 3rd party no authorization"); + auto tx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(100)}); + tx[sfDestination] = erin.human(); + env(tx, + ter{asset.raw().holds() ? tecNO_LINE : tecNO_AUTH}); + } + + { + testcase( + prefix + + " fail to withdraw to 3rd party lsfRequireDestTag"); + auto tx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(100)}); + tx[sfDestination] = dave.human(); + env(tx, ter{tecDST_TAG_NEEDED}); + } + + { + testcase(prefix + " withdraw to authorized 3rd party"); + auto tx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(100)}); + tx[sfDestination] = charlie.human(); + env(tx); + } + + { + testcase(prefix + " withdraw remaining assets"); + auto tx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(100)}); env(tx); } @@ -345,40 +401,57 @@ class Vault_test : public beast::unit_test::suite } }; - auto testCases = - [this, &testSequence]( - std::string prefix, - std::function - setup) { - Env env{*this}; - Account issuer{"issuer"}; - Account owner{"owner"}; - Account depositor{"depositor"}; - Vault vault{env}; - env.fund(XRP(1000), issuer, owner, depositor); - env.close(); - env(fset(issuer, asfAllowTrustLineClawback)); - env.close(); - env.require(flags(issuer, asfAllowTrustLineClawback)); + auto testCases = [this, &testSequence]( + std::string prefix, + std::function setup) { + Env env{*this}; + 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.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env(fset(issuer, asfRequireAuth)); + env.close(); + env.require(flags(issuer, asfAllowTrustLineClawback)); + env.require(flags(issuer, asfRequireAuth)); - PrettyAsset asset = setup(env, issuer, depositor); - testSequence( - prefix, env, issuer, owner, depositor, vault, asset); - }; + PrettyAsset asset = setup(env, issuer, owner, depositor, charlie); + testSequence( + prefix, env, issuer, owner, depositor, charlie, vault, asset); + }; testCases( "XRP", - [](Env& env, Account const& issuer, Account const& depositor) - -> PrettyAsset { 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( "IOU", [](Env& env, Account const& issuer, - Account const& depositor) -> Asset { + Account const& owner, + Account const& depositor, + Account const& charlie) -> Asset { PrettyAsset asset = issuer["IOU"]; - env.trust(asset(1000), depositor); + env(trust(owner, asset(1000))); + env(trust(depositor, asset(1000))); + env(trust(charlie, asset(1000))); + env(trust(issuer, asset(0), owner, tfSetfAuth)); + env(trust(issuer, asset(0), depositor, tfSetfAuth)); + env(trust(issuer, asset(0), charlie, tfSetfAuth)); env(pay(issuer, depositor, asset(1000))); env.close(); return asset; @@ -388,13 +461,16 @@ class Vault_test : public beast::unit_test::suite "MPT", [](Env& env, Account const& issuer, - Account const& depositor) -> Asset { + Account const& owner, + Account const& depositor, + Account const& charlie) -> Asset { MPTTester mptt{env, issuer, mptInitNoFund}; mptt.create( {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); PrettyAsset asset = mptt.issuanceID(); mptt.authorize({.account = depositor}); + mptt.authorize({.account = charlie}); env(pay(issuer, depositor, asset(1000))); env.close(); return asset; diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 6734ec9dde..f3fd798af9 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -65,24 +65,16 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (!vault) return tecNO_ENTRY; - if (ctx.tx.isFieldPresent(sfDestination)) - { - auto const dstAccountID = ctx.tx.getAccountID(sfDestination); - if (auto const sleDst = ctx.view.read(keylet::account(dstAccountID)); - sleDst == nullptr) - return tecNO_DST; - } - - // Enforce valid withdrawal policy - if (vault->at(sfWithdrawalPolicy) != vaultStrategyFirstComeFirstServe) - return tefINTERNAL; - auto const assets = ctx.tx[sfAmount]; auto const asset = vault->at(sfAsset); auto const share = vault->at(sfShareMPTID); if (assets.asset() != asset && assets.asset() != share) return tecWRONG_ASSET; + // Enforce valid withdrawal policy + if (vault->at(sfWithdrawalPolicy) != vaultStrategyFirstComeFirstServe) + return tefINTERNAL; + auto const account = ctx.tx[sfAccount]; auto const dstAcct = [&]() -> AccountID { if (ctx.tx.isFieldPresent(sfDestination)) @@ -90,14 +82,38 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) return account; }(); - if (account != dstAcct && assets.holds()) + // Withdrawal to a 3rd party destination account is essentially a transfer, + // via shares in the vault. Enforce all the usual asset transfer checks. + if (account != dstAcct) { - auto mptID = assets.get().getMptID(); - auto issuance = ctx.view.read(keylet::mptIssuance(mptID)); - if (!issuance) - return tecOBJECT_NOT_FOUND; - if ((issuance->getFlags() & lsfMPTCanTransfer) == 0) - return tecNO_AUTH; + auto const sleDst = ctx.view.read(keylet::account(dstAcct)); + if (sleDst == nullptr) + return tecNO_DST; + + if (sleDst->getFlags() & lsfRequireDestTag) + return tecDST_TAG_NEEDED; // Cannot send without a tag + + if (sleDst->getFlags() & lsfDepositAuth) + { + if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account))) + return tecNO_PERMISSION; + } + + if (auto const ter = std::visit( + [&](TIss const& issue) -> TER { + return requireAuth(ctx.view, issue, dstAcct); + }, + asset.value()); + !isTesSuccess(ter)) + return ter; + + if (assets.holds()) + { + if (auto const ter = canTransfer( + ctx.view, assets.get(), account, dstAcct); + !isTesSuccess(ter)) + return ter; + } } // Cannot withdraw from a Vault an Asset frozen for the destination account