ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-31 19:10:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Unify AMM Deposit/Withdraw freeze checks with checkDepositFreeze/checkWithdrawFreeze

Browse Source 
Migrate AMMDeposit and AMMWithdraw to use the unified freeze helpers
introduced for Vault and LoanBroker, gated on fixCleanup3_3_0.

AMMDeposit: checkDepositFreeze for both pool assets replaces the
two-layer checkAsset + checkAmount freeze logic. If either pool asset
is frozen (depositor or AMM account), all deposits are blocked.

AMMWithdraw: checkWithdrawFreeze per withdrawn amount replaces
checkFrozen(AMM) + checkIndividualFrozen(user). Regular IOU freeze
no longer blocks self-withdrawal; only deep freeze does. The issuer
exemption allows the token issuer to withdraw from a frozen pool;
issuerFreezeHandling() ensures doApply uses IgnoreFreeze so pool
math does not divide by zero.

Rename checkWithdrawFreezes -> checkWithdrawFreeze (singular) and
update all callers. Document both helpers with full freeze semantics.
This commit is contained in:
Vito
2026-06-18 19:41:35 +02:00
parent 34b4bb43af
commit 7b63288497
8 changed files with 242 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
include/xrpl/ledger/helpers/TokenHelpers.h
View File

@@ -132,18 +132,32 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, MPTIssue const&
checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& asset);
/**
* Checks freeze compliance for a pseudo-account withdrawal to a non-issuer destination.
* Checks freeze compliance for withdrawing an asset from a pseudo-account
* (e.g. Vault, AMM, LoanBroker) to a destination account.
*
* Returns tesSUCCESS immediately when dstAcct is the asset issuer — redemption
* to the issuer bypasses all freeze checks.
* Asserts that sourceAcct is a pseudo-account.
*
* Otherwise checks:
* - The source pseudo-account is not frozen for the asset (checkFrozen).
* - The submitting account is not individually frozen for the asset (checkFrozen).
* - The destination is not deep-frozen for the asset (checkDeepFrozen).
* Issuer exemption: returns tesSUCCESS immediately when dstAcct is the asset
* issuer — the issuer can always receive their own token, even when the pool
* is frozen. Callers that need to block withdrawals from a frozen pool even
* for the issuer (e.g. because the pool math cannot handle it) must check
* checkFrozen(sourceAcct, asset) separately before calling this function.
*
* Otherwise checks, in order:
* 1. checkFrozen(sourceAcct, asset) — the pseudo-account's trustline /
* global freeze must not block sending.
* 2. checkFrozen(submitterAcct, asset) — skipped when submitter == dst
* (self-withdrawal); a regular freeze should not prevent recovering
* one's own funds.
* 3. checkDeepFrozen(dstAcct, asset) — the destination must not be
* deep-frozen (cannot receive under any circumstance).
*
* For IOUs a regular individual freeze on the withdrawer does NOT block
* self-withdrawal; only deep freeze does. For MPTs "locked" is equivalent
* to deep-frozen, so locked MPT holders are always blocked.
*/
[[nodiscard]] TER
checkWithdrawFreezes(
checkWithdrawFreeze(
ReadView const& view,
AccountID const& sourceAcct,
AccountID const& submitterAcct,
@@ -151,14 +165,23 @@ checkWithdrawFreezes(
Asset const& asset);
/**
* Checks freeze compliance for a pseudo-account deposit.
* - The source pseudo-account is not deep frozen for the asset
* - The submitting account is not individually frozen for the asset
* Checks freeze compliance for depositing an asset into a pseudo-account
* (e.g. Vault, AMM, LoanBroker).
*
* Asserts that dstAcct is a pseudo-account.
*
* Checks, in order:
* 1. checkFrozen(srcAcct, asset) — the depositor must not be frozen
* (global or individual) for the asset.
* 2. checkFrozen(dstAcct, asset) — the pseudo-account must not be
* frozen for the asset. Unlike regular accounts, pseudo-accounts
* cannot receive deposits under a regular freeze because the
* deposited funds could not later be withdrawn.
*/
[[nodiscard]] TER
checkDepositFreeze(
ReadView const& view,
AccountID const& sourceAcct,
AccountID const& srcAcct,
AccountID const& dstAcct,
Asset const& asset);

5
include/xrpl/tx/transactors/dex/AMMWithdraw.h
View File

@@ -159,6 +159,11 @@ public:
beast::Journal const& journal);
private:
/** Returns IgnoreFreeze when the withdrawer is the issuer of a pool
* asset (post-fixCleanup3_3_0), ZeroIfFrozen otherwise. */
[[nodiscard]] FreezeHandling
issuerFreezeHandling() const;
std::pair<TER, bool>
applyGuts(Sandbox& view);

2
src/libxrpl/ledger/helpers/TokenHelpers.cpp
View File

@@ -165,7 +165,7 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& ass
}
TER
checkWithdrawFreezes(
checkWithdrawFreeze(
ReadView const& view,
AccountID const& srcAcct,
AccountID const& submitterAcct,

68
src/libxrpl/tx/transactors/dex/AMMDeposit.cpp
View File

@@ -251,7 +251,36 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
: tecUNFUNDED_AMM;
};
if (ctx.view.rules().enabled(featureAMMClawback))
auto const amount = ctx.tx[~sfAmount];
auto const amount2 = ctx.tx[~sfAmount2];
auto const ammAccountID = ammSle->getAccountID(sfAccount);
if (ctx.view.rules().enabled(fixCleanup3_3_0))
{
// Unified deposit freeze check for both pool assets.
// AMMDeposit is not allowed if either asset is frozen.
auto checkAsset = [&](Asset const& asset) -> TER {
if (auto const ter = requireAuth(ctx.view, asset, accountID, AuthType::WeakAuth))
{
JLOG(ctx.j.debug()) << "AMM Deposit: account is not authorized, " << asset;
return ter;
}
if (auto const ter = checkDepositFreeze(ctx.view, accountID, ammAccountID, asset))
{
JLOG(ctx.j.debug())
<< "AMM Deposit: frozen, " << to_string(accountID) << " " << to_string(asset);
return ter;
}
return tesSUCCESS;
};
if (auto const ter = checkAsset(ctx.tx[sfAsset]))
return ter;
if (auto const ter = checkAsset(ctx.tx[sfAsset2]))
return ter;
}
else if (ctx.view.rules().enabled(featureAMMClawback))
{
// Check if either of the assets is frozen, AMMDeposit is not allowed
// if either asset is frozen
@@ -283,10 +312,6 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
return ter;
}
auto const amount = ctx.tx[~sfAmount];
auto const amount2 = ctx.tx[~sfAmount2];
auto const ammAccountID = ammSle->getAccountID(sfAccount);
auto checkAmount = [&](std::optional<STAmount> const& amount, bool checkBalance) -> TER {
if (amount)
{
@@ -301,21 +326,26 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
return ter;
// LCOV_EXCL_STOP
}
// AMM account or currency frozen
if (auto const ter = checkFrozen(ctx.view, ammAccountID, amount->asset());
!isTesSuccess(ter))
if (!ctx.view.rules().enabled(fixCleanup3_3_0))
{
JLOG(ctx.j.debug()) << "AMM Deposit: AMM account or currency is frozen or locked, "
<< to_string(accountID);
return ter;
}
// Account frozen
if (auto const ter = checkIndividualFrozen(ctx.view, accountID, amount->asset());
!isTesSuccess(ter))
{
JLOG(ctx.j.debug()) << "AMM Deposit: account is frozen or locked, "
<< to_string(accountID) << " " << to_string(amount->asset());
return ter;
// AMM account or currency frozen
if (auto const ter = checkFrozen(ctx.view, ammAccountID, amount->asset());
!isTesSuccess(ter))
{
JLOG(ctx.j.debug())
<< "AMM Deposit: AMM account or currency is frozen or locked, "
<< to_string(accountID);
return ter;
}
// Account frozen
if (auto const ter = checkIndividualFrozen(ctx.view, accountID, amount->asset());
!isTesSuccess(ter))
{
JLOG(ctx.j.debug())
<< "AMM Deposit: account is frozen or locked, " << to_string(accountID)
<< " " << to_string(amount->asset());
return ter;
}
}
if (checkBalance)
{

65
src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp
View File

@@ -238,21 +238,36 @@ AMMWithdraw::preclaim(PreclaimContext const& ctx)
<< "AMM Withdraw: account is not authorized, " << amount->asset();
return ter;
}
// AMM account or currency frozen
if (auto const ter = checkFrozen(ctx.view, ammAccountID, amount->asset());
!isTesSuccess(ter))
if (ctx.view.rules().enabled(fixCleanup3_3_0))
{
JLOG(ctx.j.debug()) << "AMM Withdraw: AMM account or currency is frozen or locked, "
<< to_string(accountID);
return ter;
if (auto const ret = checkWithdrawFreeze(
ctx.view, ammAccountID, accountID, accountID, amount->asset()))
{
JLOG(ctx.j.debug()) << "AMM Withdraw: frozen, " << to_string(accountID) << " "
<< to_string(amount->asset());
return ret;
}
}
// Account frozen
if (auto const ter = checkIndividualFrozen(ctx.view, accountID, amount->asset());
!isTesSuccess(ter))
else
{
JLOG(ctx.j.debug()) << "AMM Withdraw: account is frozen or locked, "
<< to_string(accountID) << " " << to_string(amount->asset());
return ter;
// AMM account or currency frozen
if (auto const ter = checkFrozen(ctx.view, ammAccountID, amount->asset());
!isTesSuccess(ter))
{
JLOG(ctx.j.debug())
<< "AMM Withdraw: AMM account or currency is frozen or locked, "
<< to_string(accountID);
return ter;
}
// Account frozen
if (auto const ter = checkIndividualFrozen(ctx.view, accountID, amount->asset());
!isTesSuccess(ter))
{
JLOG(ctx.j.debug())
<< "AMM Withdraw: account is frozen or locked, " << to_string(accountID)
<< " " << to_string(amount->asset());
return ter;
}
}
}
return tesSUCCESS;
@@ -302,6 +317,24 @@ AMMWithdraw::preclaim(PreclaimContext const& ctx)
return tesSUCCESS;
}
FreezeHandling
AMMWithdraw::issuerFreezeHandling() const
{
// When the withdrawer is the issuer of a pool asset, the issuer can
// always receive their own token — even when the pool is frozen.
// Use IgnoreFreeze so ammHolds returns real balances instead of zero.
if (ctx_.view().rules().enabled(fixCleanup3_3_0))
{
auto const asset1 = Asset{ctx_.tx[sfAsset]};
auto const asset2 = Asset{ctx_.tx[sfAsset2]};
if (!asset1.native() && accountID_ == asset1.getIssuer())
return FreezeHandling::IgnoreFreeze;
if (!asset2.native() && accountID_ == asset2.getIssuer())
return FreezeHandling::IgnoreFreeze;
}
return FreezeHandling::ZeroIfFrozen;
}
std::pair<TER, bool>
AMMWithdraw::applyGuts(Sandbox& sb)
{
@@ -329,12 +362,14 @@ AMMWithdraw::applyGuts(Sandbox& sb)
auto const tfee = getTradingFee(ctx_.view(), *ammSle, accountID_);
auto const freezeHandling = issuerFreezeHandling();
auto const expected = ammHolds(
sb,
*ammSle,
amount ? amount->asset() : std::optional<Asset>{},
amount2 ? amount2->asset() : std::optional<Asset>{},
FreezeHandling::ZeroIfFrozen,
freezeHandling,
AuthHandling::ZeroIfUnauthorized,
ctx_.journal);
if (!expected)
@@ -459,7 +494,7 @@ AMMWithdraw::withdraw(
lpTokensAMMBalance,
lpTokensWithdraw,
tfee,
FreezeHandling::ZeroIfFrozen,
issuerFreezeHandling(),
AuthHandling::ZeroIfUnauthorized,
isWithdrawAll(ctx_.tx),
preFeeBalance_,
@@ -746,7 +781,7 @@ AMMWithdraw::equalWithdrawTokens(
lpTokens,
lpTokensWithdraw,
tfee,
FreezeHandling::ZeroIfFrozen,
issuerFreezeHandling(),
AuthHandling::ZeroIfUnauthorized,
isWithdrawAll(ctx_.tx),
preFeeBalance_,

2
src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp
View File

@@ -128,7 +128,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
if (ctx.view.rules().enabled(fixCleanup3_3_0))
{
if (auto const ret =
checkWithdrawFreezes(ctx.view, pseudoAccountID, account, dstAcct, vaultAsset))
checkWithdrawFreeze(ctx.view, pseudoAccountID, account, dstAcct, vaultAsset))
return ret;
}
else

2
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp
View File

@@ -170,7 +170,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
// The only way shares become locked is transitively via the
// underlying asset, which checkWithdrawFreezes already covers.
if (auto const ret =
checkWithdrawFreezes(ctx.view, vaultAccount, account, dstAcct, vaultAsset))
checkWithdrawFreeze(ctx.view, vaultAccount, account, dstAcct, vaultAsset))
return ret;
}
else

152
src/test/app/AMM_test.cpp
View File

@@ -85,6 +85,21 @@ private:
return jtx::testableAmendments() - featureSingleAssetVault - featureLendingProtocol;
}
// All 2^N permutations of testableAmendments() with each subset of the
// given features excluded.
static std::vector<FeatureBitset>
amendmentCombinations(std::initializer_list<uint256> features)
{
std::vector<FeatureBitset> result{testableAmendments()};
for (auto const& f : features)
{
auto const n = result.size();
for (std::size_t i = 0; i < n; ++i)
result.push_back(result[i] - f);
}
return result;
}
void
testInstanceCreate()
{
@@ -746,18 +761,19 @@ private:
testAMM(
[&](AMM& ammAlice, Env& env) {
env(fset(gw_, asfGlobalFreeze));
if (!features[featureAMMClawback])
auto const freezeBlocksAll =
features[featureAMMClawback] || features[fixCleanup3_3_0];
if (!freezeBlocksAll)
{
// If the issuer set global freeze, the holder still can
// deposit the other non-frozen token when AMMClawback is
// not enabled.
// deposit the other non-frozen token when neither
// AMMClawback nor fixCleanup3_3_0 is enabled.
ammAlice.deposit(carol_, XRP(100));
}
else
{
// If the issuer set global freeze, the holder cannot
// deposit the other non-frozen token when AMMClawback is
// enabled.
// deposit the other non-frozen token.
ammAlice.deposit(
carol_, XRP(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecFROZEN));
}
@@ -786,16 +802,18 @@ private:
[&](AMM& ammAlice, Env& env) {
env(trust(gw_, carol_["USD"](0), tfSetFreeze));
env.close();
if (!features[featureAMMClawback])
auto const freezeBlocksAll =
features[featureAMMClawback] || features[fixCleanup3_3_0];
if (!freezeBlocksAll)
{
// Can deposit non-frozen token if AMMClawback is not
// enabled
// Can deposit non-frozen token if neither AMMClawback
// nor fixCleanup3_3_0 is enabled
ammAlice.deposit(carol_, XRP(100));
}
else
{
// Cannot deposit non-frozen token if the other token is
// frozen when AMMClawback is enabled
// frozen
ammAlice.deposit(
carol_, XRP(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecFROZEN));
}
@@ -810,8 +828,18 @@ private:
STAmount{Issue{gw_["USD"].currency, ammAlice.ammAccount()}, 0},
tfSetFreeze));
env.close();
// Can deposit non-frozen token
ammAlice.deposit(carol_, XRP(100));
// Post-fixCleanup3_3_0: checkDepositFreeze checks both pool
// assets against the AMM account, so depositing the
// non-frozen token is also blocked.
if (!features[fixCleanup3_3_0])
{
ammAlice.deposit(carol_, XRP(100));
}
else
{
ammAlice.deposit(
carol_, XRP(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecFROZEN));
}
ammAlice.deposit(carol_, 1'000'000, std::nullopt, std::nullopt, Ter(tecFROZEN));
ammAlice.deposit(
carol_, USD(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecFROZEN));
@@ -861,11 +889,10 @@ private:
AMM amm(env, alice_, XRP(10), gw_["USD"](10), Ter(tesSUCCESS));
env.close();
if (features[featureAMMClawback])
if (features[featureAMMClawback] || features[fixCleanup3_3_0])
{
// if featureAMMClawback is enabled, bob_ can not deposit XRP
// because he's not authorized to hold the paired token
// gw_["USD"].
// bob_ can not deposit XRP because he's not authorized to
// hold the paired token gw_["USD"].
amm.deposit(
bob_, XRP(10), std::nullopt, std::nullopt, std::nullopt, Ter(tecNO_AUTH));
}
@@ -1734,36 +1761,57 @@ private:
});
// Globally frozen asset
testAMM([&](AMM& ammAlice, Env& env) {
ammAlice.deposit({.account = gw_, .asset1In = USD(1'000), .asset2In = XRP(1'000)});
env(fset(gw_, asfGlobalFreeze));
env.close();
// Can withdraw non-frozen token
for (auto const& account : {alice_, gw_})
{
ammAlice.withdraw(account, XRP(100));
ammAlice.withdraw(account, USD(100), std::nullopt, std::nullopt, Ter(tecFROZEN));
ammAlice.withdraw(account, 1'000, std::nullopt, std::nullopt, Ter(tecFROZEN));
}
});
testAMM(
[&](AMM& ammAlice, Env& env) {
auto const fix330 = env.current()->rules().enabled(fixCleanup3_3_0);
ammAlice.deposit({.account = gw_, .asset1In = USD(1'000), .asset2In = XRP(1'000)});
env(fset(gw_, asfGlobalFreeze));
env.close();
// Can withdraw non-frozen token
for (auto const& account : {alice_, gw_})
{
ammAlice.withdraw(account, XRP(100));
// Post-fixCleanup3_3_0 the issuer can withdraw their own
// frozen token from the pool.
auto const frozenErr =
(fix330 && account == gw_) ? Ter(tesSUCCESS) : Ter(tecFROZEN);
ammAlice.withdraw(account, USD(100), std::nullopt, std::nullopt, frozenErr);
ammAlice.withdraw(account, 1'000, std::nullopt, std::nullopt, frozenErr);
}
},
std::nullopt,
0,
std::nullopt,
amendmentCombinations({fixCleanup3_3_0}));
// Individually frozen (AMM) account
testAMM([&](AMM& ammAlice, Env& env) {
env(trust(gw_, alice_["USD"](0), tfSetFreeze));
env.close();
// Can withdraw non-frozen token
ammAlice.withdraw(alice_, XRP(100));
ammAlice.withdraw(alice_, 1'000, std::nullopt, std::nullopt, Ter(tecFROZEN));
ammAlice.withdraw(alice_, USD(100), std::nullopt, std::nullopt, Ter(tecFROZEN));
env(trust(gw_, alice_["USD"](0), tfClearFreeze));
// Individually frozen AMM
env(trust(
gw_, STAmount{Issue{gw_["USD"].currency, ammAlice.ammAccount()}, 0}, tfSetFreeze));
// Can withdraw non-frozen token
ammAlice.withdraw(alice_, XRP(100));
ammAlice.withdraw(alice_, 1'000, std::nullopt, std::nullopt, Ter(tecFROZEN));
ammAlice.withdraw(alice_, USD(100), std::nullopt, std::nullopt, Ter(tecFROZEN));
});
testAMM(
[&](AMM& ammAlice, Env& env) {
auto const fix330 = env.current()->rules().enabled(fixCleanup3_3_0);
env(trust(gw_, alice_["USD"](0), tfSetFreeze));
env.close();
// Can withdraw non-frozen token
ammAlice.withdraw(alice_, XRP(100));
// Post-fixCleanup3_3_0 regular freeze no longer blocks
// self-withdrawal; only deep freeze does.
auto const indivFreezeErr = fix330 ? Ter(tesSUCCESS) : Ter(tecFROZEN);
ammAlice.withdraw(alice_, 1'000, std::nullopt, std::nullopt, indivFreezeErr);
ammAlice.withdraw(alice_, USD(100), std::nullopt, std::nullopt, indivFreezeErr);
env(trust(gw_, alice_["USD"](0), tfClearFreeze));
// Individually frozen AMM — still blocked regardless of
// fixCleanup3_3_0 because the AMM account itself is frozen.
env(trust(
gw_,
STAmount{Issue{gw_["USD"].currency, ammAlice.ammAccount()}, 0},
tfSetFreeze));
ammAlice.withdraw(alice_, XRP(100));
ammAlice.withdraw(alice_, 1'000, std::nullopt, std::nullopt, Ter(tecFROZEN));
ammAlice.withdraw(alice_, USD(100), std::nullopt, std::nullopt, Ter(tecFROZEN));
},
std::nullopt,
0,
std::nullopt,
amendmentCombinations({fixCleanup3_3_0}));
// Carol withdraws more than she owns
testAMM([&](AMM& ammAlice, Env&) {
@@ -6649,11 +6697,11 @@ private:
});
}
if (features[featureAMMClawback])
if (features[featureAMMClawback] || features[fixCleanup3_3_0])
{
// Deposit one asset which is not the frozen token,
// but the other asset is frozen. We should get tecFROZEN error
// when feature AMMClawback is enabled.
// but the other asset is frozen. tecFROZEN when either
// AMMClawback or fixCleanup3_3_0 is enabled.
Env env(*this, features);
testAMMDeposit(env, [&](AMM& amm) {
amm.deposit(
@@ -6663,8 +6711,8 @@ private:
else
{
// Deposit one asset which is not the frozen token,
// but the other asset is frozen. We will get tecSUCCESS
// when feature AMMClawback is not enabled.
// but the other asset is frozen. tesSUCCESS only when
// neither AMMClawback nor fixCleanup3_3_0 is enabled.
Env env(*this, features);
testAMMDeposit(env, [&](AMM& amm) {
amm.deposit(
@@ -7187,8 +7235,8 @@ private:
FeatureBitset const all{testableAmendments()};
testInvalidInstance();
testInstanceCreate();
testInvalidDeposit(all);
testInvalidDeposit(all - featureAMMClawback);
for (auto const& f : amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}))
testInvalidDeposit(f);
testDeposit();
testInvalidWithdraw();
testWithdraw();
@@ -7238,8 +7286,8 @@ private:
testAMMClawback(all - featureAMMClawback - featureSingleAssetVault);
testAMMClawback(all - featureAMMClawback);
testAMMClawback(all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback);
testAMMDepositWithFrozenAssets(all);
testAMMDepositWithFrozenAssets(all - featureAMMClawback);
for (auto const& f : amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}))
testAMMDepositWithFrozenAssets(f);
testAMMDepositWithFrozenAssets(all - fixAMMv1_1 - featureAMMClawback);
testAMMDepositWithFrozenAssets(all - fixAMMv1_1 - fixAMMv1_3 - featureAMMClawback);
testFixReserveCheckOnWithdrawal(all);