From 488e1be1dc53554b28d746e4719d19dd29b5631e Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 2 Jun 2026 18:14:11 +0200 Subject: [PATCH] fix: Check vault sender freeze and use checkDeepFrozen for destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under fixCleanup3_2_0, VaultWithdraw::preclaim now explicitly checks that the vault pseudo-account (sender) is not frozen, and replaces the destination checkFrozen with checkDeepFrozen — a frozen account can still receive assets, only a deep-frozen one cannot. Pre-amendment behaviour is preserved via the else branch. Tests are updated to reflect the changed error codes and new pre/post amendment cases are added for the vault-account and 3rd-party freeze scenarios. --- .../tx/transactors/vault/VaultWithdraw.cpp | 24 ++- src/test/app/Vault_test.cpp | 158 +++++++++++++++++- 2 files changed, 169 insertions(+), 13 deletions(-) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index cfcf79fdba..11c5feff8d 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -78,12 +78,13 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) auto const& vaultAccount = vault->at(sfAccount); auto const& account = ctx.tx[sfAccount]; auto const& dstAcct = ctx.tx[~sfDestination].value_or(account); + + auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); // Post-fixCleanup3_2_0: withdraw is a recovery path that bypasses the // lsfMPTCanTransfer flag check, so an issuer cannot trap depositor funds. // Other transferability checks (IOU NoRipple, freeze, requireAuth) still // apply. - auto const waive = ctx.view.rules().enabled(fixCleanup3_2_0) ? WaiveMPTCanTransfer::Yes - : WaiveMPTCanTransfer::No; + auto const waive = fix320Enabled ? WaiveMPTCanTransfer::Yes : WaiveMPTCanTransfer::No; if (auto ter = canTransfer(ctx.view, vaultAsset, vaultAccount, dstAcct, waive); !isTesSuccess(ter)) { @@ -160,9 +161,22 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) 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 (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset)) - return ret; + // Pre-fixCleanup3_2_0: any freeze on the destination blocked withdrawal + // even though frozen accounts can still receive. Post-amendment: + // - Frozen pseudo-account (vault) cannot send assets. + // - Only deep-frozen destinations (which cannot receive at all) are blocked. + if (fix320Enabled) + { + if (auto const ret = checkFrozen(ctx.view, vaultAccount, vaultAsset)) + return ret; + if (auto const ret = checkDeepFrozen(ctx.view, dstAcct, vaultAsset)) + return ret; + } + else + { + if (auto const ret = checkFrozen(ctx.view, dstAcct, vaultAsset)) + return ret; + } // Cannot return shares to the vault, if the underlying asset was frozen for // the submitter diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index bf707afae9..b9cfc68336 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -2791,13 +2791,16 @@ class Vault_test : public beast::unit_test::Suite } { + // Post-fixCleanup3_2_0: the frozen pseudo-account is checked + // directly as a sender, so the error is tecFROZEN rather than + // the transitive tecLOCKED from the share freeze check. auto tx = vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(100)}); - env(tx, Ter{tecLOCKED}); + env(tx, Ter{tecFROZEN}); // also when trying to withdraw to a 3rd party tx[sfDestination] = charlie.human(); - env(tx, Ter{tecLOCKED}); + env(tx, Ter{tecFROZEN}); env.close(); } @@ -2821,6 +2824,76 @@ class Vault_test : public beast::unit_test::Suite env.close(); }); + { + testcase("IOU frozen trust line to vault account: pre-fixCleanup3_2_0"); + + // Pre-amendment: there is no direct pseudo-account sender check. + // The frozen trust line is caught transitively via the share freeze + // (isVaultPseudoAccountFrozen), so withdrawals fail with tecLOCKED + // rather than tecFROZEN. + Env env{*this, testableAmendments() - fixCleanup3_2_0}; + Account const owner{"owner"}; + Account const issuer{"issuer"}; + Account const charlie{"charlie"}; + Vault const vault{env}; + env.fund(XRP(1000), issuer, owner, charlie); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1000), owner); + env(pay(issuer, owner, asset(200))); + env.trust(asset(1000), charlie); + env.close(); + + 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(); + + // Freeze the trust line to the vault pseudo-account + auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); + auto trustSet = [&]() { + json::Value jv; + jv[jss::Account] = issuer.human(); + { + auto& ja = jv[jss::LimitAmount] = + asset(0).value().getJson(JsonOptions::Values::None); + ja[jss::issuer] = toBase58(vaultAcct.id()); + } + jv[jss::TransactionType] = jss::TrustSet; + jv[jss::Flags] = tfSetFreeze; + return jv; + }(); + env(trustSet); + env.close(); + + { + auto t = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(50)}); + // Pre-amendment: caught transitively via share freeze. + env(t, Ter{tecLOCKED}); + + t[sfDestination] = charlie.human(); + env(t, Ter{tecLOCKED}); + env.close(); + } + + // Clear freeze and clean up + trustSet[jss::Flags] = tfClearFreeze; + env(trustSet); + env.close(); + + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(0)})); + env.close(); + + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + } + testCase( [&, this]( Env& env, @@ -2913,10 +2986,13 @@ class Vault_test : public beast::unit_test::Suite env(trust(issuer, asset(0), owner, tfSetFreeze)); env.close(); - // Cannot withdraw + // Post-fixCleanup3_2_0: destination check uses checkDeepFrozen; a + // regularly-frozen owner can still receive, so withdrawal is not + // blocked by the destination check. The share lock (transitive + // from the frozen underlying IOU) still prevents withdrawal. auto const withdraw = vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - env(withdraw, Ter{tecFROZEN}); + env(withdraw, Ter{tecLOCKED}); // Cannot withdraw to 3rd party env(withdrawToCharlie, Ter{tecLOCKED}); @@ -3260,20 +3336,33 @@ class Vault_test : public beast::unit_test::Suite }(keylet); env(withdrawToCharlie); - // Freeze the 3rd party + // Freeze the 3rd party (regular freeze only) env(trust(issuer, asset(0), charlie, tfSetFreeze)); env.close(); - // Can withdraw + // Can withdraw to self auto const withdraw = vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); env(withdraw); env.close(); - // Cannot withdraw to 3rd party + // Post-fixCleanup3_2_0: a regularly-frozen destination can still + // receive, so withdrawal to charlie is not blocked. + env(withdrawToCharlie); + env.close(); + + // Deep-freeze charlie: the destination can neither send nor receive. + env(trust(issuer, asset(0), charlie, tfSetDeepFreeze)); + env.close(); + + // Cannot withdraw to deep-frozen 3rd party. env(withdrawToCharlie, Ter{tecFROZEN}); env.close(); + // Clear the freeze so the vault can be cleaned up. + env(trust(issuer, asset(0), charlie, tfClearFreeze | tfClearDeepFreeze)); + env.close(); + env(vault.clawback( {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(0)})); env.close(); @@ -3282,6 +3371,56 @@ class Vault_test : public beast::unit_test::Suite env.close(); }); + { + testcase("IOU frozen trust line to 3rd party: pre-fixCleanup3_2_0"); + + // Pre-amendment: checkFrozen was used for the destination; a + // regularly-frozen 3rd party could not receive vault assets. + Env env{*this, testableAmendments() - fixCleanup3_2_0}; + Account const owner{"owner"}; + Account const issuer{"issuer"}; + Account const charlie{"charlie"}; + Vault vault{env}; + env.fund(XRP(1000), issuer, owner, charlie); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1000), owner); + env(pay(issuer, owner, asset(200))); + env.trust(asset(1000), charlie); + env.close(); + + 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(); + + auto const withdrawToCharlie = [&]() { + auto t = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); + t[sfDestination] = charlie.human(); + return t; + }(); + env(withdrawToCharlie); + + // Freeze the 3rd party + env(trust(issuer, asset(0), charlie, tfSetFreeze)); + env.close(); + + // Pre-amendment: regular freeze on the destination blocks withdrawal. + env(withdrawToCharlie, Ter{tecFROZEN}); + + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(0)})); + env.close(); + + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + } + testCase([&, this]( Env& env, Account const& owner, @@ -3304,7 +3443,10 @@ class Vault_test : public beast::unit_test::Suite env.close(); { - // Cannot withdraw + // Post-fixCleanup3_2_0: checkFrozen(vaultAccount, vaultAsset) + // fires first. isFrozen checks lsfGlobalFreeze on the issuer + // unconditionally, so the vault pseudo-account (sender) is + // blocked before any destination or share check is reached. auto tx = vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); env(tx, Ter{tecFROZEN});