Compare commits

...

7 Commits

Author SHA1 Message Date
Vito
5abb72ec0a test: Add withdrawal-to-issuer assertion in IOU global-freeze test
Mirror the MPT and trust-line-freeze variants: under global freeze the
redemption path (dstAcct == vaultAsset.getIssuer()) should still succeed.
2026-06-03 17:59:36 +02:00
Vito
42b7e858a4 test: Restore clawback-under-MPT-global-lock coverage
The previous edit dropped the assertion that clawback still works when
the MPT asset is globally locked. Restore it before the issuer-redemption
step, claw back 50 of the 100 deposited units, then withdraw the remaining
50 to the issuer so both behaviours are exercised in the same test.
2026-06-03 17:55:54 +02:00
Vito
3daf40c408 fix: clarifying comment 2026-06-03 17:43:30 +02:00
Vito
43fc095b1a fix: Use IgnoreFreeze in doApply for issuer-redemption withdrawals
Hoist dstAcct into doApply and use FreezeHandling::IgnoreFreeze for
accountHolds when dstAcct == vaultAsset.getIssuer() under fixCleanup3_2_0.
Without this the locked-share balance read as zero and the redemption
failed with tecINSUFFICIENT_FUNDS despite preclaim passing.

Update tests: issuer-redemption now succeeds end-to-end for both IOU and
MPT vaults.
2026-06-03 17:40:14 +02:00
Vito
45fa34de4b test: Add freeze-check tests for VaultWithdraw issuer guard
Verify the dstAcct != vaultAsset.getIssuer() guard introduced under
fixCleanup3_2_0: frozen vault trust line and MPT global lock both block
non-issuer withdrawals, while the issuer-destination path bypasses
preclaim's freeze checks (redemption path). Preclaim passes for the
issuer destination, but doApply::accountHolds(ZeroIfFrozen) still returns
0 for locked shares — a matching doApply fix is tracked via TODO comments.

Also updates expected error codes in existing freeze tests affected by
the new vault-sender and deep-frozen-destination checks.
2026-06-03 17:32:32 +02:00
Vito Tumas
250b0d2c25 Merge branch 'develop' into tapanito/vault-freeze-check 2026-06-02 18:22:08 +02:00
Vito
488e1be1dc fix: Check vault sender freeze and use checkDeepFrozen for destination
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.
2026-06-02 18:18:14 +02:00
2 changed files with 283 additions and 27 deletions

View File

@@ -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,14 +161,36 @@ 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 (dstAcct != vaultAsset.getIssuer())
{
if (auto const ret = checkFrozen(ctx.view, vaultAccount, vaultAsset))
return ret;
// Cannot return shares to the vault, if the underlying asset was frozen for
// the submitter
if (auto const ret = checkFrozen(ctx.view, account, Asset{vaultShare}))
return ret;
if (auto const ret = checkDeepFrozen(ctx.view, dstAcct, vaultAsset))
return ret;
// Cannot return shares to the vault, if the underlying asset was frozen for
// the submitter
if (auto const ret = checkFrozen(ctx.view, account, Asset{vaultShare}))
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
if (auto const ret = checkFrozen(ctx.view, account, Asset{vaultShare}))
return ret;
}
return tesSUCCESS;
}
@@ -254,8 +277,14 @@ VaultWithdraw::doApply()
return tecPATH_DRY;
}
if (accountHolds(
view(), accountID_, share, FreezeHandling::ZeroIfFrozen, AuthHandling::IgnoreAuth, j_) <
auto const dstAcct = ctx_.tx[~sfDestination].value_or(accountID_);
bool const redeemingToIssuer =
view().rules().enabled(fixCleanup3_2_0) && dstAcct == vaultAsset.getIssuer();
auto const freezeHandling =
redeemingToIssuer ? FreezeHandling::IgnoreFreeze : FreezeHandling::ZeroIfFrozen;
// Share freeze checks are transitive. We skip them when withdrawing to the issuer alltogether.
if (accountHolds(view(), accountID_, share, freezeHandling, AuthHandling::IgnoreAuth, j_) <
sharesRedeemed)
{
JLOG(j_.debug()) << "VaultWithdraw: account doesn't hold enough shares";
@@ -358,8 +387,6 @@ VaultWithdraw::doApply()
// else quietly ignore, account balance is not zero
}
auto const dstAcct = ctx_.tx[~sfDestination].value_or(accountID_);
associateAsset(*vault, vaultAsset);
return doWithdraw(

View File

@@ -1701,16 +1701,21 @@ class Vault_test : public beast::unit_test::Suite
tx = vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(100)});
env(tx, Ter(tecLOCKED));
tx[sfDestination] = issuer.human();
env(tx, Ter(tecLOCKED));
// Clawback is still permitted, even with global lock
// Clawback is still permitted, even with global lock.
tx = vault.clawback(
{.issuer = issuer, .id = keylet.key, .holder = depositor, .amount = asset(0)});
{.issuer = issuer, .id = keylet.key, .holder = depositor, .amount = asset(50)});
env(tx);
env.close();
// Clawback removed shares MPToken
// Redemption to the issuer bypasses freeze checks end-to-end:
// preclaim's issuer guard skips all three checks, and doApply uses
// FreezeHandling::IgnoreFreeze for the accountHolds balance check.
tx = vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(50)});
tx[sfDestination] = issuer.human();
env(tx);
env.close();
// Withdrawal burned remaining depositor shares — MPToken is removed.
auto const mptSle = env.le(keylet::mptoken(share, depositor.id()));
BEAST_EXPECT(mptSle == nullptr);
@@ -2791,13 +2796,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 +2829,153 @@ 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,
Account const& owner,
Account const& issuer,
Account const& charlie,
auto vaultAccount,
Vault& vault,
PrettyAsset const& asset,
auto issuanceId) {
testcase("IOU frozen vault trust line, withdrawal to issuer is exempt");
// When the vault's trust line is frozen, withdrawals to any
// non-issuer destination are blocked by checkFrozen(vaultAccount).
// Withdrawals whose destination IS the IOU issuer bypass that
// check entirely (redemption path).
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();
Asset const share = Asset(issuanceId(keylet));
// Freeze the trust line to the vault pseudo-account
auto trustSet = [&, account = vaultAccount(keylet)]() {
json::Value jv;
jv[jss::Account] = issuer.human();
{
auto& ja = jv[jss::LimitAmount] =
asset(0).value().getJson(JsonOptions::Values::None);
ja[jss::issuer] = toBase58(account);
}
jv[jss::TransactionType] = jss::TrustSet;
jv[jss::Flags] = tfSetFreeze;
return jv;
}();
env(trustSet);
env.close();
{
// Non-issuer destinations are blocked
auto t =
vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)});
env(t, Ter{tecFROZEN});
t[sfDestination] = charlie.human();
env(t, Ter{tecFROZEN});
env.close();
}
{
// Withdrawal to the IOU issuer succeeds end-to-end: the issuer
// guard skips all preclaim checks, and doApply uses
// FreezeHandling::IgnoreFreeze so accountHolds returns the
// actual balance rather than zero.
auto t =
vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(50)});
t[sfDestination] = issuer.human();
env(t);
env.close();
}
// vault now has 50 assets / owner holds 50'000'000 shares
// Clear freeze and drain what remains.
trustSet[jss::Flags] = tfClearFreeze;
env(trustSet);
env.close();
env(vault.withdraw(
{.depositor = owner, .id = keylet.key, .amount = share(50'000'000)}));
env(vault.del({.owner = owner, .id = keylet.key}));
env.close();
});
testCase(
[&, this](
Env& env,
@@ -2913,10 +3068,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 +3418,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 +3453,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 +3525,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});
@@ -3314,6 +3538,11 @@ class Vault_test : public beast::unit_test::Suite
env(tx, Ter{tecFROZEN});
env.close();
// Withdrawal to the IOU issuer succeeds (redemption path)
tx[sfDestination] = issuer.human();
env(tx);
env.close();
// Cannot deposit some more
tx = vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(10)});