From 3e9f1d0ab856775ee5a1fc3cf5e2ee942b508821 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 26 Jun 2026 23:38:59 +0200 Subject: [PATCH] fix: Unify freeze checks for pseudo-account deposit/withdraw (#7382) Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Co-authored-by: Ayaz Salikhov Co-authored-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> --- include/xrpl/ledger/helpers/TokenHelpers.h | 69 ++ include/xrpl/tx/transactors/dex/AMMWithdraw.h | 5 + src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | 26 +- src/libxrpl/ledger/helpers/TokenHelpers.cpp | 96 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 40 +- src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | 68 +- .../tx/transactors/dex/AMMWithdraw.cpp | 67 +- .../lending/LoanBrokerCoverDeposit.cpp | 24 +- .../lending/LoanBrokerCoverWithdraw.cpp | 35 +- .../tx/transactors/vault/VaultDeposit.cpp | 24 +- .../tx/transactors/vault/VaultWithdraw.cpp | 53 +- src/test/app/AMMMPT_test.cpp | 222 ++++- src/test/app/AMM_test.cpp | 145 ++- src/test/app/Invariants_test.cpp | 205 ++++ src/test/app/LoanBroker_test.cpp | 476 ++++++++- src/test/app/Loan_test.cpp | 21 +- src/test/app/MPToken_test.cpp | 51 +- src/test/app/Vault_test.cpp | 943 +++++++++++------- src/test/jtx/Env.h | 20 + src/test/jtx/impl/utility.cpp | 1 - src/test/jtx/utility.h | 2 +- 21 files changed, 2007 insertions(+), 586 deletions(-) diff --git a/include/xrpl/ledger/helpers/TokenHelpers.h b/include/xrpl/ledger/helpers/TokenHelpers.h index f736e51d28..0c9871cd76 100644 --- a/include/xrpl/ledger/helpers/TokenHelpers.h +++ b/include/xrpl/ledger/helpers/TokenHelpers.h @@ -131,6 +131,75 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, MPTIssue const& [[nodiscard]] TER checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& asset); +/** + * Checks freeze compliance for withdrawing an asset from a pseudo-account (e.g. Vault, AMM, + * LoanBroker) to a destination account. + * + * Asserts that sourceAcct is a pseudo-account and that submitterAcct and dstAcct are not. + * + * 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. If the asset is globally frozen the remaining checks are redundant. + * 2. For MPT shares: The pseudo-account's vault share must not be transitively frozen via its + * underlying asset. + * 3. The pseudo-account's trustline / MPToken must not be frozen for sending. + * 4. Skipped when submitter == dst (self-withdrawal); a regular freeze should not prevent + * recovering one's own funds. + * 5. 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. + * + * @param view Ledger view to read freeze state from. + * @param srcAcct Pseudo-account the funds are withdrawn from (sender). + * @param submitterAcct Account that submitted the withdrawal transaction. + * @param dstAcct Account receiving the withdrawn funds. + * @param asset Asset being withdrawn. + * @return tesSUCCESS if the withdrawal is permitted, otherwise a freeze + * result (tecFROZEN for IOUs, tecLOCKED for MPTs). + */ +[[nodiscard]] TER +checkWithdrawFreeze( + ReadView const& view, + AccountID const& srcAcct, + AccountID const& submitterAcct, + AccountID const& dstAcct, + Asset const& asset); + +/** + * Checks freeze compliance for depositing an asset into a pseudo-account (e.g. Vault, AMM, + * LoanBroker). + * + * + * Checks, in order: + * 1. If the asset is globally frozen the remaining checks are redundant. + * 2. For MPT shares: the pseudo-account's vault share must not be transitively frozen via its + * underlying asset (returns tecLOCKED). + * 3. The depositor must not be individually frozen. Skipped when srcAcct is the asset issuer, + * since the issuer can always send its own asset. + * 4. The pseudo-account must not be individually 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. + * + * @param view Ledger view to read freeze state from. + * @param srcAcct Depositor sending the funds. + * @param dstAcct Pseudo-account receiving the deposit. + * @param asset Asset being deposited. + * @return tesSUCCESS if the deposit is permitted, otherwise a freeze result + * (tecFROZEN for IOUs, tecLOCKED for MPTs). + */ +[[nodiscard]] TER +checkDepositFreeze( + ReadView const& view, + AccountID const& srcAcct, + AccountID const& dstAcct, + Asset const& asset); + //------------------------------------------------------------------------------ // // Account balance functions (Asset-based dispatchers) diff --git a/include/xrpl/tx/transactors/dex/AMMWithdraw.h b/include/xrpl/tx/transactors/dex/AMMWithdraw.h index 6e88320eae..8b6d39349b 100644 --- a/include/xrpl/tx/transactors/dex/AMMWithdraw.h +++ b/include/xrpl/tx/transactors/dex/AMMWithdraw.h @@ -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 applyGuts(Sandbox& view); diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index a1af63a80f..2781902f5b 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp @@ -308,6 +308,18 @@ requireAuth( AuthType authType, std::uint8_t depth) { + bool const fix330Enabled = view.rules().enabled(fixCleanup3_3_0); + bool const featureSAVEnabled = view.rules().enabled(featureSingleAssetVault); + bool const featureMPTV2Enabled = view.rules().enabled(featureMPTokensV2); + + // Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of their participants. + // They are implicitly authorized for any MPT they hold, including vault shares whose + // underlying asset would otherwise require auth. + auto const isPseudoAccountExempt = [&] { + return (featureSAVEnabled || featureMPTV2Enabled) && + isPseudoAccount(view, account, {&sfVaultID, &sfLoanBrokerID, &sfAMMID}); + }; + auto const mptID = keylet::mptokenIssuance(mptIssue.getMptID()); auto const sleIssuance = view.read(mptID); if (!sleIssuance) @@ -319,7 +331,9 @@ requireAuth( if (mptIssuer == account) // Issuer won't have MPToken return tesSUCCESS; - bool const featureSAVEnabled = view.rules().enabled(featureSingleAssetVault); + // Post-fix330: exempt before the recursive underlying-asset auth check. + if (fix330Enabled && isPseudoAccountExempt()) + return tesSUCCESS; if (featureSAVEnabled) { @@ -382,13 +396,9 @@ requireAuth( // belong to someone who is explicitly authorized e.g. a vault owner. } - bool const featureMPTV2Enabled = view.rules().enabled(featureMPTokensV2); - if (featureSAVEnabled || featureMPTV2Enabled) - { - // Implicitly authorize Vault, LoanBroker, and AMM pseudo-accounts - if (isPseudoAccount(view, account, {&sfVaultID, &sfLoanBrokerID, &sfAMMID})) - return tesSUCCESS; - } + // Pre-fix330: exempt after domain/sleToken checks, preserving prior behavior. + if (!fix330Enabled && isPseudoAccountExempt()) + return tesSUCCESS; // mptoken must be authorized if issuance enabled requireAuth if (sleIssuance->isFlag(lsfMPTRequireAuth) && diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index f0a0220e1e..b9adfbd3e6 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/TokenHelpers.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -34,14 +35,6 @@ namespace xrpl { -// Forward declaration for function that remains in View.h/cpp -bool -isLPTokenFrozen( - ReadView const& view, - AccountID const& account, - Asset const& asset, - Asset const& asset2); - //------------------------------------------------------------------------------ // // Freeze checking (Asset-based) @@ -164,6 +157,90 @@ checkDeepFrozen(ReadView const& view, AccountID const& account, Asset const& ass [&](auto const& issue) { return checkDeepFrozen(view, account, issue); }, asset.value()); } +[[nodiscard]] TER +checkWithdrawFreeze( + ReadView const& view, + AccountID const& srcAcct, + AccountID const& submitterAcct, + AccountID const& dstAcct, + Asset const& asset) +{ + XRPL_ASSERT( + isPseudoAccount(view, srcAcct), "xrpl::checkWithdrawFreeze : source is a pseudo-account"); + XRPL_ASSERT( + !isPseudoAccount(view, submitterAcct), + "xrpl::checkWithdrawFreeze : submitter is not a pseudo-account"); + XRPL_ASSERT( + !isPseudoAccount(view, dstAcct), + "xrpl::checkWithdrawFreeze : destination is not a pseudo-account"); + + // Funds can always be sent to the issuer + if (dstAcct == asset.getIssuer()) + return tesSUCCESS; + + // If the asset is globally frozen, other checks are redundant + if (auto const ret = checkGlobalFrozen(view, asset); !isTesSuccess(ret)) + return ret; + + // Special case for shares - check if the shares (and the transitive asset) is not frozen + if (asset.holds() && + isVaultPseudoAccountFrozen(view, srcAcct, asset.get(), 0)) + { + return tecLOCKED; + } + + // The transfer is from Submitter to Destination via Source (pseudo-account) + // Both Source and Submitter must not be frozen to allow sending funds + if (auto const ret = checkIndividualFrozen(view, srcAcct, asset); !isTesSuccess(ret)) + return ret; + + // Check submitter's individual freeze only when Submitter != Destination (a regular freeze + // should not block self-withdrawal). + if (submitterAcct != dstAcct) + { + if (auto const ret = checkIndividualFrozen(view, submitterAcct, asset); !isTesSuccess(ret)) + return ret; + } + + // The destination account must not be deep frozen to receive the funds + return checkDeepFrozen(view, dstAcct, asset); +} + +[[nodiscard]] TER +checkDepositFreeze( + ReadView const& view, + AccountID const& srcAcct, + AccountID const& dstAcct, + Asset const& asset) +{ + XRPL_ASSERT( + isPseudoAccount(view, dstAcct), + "xrpl::checkDepositFreeze : destination is a pseudo-account"); + XRPL_ASSERT( + !isPseudoAccount(view, srcAcct), + "xrpl::checkDepositFreeze : source is not a pseudo-account"); + + if (auto const ret = checkGlobalFrozen(view, asset); !isTesSuccess(ret)) + return ret; + + // Special case for shares - check if the shares and the transitive asset is not frozen + if (asset.holds() && + isVaultPseudoAccountFrozen(view, dstAcct, asset.get(), 0)) + { + return tecLOCKED; + } + + if (srcAcct != asset.getIssuer()) + { + if (auto const ret = checkIndividualFrozen(view, srcAcct, asset); !isTesSuccess(ret)) + return ret; + } + + // Unlike regular accounts, pseudo-accounts cannot receive deposits under a regular freeze + // because those funds cannot be later withdrawn + return checkIndividualFrozen(view, dstAcct, asset); +} + //------------------------------------------------------------------------------ // // Account balance functions @@ -776,7 +853,8 @@ directSendNoLimitMultiIOU( if (senderID == issuer || receiverID == issuer || issuer == noAccount()) { // Direct send: redeeming IOUs and/or sending own IOUs. - if (auto const ter = directSendNoFeeIOU(view, senderID, receiverID, amount, false, j)) + if (auto const ter = directSendNoFeeIOU(view, senderID, receiverID, amount, false, j); + !isTesSuccess(ter)) return ter; actual += amount; // Do not add amount to takeFromSender, because directSendNoFeeIOU took diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 1176594bbf..278c7a7858 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include #include #include @@ -509,6 +511,15 @@ ValidMPTTransfer::isAuthorized( AccountID const& holder, bool reqAuth) const { + // Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of their + // participants and are implicitly authorized for any MPT they hold, + // including vault shares whose underlying asset would otherwise require + // auth. Exempt them here rather than relying on requireAuth: the recursive + // share -> underlying descent in requireAuth fails for a pseudo-account + // that holds the share but not the underlying. + if (isPseudoAccount(view, holder, {&sfVaultID, &sfLoanBrokerID, &sfAMMID})) + return true; + auto const key = keylet::mptoken(mptid, holder); auto const it = deletedAuthorized_.find(key.key); if (it != deletedAuthorized_.end()) @@ -524,6 +535,8 @@ ValidMPTTransfer::finalize( ReadView const& view, beast::Journal const& j) { + auto const fix330Enabled = view.rules().enabled(fixCleanup3_3_0); + if (hasPrivilege(tx, OverrideFreeze)) return true; @@ -584,12 +597,29 @@ ValidMPTTransfer::finalize( ++senders; } - // Check once: if any involved account is frozen, the whole - // issuance transfer is considered frozen. Only need to check for - // frozen if there is a transfer of funds. + // Check once: if any involved account is frozen, the whole issuance transfer is + // considered frozen. Only need to check for frozen if there is a transfer of funds. + // + // Post-fix330: full isFrozen() applies — vault-share transitive freeze is part of + // the freeze semantics for all changed holders. + // + // Pre-fix330: legacy AMM withdraw only checked individual freeze on the + // destination, not the transitive vault freeze. All other paths (and the AMM + // account itself as sender) did apply the full check. + MPTIssue const issue{mptID}; + auto const legacyAccountFrozen = [&] { + if (isGlobalFrozen(view, issue) || isIndividualFrozen(view, account, issue)) + return true; + bool const isReceiver = + !value.amtBefore.has_value() || *value.amtAfter > *value.amtBefore; + if (txnType == ttAMM_WITHDRAW && isReceiver) + return false; + return isVaultPseudoAccountFrozen(view, account, issue, 0); + }; + bool const accountFrozen = + fix330Enabled ? isFrozen(view, account, issue) : legacyAccountFrozen(); if (!invalidTransfer && - (isFrozen(view, account, MPTIssue{mptID}) || - !isAuthorized(view, mptID, account, reqAuth))) + (accountFrozen || !isAuthorized(view, mptID, account, reqAuth))) { invalidTransfer = true; } diff --git a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp index 3665850f2d..ff7fec019a 100644 --- a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp @@ -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 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) { diff --git a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp index 6cbcfb1e50..f26164c5fe 100644 --- a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp @@ -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,25 @@ 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)) + return FreezeHandling::ZeroIfFrozen; + + 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 AMMWithdraw::applyGuts(Sandbox& sb) { @@ -329,18 +363,19 @@ 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{}, amount2 ? amount2->asset() : std::optional{}, - FreezeHandling::ZeroIfFrozen, + freezeHandling, AuthHandling::ZeroIfUnauthorized, ctx_.journal); if (!expected) return {expected.error(), false}; auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected; - auto const subTxType = ctx_.tx.getFlags() & tfWithdrawSubTx; auto const [result, newLPTokenBalance] = [&, @@ -469,7 +504,7 @@ AMMWithdraw::withdraw( lpTokensAMMBalance, lpTokensWithdraw, tfee, - FreezeHandling::ZeroIfFrozen, + issuerFreezeHandling(), AuthHandling::ZeroIfUnauthorized, isWithdrawAll(ctx_.tx), preFeeBalance_, @@ -756,7 +791,7 @@ AMMWithdraw::equalWithdrawTokens( lpTokens, lpTokensWithdraw, tfee, - FreezeHandling::ZeroIfFrozen, + issuerFreezeHandling(), AuthHandling::ZeroIfUnauthorized, isWithdrawAll(ctx_.tx), preFeeBalance_, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp index 76a5493a73..69b28b57af 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp @@ -43,6 +43,8 @@ LoanBrokerCoverDeposit::preflight(PreflightContext const& ctx) TER LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) { + auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0); auto const& tx = ctx.tx; auto const account = tx[sfAccount]; @@ -77,12 +79,21 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) // Cannot transfer a non-transferable Asset if (auto const ret = canTransfer(ctx.view, vaultAsset, account, pseudoAccountID)) return ret; - // Cannot transfer a frozen Asset - if (auto const ret = checkFrozen(ctx.view, account, vaultAsset)) - return ret; - // Pseudo-account cannot receive if asset is deep frozen - if (auto const ret = checkDeepFrozen(ctx.view, pseudoAccountID, vaultAsset)) - return ret; + + if (fix330Enabled) + { + if (auto const ret = checkDepositFreeze(ctx.view, account, pseudoAccountID, vaultAsset)) + return ret; + } + else + { + if (auto const ret = checkFrozen(ctx.view, account, vaultAsset)) + return ret; + + if (auto const ret = checkDeepFrozen(ctx.view, pseudoAccountID, vaultAsset)) + return ret; + } + // Cannot transfer unauthorized asset if (auto const ret = requireAuth(ctx.view, vaultAsset, account, AuthType::StrongAuth)) return ret; @@ -92,7 +103,6 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) // `sfCoverAvailable +=` could credit the broker more than the depositor paid Computing it // here in preclaim lets us reject sub-cover-scale dust early with tecPRECISION_LOSS instead of // failing only in doApply. - bool const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); auto const roundedAmount = [&]() -> STAmount { if (!fix320Enabled) return tx[sfAmount]; diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index 426f77cc70..008857a4ad 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -55,6 +55,8 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx) TER LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) { + auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0); auto const& tx = ctx.tx; auto const account = tx[sfAccount]; @@ -103,8 +105,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) // the lsfMPTCanTransfer flag check, so an issuer cannot trap a broker's // first-loss capital. 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 const ret = canTransfer(ctx.view, vaultAsset, pseudoAccountID, dstAcct, waive)) return ret; @@ -125,22 +126,30 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType)) return ter; - // Check for freezes, unless sending directly to the issuer - if (dstAcct != vaultAsset.getIssuer()) + if (fix330Enabled) { - // Cannot send a frozen Asset - if (auto const ret = checkFrozen(ctx.view, pseudoAccountID, vaultAsset)) - return ret; - // Destination account cannot receive if asset is deep frozen - if (auto const ret = checkDeepFrozen(ctx.view, dstAcct, vaultAsset)) + if (auto const ret = + checkWithdrawFreeze(ctx.view, pseudoAccountID, account, dstAcct, vaultAsset)) return ret; } + else + { // Check for freezes, unless sending directly to the issuer + if (dstAcct != vaultAsset.getIssuer()) + { + // Cannot send a frozen Asset + if (auto const ret = checkFrozen(ctx.view, pseudoAccountID, vaultAsset)) + return ret; + // Destination account cannot receive if asset is deep frozen + if (auto const ret = checkDeepFrozen(ctx.view, dstAcct, vaultAsset)) + return ret; + } + } auto const coverAvail = sleBroker->at(sfCoverAvailable); // Cover Rate is in 1/10 bips units auto const currentDebtTotal = sleBroker->at(sfDebtTotal); auto const minimumCover = [&]() { - if (ctx.view.rules().enabled(fixCleanup3_2_0)) + if (fix320Enabled) { return minimumBrokerCover( currentDebtTotal, TenthBips32{sleBroker->at(sfCoverRateMinimum)}, vault); @@ -159,11 +168,15 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) if ((coverAvail - amount) < minimumCover) return tecINSUFFICIENT_FUNDS; + auto const freezeHandling = fix330Enabled && dstAcct == vaultAsset.getIssuer() + ? FreezeHandling::IgnoreFreeze + : FreezeHandling::ZeroIfFrozen; + if (accountHolds( ctx.view, pseudoAccountID, vaultAsset, - FreezeHandling::ZeroIfFrozen, + freezeHandling, AuthHandling::ZeroIfUnauthorized, ctx.j) < amount) return tecINSUFFICIENT_FUNDS; diff --git a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp index b70543d280..d5eb80b84d 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp @@ -63,6 +63,9 @@ VaultDeposit::preflight(PreflightContext const& ctx) TER VaultDeposit::preclaim(PreclaimContext const& ctx) { + auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0); + auto const vault = ctx.view.read(keylet::vault(ctx.tx[sfVaultID])); if (!vault) return tecNO_ENTRY; @@ -107,13 +110,21 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - // Cannot deposit inside Vault an Asset frozen for the depositor - if (isFrozen(ctx.view, account, vaultAsset)) - return vaultAsset.holds() ? tecFROZEN : tecLOCKED; + if (fix330Enabled) + { + if (auto const ret = checkDepositFreeze(ctx.view, account, vaultAccount, vaultAsset)) + return ret; + } + else + { + // Cannot deposit inside Vault an Asset frozen for the depositor + if (isFrozen(ctx.view, account, vaultAsset)) + return vaultAsset.holds() ? tecFROZEN : tecLOCKED; - // Cannot deposit if the shares of the vault are frozen - if (isFrozen(ctx.view, account, vaultShare)) - return tecLOCKED; + // Cannot deposit if the shares of the vault are frozen + if (isFrozen(ctx.view, account, vaultShare)) + return tecLOCKED; + } if (vault->isFlag(lsfVaultPrivate) && account != vault->at(sfOwner)) { @@ -141,7 +152,6 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, account); !isTesSuccess(ter)) return ter; - bool const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); auto const roundedAmount = fix320Enabled ? roundToVaultScale(amount, vault) : amount; if (fix320Enabled && roundedAmount == beast::kZero) diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 815d8ccd5b..3d30005876 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -65,6 +65,10 @@ VaultWithdraw::preflight(PreflightContext const& ctx) TER VaultWithdraw::preclaim(PreclaimContext const& ctx) { + auto const fix313Enabled = ctx.view.rules().enabled(fixCleanup3_1_3); + auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0); + auto const vault = ctx.view.read(keylet::vault(ctx.tx[sfVaultID])); if (!vault) return tecNO_ENTRY; @@ -82,8 +86,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // 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)) { @@ -100,7 +103,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) // LCOV_EXCL_STOP } - if (ctx.view.rules().enabled(fixCleanup3_1_3) && amount.asset() == vaultShare) + if (fix313Enabled && amount.asset() == vaultShare) { // Post-fixCleanup3_1_3: if the user specified shares, convert // to the equivalent asset amount before checking withdrawal @@ -160,15 +163,30 @@ 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; - - // 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 (fix330Enabled) + { + // checkWithdrawFreeze checks the underlying asset on the source + // (vault pseudo-account), the submitter, and the destination. + // A separate share-level freeze check is unnecessary: vault shares + // are issued by the vault pseudo-account, which cannot submit + // MPTokenIssuanceSet to individually lock a holder's MPToken. + // The only way shares become locked is transitively via the + // underlying asset, which checkWithdrawFreeze covers. + if (auto const ret = + checkWithdrawFreeze(ctx.view, vaultAccount, account, dstAcct, vaultAsset)) + return ret; + } + else + { + // Cannot withdraw from a Vault an Asset frozen for the destination account + 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 +272,14 @@ VaultWithdraw::doApply() return tecPATH_DRY; } - if (accountHolds( - view(), accountID_, share, FreezeHandling::ZeroIfFrozen, AuthHandling::IgnoreAuth, j_) < + // Post-fixCleanup3_3_0: preclaim already validated all freeze conditions + // (checkWithdrawFreeze), so IgnoreFreeze avoids a redundant check that + // would incorrectly return zero for vault pseudo-accounts whose shares + // are frozen via a transitively frozen underlying asset. + auto const freezeHandling = view().rules().enabled(fixCleanup3_3_0) + ? FreezeHandling::IgnoreFreeze + : FreezeHandling::ZeroIfFrozen; + if (accountHolds(view(), accountID_, share, freezeHandling, AuthHandling::IgnoreAuth, j_) < sharesRedeemed) { JLOG(j_.debug()) << "VaultWithdraw: account doesn't hold enough shares"; @@ -358,10 +382,9 @@ VaultWithdraw::doApply() // else quietly ignore, account balance is not zero } - auto const dstAcct = ctx_.tx[~sfDestination].value_or(accountID_); - associateAsset(*vault, vaultAsset); + auto const dstAcct = ctx_.tx[~sfDestination].value_or(accountID_); return doWithdraw( view(), ctx_.tx, accountID_, dstAcct, vaultAccount, preFeeBalance_, assetsWithdrawn, j_); } diff --git a/src/test/app/AMMMPT_test.cpp b/src/test/app/AMMMPT_test.cpp index 5b576b41e4..9ff6c65c17 100644 --- a/src/test/app/AMMMPT_test.cpp +++ b/src/test/app/AMMMPT_test.cpp @@ -18,10 +18,12 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -46,6 +48,7 @@ #include #include #include +#include #include #include #include @@ -726,13 +729,14 @@ private: ammAlice.deposit(carol_, 1'000, std::nullopt, std::nullopt, Ter(tecLOCKED)); - if (!features[featureAMMClawback]) + // Post-fixCleanup3_3_0 a locked holder cannot deposit the other + // (non-locked) token either, matching featureAMMClawback. + if (!features[featureAMMClawback] && !features[fixCleanup3_3_0]) { ammAlice.deposit(carol_, USD(100), std::nullopt, std::nullopt, std::nullopt); } else { - // Carol can not deposit non-frozen token either ammAlice.deposit( carol_, USD(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecLOCKED)); } @@ -753,8 +757,15 @@ private: gw_, STAmount{Issue{gw_["USD"].currency, ammAlice.ammAccount()}, 0}, tfSetFreeze)); env.close(); - // Can deposit non-frozen token - ammAlice.deposit(carol_, btc(100), std::nullopt, std::nullopt, std::nullopt); + // Post-fixCleanup3_3_0 the deposit checks both pool assets, so the + // non-frozen token cannot be deposited while the AMM's USD is frozen. + ammAlice.deposit( + carol_, + btc(100), + std::nullopt, + std::nullopt, + std::nullopt, + features[fixCleanup3_3_0] ? Ter(tecFROZEN) : Ter(tesSUCCESS)); // Cannot deposit frozen token ammAlice.deposit(carol_, 1'000'000, std::nullopt, std::nullopt, Ter(tecFROZEN)); @@ -773,8 +784,15 @@ private: // Individually lock AMM btc.set({.holder = ammAlice.ammAccount(), .flags = tfMPTLock}); - // Can deposit non-frozen token - ammAlice.deposit(carol_, USD(100), std::nullopt, std::nullopt, std::nullopt); + // Post-fixCleanup3_3_0 the non-locked token cannot be deposited + // while the AMM's BTC is locked. + ammAlice.deposit( + carol_, + USD(100), + std::nullopt, + std::nullopt, + std::nullopt, + features[fixCleanup3_3_0] ? Ter(tecLOCKED) : Ter(tesSUCCESS)); // Can not deposit locked token ammAlice.deposit(carol_, 1'000, std::nullopt, std::nullopt, Ter(tecLOCKED)); @@ -788,7 +806,9 @@ private: ammAlice.deposit(carol_, btc(100), std::nullopt, std::nullopt, std::nullopt); } - // Individually lock MPT (AMM) account with MPT/MPT AMM + // Individually lock MPT (AMM) account with MPT/MPT AMM. + // This block always runs with all amendments (incl. fixCleanup3_3_0), + // so the deposit checks both pool assets unconditionally. { Env env{*this}; env.fund(XRP(10'000), gw_, alice_, carol_); @@ -826,8 +846,10 @@ private: // Individually lock MPT BTC (AMM) account btc.set({.holder = ammAlice.ammAccount(), .flags = tfMPTLock}); - // Can deposit non-locked token USD - ammAlice.deposit(carol_, usd(100), std::nullopt, std::nullopt, std::nullopt); + // Post-fixCleanup3_3_0 the non-locked token USD cannot be deposited + // while the AMM's BTC is locked. + ammAlice.deposit( + carol_, usd(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecLOCKED)); // Can not deposit locked token BTC ammAlice.deposit(carol_, 1'000, std::nullopt, std::nullopt, Ter(tecLOCKED)); @@ -843,8 +865,10 @@ private: // Individually Lock MPT USD (AMM) account usd.set({.holder = ammAlice.ammAccount(), .flags = tfMPTLock}); - // Can deposit non-locked token BTC - ammAlice.deposit(carol_, btc(100), std::nullopt, std::nullopt, std::nullopt); + // Post-fixCleanup3_3_0 the non-locked token BTC cannot be deposited + // while the AMM's USD is locked. + ammAlice.deposit( + carol_, btc(100), std::nullopt, std::nullopt, std::nullopt, Ter(tecLOCKED)); // Can not deposit locked token USD ammAlice.deposit(carol_, 1'000, std::nullopt, std::nullopt, Ter(tecLOCKED)); @@ -876,7 +900,9 @@ private: AMM amm(env, alice, XRP(10'000), btc(10'000)); env.close(); - if (!features[featureAMMClawback]) + // Post-fixCleanup3_3_0 the deposit requires authorization for both + // pool assets, so the unauthorized MPT blocks the XRP deposit too. + if (!features[featureAMMClawback] && !features[fixCleanup3_3_0]) { amm.deposit(carol, XRP(10), std::nullopt, std::nullopt, std::nullopt); } @@ -6863,6 +6889,173 @@ private: BEAST_EXPECT(!amm.ammExists()); } + void + testAMMWithVaultShares() + { + testcase("AMM with vault shares — underlying freeze blocks share withdrawal"); + using namespace jtx; + // AMMTestBase::testableAmendments() strips featureSingleAssetVault, + // but vault shares require it. Use the global jtx set directly. + FeatureBitset const all{jtx::testableAmendments()}; + + // When alice's underlying asset is individually frozen: + // + // Deposit (post-fixCleanup3_3_0): checkDepositFreeze checks the AMM + // pseudo-account's underlying, not alice's — deposit is allowed. + // Pre-fix: featureAMMClawback calls isFrozen(alice, share) which + // descends via isVaultPseudoAccountFrozen(alice,...) and finds the + // frozen underlying — deposit is blocked. + // + // Withdrawal (post-fixCleanup3_3_0): checkWithdrawFreeze ends with + // checkDeepFrozen(alice, share) which calls isFrozen(alice, share) + // and finds the frozen underlying — withdrawal is blocked. + // Pre-fix: the old path only checks the AMM account's MPToken lock, + // which is unset — withdrawal succeeds. + + auto runIOU = [&](FeatureBitset const& features) { + bool const fix330 = features[fixCleanup3_3_0]; + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + + env.fund(XRP(100'000), gw_, alice_); + env(fset(gw_, asfDefaultRipple)); + env.close(); + + PrettyAsset const iou = gw_["IOU"]; + env.trust(iou(1'000'000), alice_); + env(pay(gw_, alice_, iou(10'000))); + env.close(); + + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = alice_, .asset = iou}); + env(createTx); + env.close(); + + // 200 IOU → 200,000,000 vault shares (IOU vault scale = 6) + env(vault.deposit({.depositor = alice_, .id = vaultKeylet.key, .amount = iou(200)})); + env.close(); + + auto const shareMPTID = env.le(vaultKeylet)->at(sfShareMPTID); + // Use half the shares for the AMM; alice keeps the other half. + STAmount const shareAmt{MPTIssue{shareMPTID}, 100'000'000}; + // Pool: XRP(100) = 1e8 drops, shares = 1e8 → LP ≈ 1e8 + AMM amm{env, alice_, XRP(100), shareAmt}; + env.close(); + + // Freeze alice's IOU trustline (individual freeze on underlying). + env(trust(gw_, iou(0), alice_, tfSetFreeze)); + env.close(); + + // post-fix330: checkDepositFreeze checks AMM pseudo's underlying + // (not alice's) → deposit is allowed + // pre-fix330: featureAMMClawback path calls isFrozen(alice, share) + // which descends to alice's frozen IOU → tecLOCKED + amm.deposit( + {.account = alice_, + .asset1In = XRP(1), + .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecLOCKED))}); + + // post-fix330: checkWithdrawFreeze → checkDeepFrozen(alice, share) + // descends to alice's frozen IOU → tecLOCKED + // pre-fix330: the AMM pseudo-account is not authorized for the + // share's underlying (requireAuth recurses share→IOU + // and the AMM holds no IOU trustline), so + // accountHolds(ZeroIfUnauthorized) reports the pool's + // share balance as 0 and the withdrawal math fails → + // tecAMM_FAILED. Vault shares deposited into an AMM are + // only withdrawable once fixCleanup3_3_0 exempts the + // pseudo-account from the recursive auth check. + amm.withdraw( + {.account = alice_, + .tokens = 1'000, + .err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))}); + + env(trust(gw_, iou(0), alice_, tfClearFreeze)); + env.close(); + + // Lifting the freeze lets the deposit through in both cases. The + // withdrawal only succeeds post-fix330; pre-fix330 the share balance + // remains inaccessible to the unauthorized pseudo-account, so the + // shares stay stuck → tecAMM_FAILED. + amm.deposit({.account = alice_, .asset1In = XRP(1)}); + amm.withdraw( + {.account = alice_, + .tokens = 1'000, + .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))}); + }; + + runIOU(all); + runIOU(all - fixCleanup3_3_0); + + auto runMPT = [&](FeatureBitset const& features) { + bool const fix330 = features[fixCleanup3_3_0]; + // Expected freeze failures fire invariant checks that log at Error; + // silence them so the test output stays clean. + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + + env.fund(XRP(100'000), gw_, alice_); + env.close(); + + MPTTester mptt{env, gw_, kMptInitNoFund}; + mptt.create({.flags = kMptDexFlags | tfMPTCanLock}); + PrettyAsset const mpt = mptt.issuanceID(); + mptt.authorize({.account = alice_}); + env(pay(gw_, alice_, mpt(30'000))); + env.close(); + + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = alice_, .asset = mpt}); + env(createTx); + env.close(); + + // 20000 MPT → 20000 vault shares (MPT vault scale = 0) + env(vault.deposit({.depositor = alice_, .id = vaultKeylet.key, .amount = mpt(20'000)})); + env.close(); + + auto const shareMPTID = env.le(vaultKeylet)->at(sfShareMPTID); + // Use half the shares for the AMM; alice keeps the other half. + // Pool: XRP(100) = 1e8 drops, shares = 10000 → LP ≈ 1e6 + STAmount const shareAmt{MPTIssue{shareMPTID}, 10'000}; + AMM amm{env, alice_, XRP(100), shareAmt}; + env.close(); + + // Lock alice's underlying MPT (individual lock). + mptt.set({.holder = alice_, .flags = tfMPTLock}); + + // Same pre/post-fix330 semantics as the IOU case above. + amm.deposit( + {.account = alice_, + .asset1In = XRP(1), + .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecLOCKED))}); + + // {.tokens = 1'000} → frac = 1000/1e6 = 0.001 + // XRP out = 1e8 * 0.001 = 1e5 drops, shares out = 10000 * 0.001 = 10 + // post-fix330: checkWithdrawFreeze sees alice's locked underlying + // MPT via the share → tecLOCKED. + // pre-fix330: the AMM pseudo-account is unauthorized for the + // share's underlying MPT (it holds no underlying + // MPToken), so accountHolds(ZeroIfUnauthorized) zeros + // the pool's share balance and the math fails → + // tecAMM_FAILED. + amm.withdraw( + {.account = alice_, + .tokens = 1'000, + .err = Ter(fix330 ? TER(tecLOCKED) : TER(tecAMM_FAILED))}); + + mptt.set({.holder = alice_, .flags = tfMPTUnlock}); + + // Unlocking lets the deposit through; the withdrawal only succeeds + // post-fix330 (pre-fix330 the shares remain stuck → tecAMM_FAILED). + amm.deposit({.account = alice_, .asset1In = XRP(1)}); + amm.withdraw( + {.account = alice_, + .tokens = 1'000, + .err = Ter(fix330 ? TER(tesSUCCESS) : TER(tecAMM_FAILED))}); + }; + + runMPT(all); + runMPT(all - fixCleanup3_3_0); + } + void testAMMDepositWithFrozenAssets() { @@ -7085,8 +7278,8 @@ private: FeatureBitset const all{jtx::testableAmendments()}; testInstanceCreate(); testInvalidInstance(); - testInvalidDeposit(all); - testInvalidDeposit(all - featureAMMClawback); + for (auto const& f : jtx::amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}, all)) + testInvalidDeposit(f); testDeposit(); testInvalidWithdraw(); testWithdraw(); @@ -7113,6 +7306,7 @@ private: testLPTokenBalance(all); testLPTokenBalance(all - fixAMMv1_3); testAMMDepositWithFrozenAssets(); + testAMMWithVaultShares(); testAutoDelete(); } }; diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b01d58ddff..3672c4a68a 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -57,6 +57,7 @@ #include #include #include +#include #include #include #include @@ -85,6 +86,13 @@ private: return jtx::testableAmendments() - featureSingleAssetVault - featureLendingProtocol; } + // Seed from the local testableAmendments() which strips SAV and Lending. + static std::vector + amendmentCombinations(std::initializer_list features) + { + return jtx::amendmentCombinations(features, testableAmendments()); + } + void testInstanceCreate() { @@ -746,18 +754,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 +795,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 +821,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 +882,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 +1754,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&) { @@ -6659,11 +6700,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( @@ -6673,8 +6714,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( @@ -7197,8 +7238,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(); @@ -7248,8 +7289,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); diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 804cfaebfc..6aa17c7840 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -4598,6 +4599,210 @@ class Invariants_test : public beast::unit_test::Suite } } } + + // Vault-share transfer: ValidMPTTransfer gates isVaultPseudoAccountFrozen + // on fixCleanup3_3_0. Pre-amendment, vault-share transfers are allowed + // even when the underlying asset is individually frozen for the sender; + // post-amendment they are blocked. + { + Account const gw{"gw"}; + MPTID shareID{}; + + auto const preclose = [&](Account const& a1, Account const& a2, Env& env) -> bool { + env.fund(XRP(1'000), gw); + env.trust(gw["IOU"](10'000), a1); + env.trust(gw["IOU"](10'000), a2); + env.close(); + env(pay(gw, a1, gw["IOU"](500))); + env(pay(gw, a2, gw["IOU"](500))); + env.close(); + + PrettyAsset const iou = gw["IOU"]; + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = iou}); + env(createTx); + env.close(); + // Both a1 and a2 deposit IOU, each receiving vault shares. + env(vault.deposit({.depositor = a1, .id = vaultKeylet.key, .amount = iou(100)})); + env(vault.deposit({.depositor = a2, .id = vaultKeylet.key, .amount = iou(100)})); + env.close(); + + shareID = env.le(vaultKeylet)->at(sfShareMPTID); + + // Freeze a2's IOU trustline from the issuer side. + // a2 is the receiver in the simulated AMM withdraw; the + // distinction under test is that pre-fix330 the invariant + // does not apply the transitive vault freeze to receivers. + env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); + env.close(); + return true; + }; + + // Simulate a vault-share transfer: a1 sends 10 shares to a2. + auto const precheck = + [&](Account const& a1, Account const& a2, ApplyContext& ac) -> bool { + auto sle1 = ac.view().peek(keylet::mptoken(shareID, a1.id())); + auto sle2 = ac.view().peek(keylet::mptoken(shareID, a2.id())); + if (!sle1 || !sle2) + return false; + (*sle1)[sfMPTAmount] -= 10; + (*sle2)[sfMPTAmount] += 10; + ac.view().update(sle1); + ac.view().update(sle2); + return true; + }; + + // post-fixCleanup3_3_0: full isFrozen() applies to all holders; + // isVaultPseudoAccountFrozen finds a2's underlying IOU frozen → + // invalidTransfer → invariant fires. + doInvariantCheck( + Env{*this, defaultAmendments()}, + {{"invalid MPToken transfer between holders"}}, + precheck, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose); + + // pre-fixCleanup3_3_0: legacy AMM withdraw only checked + // checkIndividualFrozen on the destination, not the transitive + // vault freeze; a2 as receiver is exempt → invariant passes. + doInvariantCheck( + Env{*this, defaultAmendments() - fixCleanup3_3_0}, + {}, + precheck, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tesSUCCESS, tesSUCCESS}, + preclose); + } + + // Side-specific vault-share AMM_WITHDRAW invariant tests. + // Both cases use a real vault (IOU underlying) and a real AMM whose + // pool includes vault shares. precheck simulates an AMM_WITHDRAW by + // transferring 10 vault shares from the AMM pseudo-account to a2. + { + MPTID shareID{}; + AccountID ammAcctID{}; + AccountID vaultPseudoID{}; + Account const gw{"gw"}; + + // Simulate AMM_WITHDRAW: AMM pseudo-account sends 10 vault shares + // to a2. The AMM pseudo is the sender (decreasing balance); + // a2 is the receiver (increasing balance). + auto const precheck2 = + [&](Account const& /*a1*/, Account const& a2, ApplyContext& ac) -> bool { + auto sleAMM = ac.view().peek(keylet::mptoken(shareID, ammAcctID)); + auto sle2 = ac.view().peek(keylet::mptoken(shareID, a2.id())); + if (!sleAMM || !sle2) + return false; + (*sleAMM)[sfMPTAmount] -= 10; + (*sle2)[sfMPTAmount] += 10; + ac.view().update(sleAMM); + ac.view().update(sle2); + return true; + }; + + // Shared vault + AMM setup: a1 deposits 500 IOU into a vault and + // creates an AMM with XRP + 100 vault shares, giving the AMM + // pseudo-account a vault-share MPToken balance. + auto const setupVaultAMM = [&](Account const& a1, Account const& a2, Env& env) -> bool { + env.fund(XRP(1'000), gw); + env(fset(gw, asfDefaultRipple)); + env.close(); + + env.trust(gw["IOU"](10'000), a1); + env.trust(gw["IOU"](10'000), a2); + env.close(); + env(pay(gw, a1, gw["IOU"](1'000))); + env(pay(gw, a2, gw["IOU"](500))); + env.close(); + + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = a1, .asset = gw["IOU"]}); + env(createTx); + env.close(); + + env(vault.deposit( + {.depositor = a1, .id = vaultKeylet.key, .amount = gw["IOU"](500)})); + env(vault.deposit( + {.depositor = a2, .id = vaultKeylet.key, .amount = gw["IOU"](200)})); + env.close(); + + shareID = env.le(vaultKeylet)->at(sfShareMPTID); + vaultPseudoID = env.le(vaultKeylet)->at(sfAccount); + + // a1 creates AMM with XRP + 100 vault shares; the AMM + // pseudo-account receives an MPToken record for shareID. + AMM const amm(env, a1, XRP(100), STAmount{MPTIssue{shareID}, 100}); + ammAcctID = amm.ammAccount(); + return true; + }; + + // Case 1: freeze the vault pseudo-account's IOU trustline. + // isVaultPseudoAccountFrozen(ammAcct) calls isAnyFrozen({vaultPseudo, + // ammAcct}, IOU); since vaultPseudo is frozen it returns true. The + // AMM sender has a decreasing balance (not a receiver) so it is + // never exempt from the check — invariant fires both pre- and + // post-fixCleanup3_3_0. + auto const preclose3 = [&](Account const& a1, Account const& a2, Env& env) -> bool { + if (!setupVaultAMM(a1, a2, env)) + return false; + env(trust(gw, gw["IOU"](0), Account{"vaultPseudo", vaultPseudoID}, tfSetFreeze)); + env.close(); + return true; + }; + + doInvariantCheck( + Env{*this, defaultAmendments()}, + {{"invalid MPToken transfer between holders"}}, + precheck2, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose3); + + doInvariantCheck( + Env{*this, defaultAmendments() - fixCleanup3_3_0}, + {{"invalid MPToken transfer between holders"}}, + precheck2, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose3); + + // Case 2: freeze a2's (receiver's) IOU trustline. + // isVaultPseudoAccountFrozen(a2) → isAnyFrozen({vaultPseudo, a2}, + // IOU) → true. The AMM sender's check passes (vaultPseudo and + // ammAcct are not frozen). Pre-fix330: receiver is exempt from + // isVaultPseudoAccountFrozen in ttAMM_WITHDRAW → passes. + // Post-fix330: full isFrozen() applied to a2 → fires. + auto const preclose4 = [&](Account const& a1, Account const& a2, Env& env) -> bool { + if (!setupVaultAMM(a1, a2, env)) + return false; + env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); + env.close(); + return true; + }; + + doInvariantCheck( + Env{*this, defaultAmendments()}, + {{"invalid MPToken transfer between holders"}}, + precheck2, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + preclose4); + + doInvariantCheck( + Env{*this, defaultAmendments() - fixCleanup3_3_0}, + {}, + precheck2, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject&) {}}, + {tesSUCCESS, tesSUCCESS}, + preclose4); + } } void diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 671f98a901..bdc950eeff 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -936,11 +936,7 @@ class LoanBroker_test : public beast::unit_test::Suite env.close(); env(coverDeposit(alice, brokerKeylet.key, vaultInfo.asset(10)), Ter(tecINSUFFICIENT_FUNDS)); - - // preclaim: tecFROZEN - env(fset(issuer, asfGlobalFreeze)); - env.close(); - env(coverDeposit(alice, brokerKeylet.key, vaultInfo.asset(10)), Ter(tecFROZEN)); + // Freeze/lock tests are in testCoverDepositFreezes/testCoverWithdrawFreezes } else { @@ -966,35 +962,20 @@ class LoanBroker_test : public beast::unit_test::Suite // preclaim: tecDST_TAG_NEEDED Account const dest{"dest"}; env.fund(XRP(1'000), dest); + env(fset(dest, asfRequireDest)); - env.close(); env(coverWithdraw(alice, brokerKeylet.key, asset(10)), kDestination(dest), Ter(tecDST_TAG_NEEDED)); + env(fclear(dest, asfRequireDest)); // preclaim: tecNO_PERMISSION - env(fclear(dest, asfRequireDest)); env(fset(dest, asfDepositAuth)); - env.close(); env(coverWithdraw(alice, brokerKeylet.key, asset(10)), kDestination(dest), Ter(tecNO_PERMISSION)); - - // preclaim: tecFROZEN - env(trust(dest, asset(1'000))); env(fclear(dest, asfDepositAuth)); - env(fset(issuer, asfGlobalFreeze)); - env.close(); - env(coverWithdraw(alice, brokerKeylet.key, asset(10)), - kDestination(dest), - Ter(tecFROZEN)); - - // preclaim:: tecFROZEN (deep frozen) - env(fclear(issuer, asfGlobalFreeze)); - env(trust(issuer, asset(1'000), dest, tfSetFreeze | tfSetDeepFreeze)); - env(coverWithdraw(alice, brokerKeylet.key, asset(10)), - kDestination(dest), - Ter(tecFROZEN)); + // Freeze/lock tests are in testCoverDepositFreezes/testCoverWithdrawFreezes // preclaim: tecPSEUDO_ACCOUNT env(coverWithdraw(alice, brokerKeylet.key, asset(10)), @@ -1784,6 +1765,444 @@ class LoanBroker_test : public beast::unit_test::Suite BEAST_EXPECT(aliceBalanceAfter == aliceBalanceBefore); } + void + testCoverDepositFreezes() + { + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + // === IOU === + { + testcase("LoanBrokerCoverDeposit IOU freeze checks"); + Env env(*this); + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env(trust(alice, issuer["IOU"](1'000'000))); + env.close(); + PrettyAsset const asset(issuer["IOU"]); + env(pay(issuer, alice, asset(100'000))); + env.close(); + + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); + env(tx); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)})); + env.close(); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Account const brokerPseudo("pseudo", broker->at(sfAccount)); + + env(coverDeposit(alice, brokerKeylet.key, asset(10))); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Global freeze + env(fset(issuer, asfGlobalFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1)), Ter(tecFROZEN)); + env(fclear(issuer, asfGlobalFreeze)); + + // Source regular freeze + env(trust(issuer, asset(0), alice, tfSetFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1)), Ter(tecFROZEN)); + env(trust(issuer, asset(0), alice, tfClearFreeze)); + + // Source deep freeze + env(trust(issuer, asset(0), alice, tfSetFreeze | tfSetDeepFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1)), Ter(tecFROZEN)); + env(trust(issuer, asset(0), alice, tfClearFreeze | tfClearDeepFreeze)); + + // Pseudo regular freeze — post-fix blocks, pre-fix allows (BUG) + TER const pseudoTer = fix330Enabled ? TER(tecFROZEN) : TER(tesSUCCESS); + env(trust(issuer, asset(0), brokerPseudo, tfSetFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1)), Ter(pseudoTer)); + env(trust(issuer, asset(0), brokerPseudo, tfClearFreeze)); + + // Pseudo deep freeze + env(trust(issuer, asset(0), brokerPseudo, tfSetFreeze | tfSetDeepFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1)), Ter(tecFROZEN)); + env(trust(issuer, asset(0), brokerPseudo, tfClearFreeze | tfClearDeepFreeze)); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + // === MPT === + { + testcase("LoanBrokerCoverDeposit MPT lock checks"); + Env env(*this); + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + PrettyAsset const mpt{mptt.issuanceID()}; + + mptt.authorize({.account = alice}); + env(pay(issuer, alice, mpt(100'000))); + env.close(); + + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = mpt}); + env(tx); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = mpt(50)})); + env.close(); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Account const brokerPseudo("pseudo", broker->at(sfAccount)); + + env(coverDeposit(alice, brokerKeylet.key, mpt(10))); + env.close(); + + // For MPT isDeepFrozen == isFrozen, so all locks block in + // both pre- and post-fix. No behavioral difference. + auto runTests = [&]() { + // Global lock + mptt.set({.flags = tfMPTLock}); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, mpt(1)), Ter(tecLOCKED)); + mptt.set({.flags = tfMPTUnlock}); + env.close(); + + // Source (alice) individual lock + mptt.set({.holder = alice, .flags = tfMPTLock}); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, mpt(1)), Ter(tecLOCKED)); + mptt.set({.holder = alice, .flags = tfMPTUnlock}); + env.close(); + + // Pseudo individual lock + mptt.set({.holder = brokerPseudo, .flags = tfMPTLock}); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, mpt(1)), Ter(tecLOCKED)); + mptt.set({.holder = brokerPseudo, .flags = tfMPTUnlock}); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + } + + // Focused demonstration: a cover-withdraw submitter under a regular + // individual IOU freeze can still withdraw to themselves (self-withdrawal). + // + // Pre-fixCleanup3_3_0: the old code only checked the pseudo-account source + // and the destination for deep-freeze; it did not check the submitter's + // individual freeze at all. Self-withdrawal therefore always succeeded. + // Post-fixCleanup3_3_0: checkWithdrawFreeze explicitly skips the submitter + // freeze check when submitter == destination, preserving the same result. + void + testCoverWithdrawSelfWhileFrozen() + { + testcase("LoanBrokerCoverWithdraw IOU self-withdrawal while individually frozen"); + + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const dest{"dest"}; + Env env{*this}; + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice, dest); + env(trust(alice, issuer["IOU"](1'000'000))); + env(trust(dest, issuer["IOU"](1'000'000))); + env.close(); + + PrettyAsset const asset(issuer["IOU"]); + env(pay(issuer, alice, asset(100'000))); + env.close(); + + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); + env(vaultTx); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)})); + env.close(); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, asset(10))); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Set a regular individual freeze on alice's IOU trustline. + env(trust(issuer, asset(0), alice, tfSetFreeze)); + env.close(); + + // Self-withdrawal: submitter == destination (no sfDestination in tx). + // Both pre- and post-fixCleanup3_3_0 this succeeds: + // pre-fix: old code never checked the submitter's freeze. + // post-fix: checkWithdrawFreeze skips submitter when submitter==dst. + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), Ter(tesSUCCESS)); + + // Withdrawal to a third party is blocked by the submitter freeze + // under fixCleanup3_3_0; pre-fix it was not checked. + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(fix330Enabled ? TER(tecFROZEN) : TER(tesSUCCESS))); + + env(trust(issuer, asset(0), alice, tfClearFreeze)); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + void + testCoverWithdrawFreezes() + { + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + // === IOU === + { + testcase("LoanBrokerCoverWithdraw IOU freeze checks"); + Env env(*this); + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env(trust(alice, issuer["IOU"](1'000'000))); + env.close(); + PrettyAsset const asset(issuer["IOU"]); + env(pay(issuer, alice, asset(100'000))); + env.close(); + + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); + env(tx); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)})); + env.close(); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Account const brokerPseudo("pseudo", broker->at(sfAccount)); + + env(coverDeposit(alice, brokerKeylet.key, asset(10))); + env.close(); + + Account const dest{"dest"}; + env.fund(XRP(1'000), dest); + env(trust(dest, asset(1'000))); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + TER const expectedTec = fix330Enabled ? TER(tecFROZEN) : TER(tesSUCCESS); + + // Global freeze + env(fset(issuer, asfGlobalFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(tecFROZEN)); + env(fclear(issuer, asfGlobalFreeze)); + + // Source (pseudo) regular freeze + env(trust(issuer, asset(0), brokerPseudo, tfSetFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(tecFROZEN)); + env(trust(issuer, asset(0), brokerPseudo, tfClearFreeze)); + + // Source (pseudo) deep freeze + env(trust(issuer, asset(0), brokerPseudo, tfSetFreeze | tfSetDeepFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(tecFROZEN)); + env(trust(issuer, asset(0), brokerPseudo, tfClearFreeze | tfClearDeepFreeze)); + + // Submitter regular freeze → dest + env(trust(issuer, asset(0), alice, tfSetFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(expectedTec)); + // Submitter regular freeze → self: always allowed + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), Ter(tesSUCCESS)); + env(trust(issuer, asset(0), alice, tfClearFreeze)); + env(coverDeposit( + alice, brokerKeylet.key, asset(isTesSuccess(expectedTec) ? 2 : 1))); + + // Submitter deep freeze → dest + env(trust(issuer, asset(0), alice, tfSetFreeze | tfSetDeepFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(expectedTec)); + // Submitter deep freeze → self: blocked (checkDeepFrozen) + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), Ter(tecFROZEN)); + env(trust(issuer, asset(0), alice, tfClearFreeze | tfClearDeepFreeze)); + if (isTesSuccess(expectedTec)) + env(coverDeposit(alice, brokerKeylet.key, asset(1))); + + // Destination regular freeze: only deep freeze blocks + env(trust(issuer, asset(0), dest, tfSetFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(tesSUCCESS)); + env(trust(issuer, asset(0), dest, tfClearFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1))); + + // Destination deep freeze + env(trust(issuer, asset(0), dest, tfSetFreeze | tfSetDeepFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(dest), + Ter(tecFROZEN)); + env(trust(issuer, asset(0), dest, tfClearFreeze | tfClearDeepFreeze)); + + // Submitter frozen → issuer: bypasses all freeze checks + env(trust(issuer, asset(0), alice, tfSetFreeze)); + env(coverWithdraw(alice, brokerKeylet.key, asset(1)), + kDestination(issuer), + Ter(tesSUCCESS)); + env(trust(issuer, asset(0), alice, tfClearFreeze)); + env(coverDeposit(alice, brokerKeylet.key, asset(1))); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + // === MPT === + { + testcase("LoanBrokerCoverWithdraw MPT lock checks"); + Env env(*this); + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + PrettyAsset const mpt{mptt.issuanceID()}; + + mptt.authorize({.account = alice}); + env(pay(issuer, alice, mpt(100'000))); + env.close(); + + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = mpt}); + env(tx); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = mpt(50)})); + env.close(); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Account const brokerPseudo("pseudo", broker->at(sfAccount)); + + env(coverDeposit(alice, brokerKeylet.key, mpt(10))); + env.close(); + + Account const dest{"dest"}; + env.fund(XRP(1'000), dest); + mptt.authorize({.account = dest}); + env.close(); + + auto runTests = [&]() { + auto const withFix = env.current()->rules().enabled(fixCleanup3_3_0); + // Only submitter-to-dest differs: post-fix blocks, pre-fix + // doesn't (BUG). All other locks block in both because for + // MPT isDeepFrozen == isFrozen. + TER const submitterToDest = withFix ? TER(tecLOCKED) : TER(tesSUCCESS); + + // Global lock + mptt.set({.flags = tfMPTLock}); + env.close(); + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), + kDestination(dest), + Ter(tecLOCKED)); + mptt.set({.flags = tfMPTUnlock}); + env.close(); + + // Source (pseudo) individual lock + mptt.set({.holder = brokerPseudo, .flags = tfMPTLock}); + env.close(); + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), + kDestination(dest), + Ter(tecLOCKED)); + mptt.set({.holder = brokerPseudo, .flags = tfMPTUnlock}); + env.close(); + + // Submitter individual lock → dest + mptt.set({.holder = alice, .flags = tfMPTLock}); + env.close(); + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), + kDestination(dest), + Ter(submitterToDest)); + // Submitter individual lock → self: blocked + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), Ter(tecLOCKED)); + mptt.set({.holder = alice, .flags = tfMPTUnlock}); + env.close(); + if (isTesSuccess(submitterToDest)) + env(coverDeposit(alice, brokerKeylet.key, mpt(1))); + env.close(); + + // Dest individual lock: blocked + mptt.set({.holder = dest, .flags = tfMPTLock}); + env.close(); + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), + kDestination(dest), + Ter(tecLOCKED)); + mptt.set({.holder = dest, .flags = tfMPTUnlock}); + env.close(); + + // Submitter locked → issuer: bypasses all freeze checks + mptt.set({.holder = alice, .flags = tfMPTLock}); + env.close(); + env(coverWithdraw(alice, brokerKeylet.key, mpt(1)), + kDestination(issuer), + Ter(tesSUCCESS)); + mptt.set({.holder = alice, .flags = tfMPTUnlock}); + env(coverDeposit(alice, brokerKeylet.key, mpt(1))); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + } + void testRIPD4274IOU() { @@ -2244,6 +2663,13 @@ public: void run() override { + testInvalidLoanBrokerCoverClawback(); + testInvalidLoanBrokerCoverDeposit(); + testInvalidLoanBrokerCoverWithdraw(); + testCoverDepositFreezes(); + testCoverWithdrawFreezes(); + testCoverWithdrawSelfWhileFrozen(); + testCoverPrecisionGuard(); testLoanBrokerSetDebtMaximum(); @@ -2251,9 +2677,6 @@ public: testDisabled(); testLifecycle(); - testInvalidLoanBrokerCoverClawback(); - testInvalidLoanBrokerCoverDeposit(); - testInvalidLoanBrokerCoverWithdraw(); testInvalidLoanBrokerDelete(); testInvalidLoanBrokerSet(); testRequireAuth(); @@ -2268,7 +2691,6 @@ public: testLoanBrokerDeleteFrozenIOU(all_); testLoanBrokerDeleteFrozenIOU(all_ - fixCleanup3_2_0); - // TODO: Write clawback failure tests with an issuer / MPT that doesn't // have the right flags set. } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 1b0dd414e1..56a7cab47a 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -92,23 +92,6 @@ protected: // even if they are set to unsupported. FeatureBitset const all_{jtx::testableAmendments()}; - - // All 2^N permutations of `all_` with each subset of the given features - // excluded. The first entry is always `all_` itself (empty exclusion); - // the last excludes every feature in the list. - std::vector - amendmentCombinations(std::initializer_list features) const - { - std::vector result{all_}; - 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; - } - std::string const iouCurrency_{"IOU"}; void @@ -8624,8 +8607,8 @@ public: run() override { runAmendmentIndependent(); - for (auto const& features : - amendmentCombinations({fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2})) + for (auto const& features : jtx::amendmentCombinations( + {fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2}, all_)) runAmendmentSensitive(features); } }; diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index bfb80d3e36..323184aa36 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -7461,21 +7461,42 @@ class MPToken_test : public beast::unit_test::Suite // MPTLock is set usd.set({.flags = tfMPTLock}); - // carol and issuer can't withdraw - for (auto const& account : {carol, gw}) - { - amm.withdraw( - {.account = account, - .asset1Out = usd(1), - .asset2Out = eur(1), - .err = Ter(tecLOCKED)}); - amm.withdraw({.account = account, .tokens = 1'000, .err = Ter(tecLOCKED)}); - // can single withdraw another asset - amm.withdraw( - {.account = account, - .asset1Out = eur(1), - .assets = std::make_pair(eur, usd)}); - } + auto const fix330 = env.current()->rules().enabled(fixCleanup3_3_0); + + // carol can't withdraw the locked token (any withdrawal type) + amm.withdraw( + {.account = carol, + .asset1Out = usd(1), + .asset2Out = eur(1), + .err = Ter(tecLOCKED)}); + amm.withdraw({.account = carol, .tokens = 1'000, .err = Ter(tecLOCKED)}); + // can single withdraw the non-locked asset + amm.withdraw( + {.account = carol, .asset1Out = eur(1), .assets = std::make_pair(eur, usd)}); + + // post-fixCleanup3_3_0 the issuer can redeem even when locked. + // Each successful withdrawal burns LP tokens, so replenish between + // each type to keep gw's LP balance stable. + auto const gwLockErr = fix330 ? Ter(tesSUCCESS) : Ter(tecLOCKED); + auto const replenish = [&](IOUAmount tokens) { + usd.set({.flags = tfMPTUnlock}); + amm.deposit({.account = gw, .tokens = tokens}); + usd.set({.flags = tfMPTLock}); + }; + + amm.withdraw( + {.account = gw, .asset1Out = usd(1), .asset2Out = eur(1), .err = gwLockErr}); + if (fix330) + replenish(IOUAmount{1}); + + amm.withdraw({.account = gw, .tokens = 1'000, .err = gwLockErr}); + if (fix330) + replenish(IOUAmount{1'000}); + + // can single withdraw the non-locked asset + amm.withdraw( + {.account = gw, .asset1Out = eur(1), .assets = std::make_pair(eur, usd)}); + usd.set({.flags = tfMPTUnlock}); // MPTRequireAuth is set diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 4643bc8555..6fefcfc404 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -1573,6 +1573,7 @@ class Vault_test : public beast::unit_test::Suite bool enableClawback = true; bool requireAuth = true; int initialXRP = 1000; + FeatureBitset features = testableAmendments(); }; auto testCase = [this]( @@ -1585,7 +1586,7 @@ class Vault_test : public beast::unit_test::Suite Vault& vault, MPTTester& mptt)> test, CaseArgs args = {}) { - Env env{*this, testableAmendments()}; + Env env{*this, args.features}; Account const issuer{"issuer"}; Account const owner{"owner"}; Account const depositor{"depositor"}; @@ -1632,6 +1633,8 @@ class Vault_test : public beast::unit_test::Suite env(tx, Ter(tecNO_ENTRY)); }); + // Freeze/lock tests are in testVaultDepositFreeze/testVaultWithdrawFreeze + testCase([this]( Env& env, Account const& issuer, @@ -1646,81 +1649,6 @@ class Vault_test : public beast::unit_test::Suite env(tx, Ter(tecLOCKED)); }); - testCase([this]( - Env& env, - Account const& issuer, - Account const& owner, - Account const& depositor, - Asset const& asset, - Vault& vault, - MPTTester& mptt) { - testcase("MPT global lock blocks deposit"); - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(tx); - env.close(); - - mptt.set({.account = issuer, .flags = tfMPTLock}); - env.close(); - - tx = vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(100)}); - env(tx, Ter{tecLOCKED}); - env.close(); - - // Can delete empty vault, even if global lock - tx = vault.del({.owner = owner, .id = keylet.key}); - env(tx); - }); - - testCase([this]( - Env& env, - Account const& issuer, - Account const& owner, - Account const& depositor, - Asset const& asset, - Vault& vault, - MPTTester& mptt) { - testcase("MPT global lock blocks withdrawal"); - 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(); - - // Check that the OutstandingAmount field of MPTIssuance - // accounts for the issued shares. - auto v = env.le(keylet); - BEAST_EXPECT(v); - MPTID const share = (*v)[sfShareMPTID]; - auto issuance = env.le(keylet::mptokenIssuance(share)); - BEAST_EXPECT(issuance); - Number const outstandingShares = issuance->at(sfOutstandingAmount); - BEAST_EXPECT(outstandingShares == 100); - - mptt.set({.account = issuer, .flags = tfMPTLock}); - env.close(); - - 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 - tx = vault.clawback( - {.issuer = issuer, .id = keylet.key, .holder = depositor, .amount = asset(0)}); - env(tx); - env.close(); - - // Clawback removed shares MPToken - auto const mptSle = env.le(keylet::mptoken(share, depositor.id())); - BEAST_EXPECT(mptSle == nullptr); - - // Can delete empty vault, even if global lock - tx = vault.del({.owner = owner, .id = keylet.key}); - env(tx); - }); - testCase([this]( Env& env, Account const& issuer, @@ -2155,57 +2083,6 @@ class Vault_test : public beast::unit_test::Suite env(vault.del({.owner = owner, .id = keylet.key})); }); - testCase([this]( - Env& env, - Account const& issuer, - Account const& owner, - Account const& depositor, - Asset const& asset, - Vault& vault, - MPTTester& mptt) { - testcase("MPT lock of vault pseudo-account"); - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(tx); - env.close(); - - auto const vaultAccount = [&env, keylet = keylet, this]() -> AccountID { - auto const vault = env.le(keylet); - BEAST_EXPECT(vault != nullptr); - return vault->at(sfAccount); - }(); - - tx = vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(100)}); - env(tx); - env.close(); - - tx = [&]() { - json::Value jv; - jv[jss::Account] = issuer.human(); - jv[sfMPTokenIssuanceID] = to_string(asset.get().getMptID()); - jv[jss::Holder] = toBase58(vaultAccount); - jv[jss::TransactionType] = jss::MPTokenIssuanceSet; - jv[jss::Flags] = tfMPTLock; - return jv; - }(); - env(tx); - env.close(); - - tx = vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(100)}); - env(tx, Ter(tecLOCKED)); - - tx = vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(100)}); - env(tx, Ter(tecLOCKED)); - - // Clawback works, even when locked - tx = vault.clawback( - {.issuer = issuer, .id = keylet.key, .holder = depositor, .amount = asset(100)}); - env(tx); - - // Can delete an empty vault even when asset is locked. - tx = vault.del({.owner = owner, .id = keylet.key}); - env(tx); - }); - { testcase("MPT shares to a vault"); @@ -2438,6 +2315,7 @@ class Vault_test : public beast::unit_test::Suite Number initialIOU = 200; double transferRate = 1.0; bool charlieRipple = true; + FeatureBitset features = testableAmendments(); }; auto testCase = [&, this]( @@ -2451,7 +2329,7 @@ class Vault_test : public beast::unit_test::Suite PrettyAsset const& asset, std::function issuanceId)> test, CaseArgs args = {}) { - Env env{*this, testableAmendments()}; + Env env{*this, args.features}; Account const owner{"owner"}; Account const issuer{"issuer"}; Account const charlie{"charlie"}; @@ -2543,83 +2421,6 @@ class Vault_test : public beast::unit_test::Suite 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 trust line to vault account"); - - 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 trustline to the vault - 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(); - - { - // Note, the "frozen" state of the trust line to vault account - // is reported as "locked" state of the vault shares, because - // this state is attached to shares by means of the transitive - // isFrozen. - auto tx = - vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(80)}); - env(tx, Ter{tecLOCKED}); - } - - { - auto tx = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(100)}); - env(tx, Ter{tecLOCKED}); - - // also when trying to withdraw to a 3rd party - tx[sfDestination] = charlie.human(); - env(tx, Ter{tecLOCKED}); - env.close(); - } - - { - // Clawback works, even when locked - auto tx = vault.clawback( - {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(50)}); - env(tx); - env.close(); - } - - // Clear the frozen state - 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, @@ -2681,65 +2482,6 @@ class Vault_test : public beast::unit_test::Suite }, CaseArgs{.transferRate = 1.25}); - testCase([&, this]( - Env& env, - Account const& owner, - Account const& issuer, - Account const& charlie, - auto, - Vault& vault, - PrettyAsset const& asset, - auto&&...) { - testcase("IOU frozen trust line to depositor"); - - 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(); - - // Withdraw to 3rd party works - auto const withdrawToCharlie = [&](xrpl::Keylet keylet) { - auto tx = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - tx[sfDestination] = charlie.human(); - return tx; - }(keylet); - env(withdrawToCharlie); - - // Freeze the owner - env(trust(issuer, asset(0), owner, tfSetFreeze)); - env.close(); - - // Cannot withdraw - auto const withdraw = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - env(withdraw, Ter{tecFROZEN}); - - // Cannot withdraw to 3rd party - env(withdrawToCharlie, Ter{tecLOCKED}); - env.close(); - - { - // Cannot deposit some more - auto tx = - vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - env(tx, Ter{tecFROZEN}); - } - - { - // Clawback still works - auto tx = vault.clawback( - {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(0)}); - env(tx); - env.close(); - } - - env(vault.del({.owner = owner, .id = keylet.key})); - env.close(); - }); - testCase([&, this]( Env& env, Account const& owner, @@ -3031,102 +2773,6 @@ class Vault_test : public beast::unit_test::Suite env.close(); }, CaseArgs{.initialXRP = acctReserve + (incReserve * 4) + 1}); - - testCase([&, this]( - Env& env, - Account const& owner, - Account const& issuer, - Account const& charlie, - auto, - Vault& vault, - PrettyAsset const& asset, - auto&&...) { - testcase("IOU frozen 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(); - - // Withdraw to 3rd party works - auto const withdrawToCharlie = [&](xrpl::Keylet keylet) { - auto tx = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - tx[sfDestination] = charlie.human(); - return tx; - }(keylet); - env(withdrawToCharlie); - - // Freeze the 3rd party - env(trust(issuer, asset(0), charlie, tfSetFreeze)); - env.close(); - - // Can withdraw - auto const withdraw = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - env(withdraw); - env.close(); - - // Cannot withdraw to 3rd party - env(withdrawToCharlie, Ter{tecFROZEN}); - 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, - Vault& vault, - PrettyAsset const& asset, - auto&&...) { - testcase("IOU global freeze"); - - 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(); - - env(fset(issuer, asfGlobalFreeze)); - env.close(); - - { - // Cannot withdraw - auto tx = - vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - env(tx, Ter{tecFROZEN}); - - // Cannot withdraw to 3rd party - tx[sfDestination] = charlie.human(); - env(tx, Ter{tecFROZEN}); - env.close(); - - // Cannot deposit some more - tx = vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(10)}); - - env(tx, Ter{tecFROZEN}); - } - - // Clawback is permitted - 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(); - }); } void @@ -7824,6 +7470,579 @@ class Vault_test : public beast::unit_test::Suite } } + void + testVaultDepositFreeze() + { + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + + // === IOU === + { + testcase("VaultDeposit IOU freeze checks"); + Env env{*this}; + Vault vault{env}; + + env.fund(XRP(100'000), issuer, owner); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1'000'000), owner); + env(pay(issuer, owner, asset(100'000))); + env.close(); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); + + // Initial deposit so the vault pseudo-account has a trustline + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Global freeze + env(fset(issuer, asfGlobalFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(tecFROZEN)); + env(fclear(issuer, asfGlobalFreeze)); + + // Depositor regular freeze + env(trust(issuer, asset(0), owner, tfSetFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(tecFROZEN)); + env(trust(issuer, asset(0), owner, tfClearFreeze)); + + // Depositor deep freeze + env(trust(issuer, asset(0), owner, tfSetFreeze | tfSetDeepFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(tecFROZEN)); + env(trust(issuer, asset(0), owner, tfClearFreeze | tfClearDeepFreeze)); + + // Vault-account regular freeze + // Post-fix: checkDepositFreeze catches it → tecFROZEN + // Pre-fix: not checked directly, but the transitive share + // check triggers → tecLOCKED + { + 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; + return jv; + }(); + + trustSet[jss::Flags] = tfSetFreeze; + env(trustSet); + env.close(); + + TER const expected = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(expected)); + + trustSet[jss::Flags] = tfClearFreeze; + env(trustSet); + env.close(); + } + + // Vault-account deep freeze + { + 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; + return jv; + }(); + + trustSet[jss::Flags] = tfSetFreeze | tfSetDeepFreeze; + env(trustSet); + env.close(); + + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED))); + + trustSet[jss::Flags] = tfClearFreeze | tfClearDeepFreeze; + env(trustSet); + env.close(); + } + + // Clawback works while frozen + env(fset(issuer, asfGlobalFreeze)); + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(1)})); + env(fclear(issuer, asfGlobalFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + // === MPT === + { + testcase("VaultDeposit MPT lock checks"); + Env env{*this}; + Vault vault{env}; + + env.fund(XRP(100'000), issuer, owner); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); + PrettyAsset const mpt{mptt.issuanceID()}; + + mptt.authorize({.account = owner}); + mptt.authorize({.account = issuer, .holder = owner}); + env.close(); + env(pay(issuer, owner, mpt(100'000))); + env.close(); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); + env(tx); + env.close(); + auto const vaultAcctID = env.le(keylet)->at(sfAccount); + Account const vaultAcct("vault", vaultAcctID); + + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); + env.close(); + + // For MPT isDeepFrozen == isFrozen, so all locks block in + // both pre- and post-fix. + auto runTests = [&]() { + // Global lock + mptt.set({.flags = tfMPTLock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + mptt.set({.flags = tfMPTUnlock}); + env.close(); + + // Depositor individual lock + mptt.set({.holder = owner, .flags = tfMPTLock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + mptt.set({.holder = owner, .flags = tfMPTUnlock}); + env.close(); + + // Vault pseudo-account individual lock + mptt.set({.holder = vaultAcct, .flags = tfMPTLock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + mptt.set({.holder = vaultAcct, .flags = tfMPTUnlock}); + env.close(); + + // Clawback works while locked + mptt.set({.flags = tfMPTLock}); + env.close(); + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = mpt(1)})); + mptt.set({.flags = tfMPTUnlock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + } + + // Focused demonstration: a depositor under a regular individual IOU freeze + // can still withdraw to themselves (self-withdrawal), but is blocked from + // withdrawing to a third party. + // + // Pre-fixCleanup3_3_0: both the self-withdrawal AND the third-party + // withdrawal were blocked because the old code checked checkFrozen on the + // destination regardless of whether it was the submitter. + // Post-fixCleanup3_3_0: checkWithdrawFreeze skips the submitter freeze + // check when submitter == destination, so self-withdrawal succeeds. + void + testVaultSelfWithdrawWhileFrozen() + { + testcase("VaultWithdraw IOU self-withdrawal while individually frozen"); + + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const charlie{"charlie"}; + Env env{*this}; + Vault vault{env}; + + env.fund(XRP(100'000), issuer, owner, charlie); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1'000'000), owner); + env.trust(asset(1'000'000), charlie); + env(pay(issuer, owner, asset(100'000))); + 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(10)})); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Set a regular individual freeze on the owner's IOU trustline. + env(trust(issuer, asset(0), owner, tfSetFreeze)); + env.close(); + + // Self-withdrawal: submitter == destination, so the submitter + // freeze check is skipped. + // Post-fix: tesSUCCESS. Pre-fix: tecFROZEN. + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); + + // Withdrawal to a third party is blocked: submitter != destination + // so the submitter freeze check applies. + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + // Post-fix: tecFROZEN (checkIndividualFrozen on submitter). + // Pre-fix: tecLOCKED (isFrozen on the vault share). + env(withdrawToCharlie, Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED))); + } + + env(trust(issuer, asset(0), owner, tfClearFreeze)); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + void + testVaultWithdrawFreeze() + { + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + + // === IOU === + { + testcase("VaultWithdraw IOU freeze checks"); + Env env{*this}; + Vault vault{env}; + + env.fund(XRP(100'000), issuer, owner); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env.trust(asset(1'000'000), owner); + env(pay(issuer, owner, asset(100'000))); + env.close(); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + auto const vaultAcct = Account("vault", env.le(keylet)->at(sfAccount)); + + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(100)})); + env.close(); + + Account const charlie{"charlie"}; + env.fund(XRP(10'000), charlie); + env.trust(asset(1'000'000), charlie); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + // Post-fix: submitter freeze blocks withdraw to 3rd party + // Pre-fix: submitter's IOU freeze not checked, but + // checkFrozen(depositor, share) may trigger tecLOCKED + TER const submitterTo3rd = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); + + // Global freeze → self-withdraw + env(fset(issuer, asfGlobalFreeze)); + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(tecFROZEN)); + // Global freeze → withdraw to 3rd party + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(tecFROZEN)); + } + env(fclear(issuer, asfGlobalFreeze)); + + // Vault-account regular freeze + { + 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; + return jv; + }(); + + trustSet[jss::Flags] = tfSetFreeze; + env(trustSet); + env.close(); + + TER const vaultAcctFreeze = fix330Enabled ? TER(tecFROZEN) : TER(tecLOCKED); + + // Self-withdraw + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(vaultAcctFreeze)); + // Withdraw to 3rd party + { + auto withdrawToCharlie = vault.withdraw( + {.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(vaultAcctFreeze)); + } + + trustSet[jss::Flags] = tfClearFreeze; + env(trustSet); + env.close(); + } + + // Depositor regular freeze → self-withdraw + env(trust(issuer, asset(0), owner, tfSetFreeze)); + // Post-fix: self-withdraw allowed (submitter==dst skip) + // Pre-fix: isFrozen(depositor, iou) catches it + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); + + // Depositor regular freeze → withdraw to 3rd party + { + auto withdrawTo3rd = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawTo3rd[sfDestination] = charlie.human(); + env(withdrawTo3rd, Ter(submitterTo3rd)); + } + env(trust(issuer, asset(0), owner, tfClearFreeze)); + // Replenish what was withdrawn + if (fix330Enabled) + { + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + } + env.close(); + + // Depositor deep freeze → self-withdraw blocked + env(trust(issuer, asset(0), owner, tfSetFreeze | tfSetDeepFreeze)); + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}), + Ter(fix330Enabled ? TER(tecFROZEN) : TER(tecFROZEN))); + env(trust(issuer, asset(0), owner, tfClearFreeze | tfClearDeepFreeze)); + + // Destination regular freeze → withdraw to 3rd party + env(trust(issuer, asset(0), charlie, tfSetFreeze)); + // Self-withdraw unaffected by charlie's freeze + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + // Post-fix: regular freeze on dst allowed + // Pre-fix: checkFrozen(dst, iou) catches it + env(withdrawToCharlie, Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecFROZEN))); + } + env(trust(issuer, asset(0), charlie, tfClearFreeze)); + // Replenish: 1 for self-withdraw + 1 if charlie withdraw succeeded + env(vault.deposit( + {.depositor = owner, + .id = keylet.key, + .amount = asset(fix330Enabled ? 2 : 1)})); + env.close(); + + // Destination deep freeze → withdraw to 3rd party blocked + env(trust(issuer, asset(0), charlie, tfSetFreeze | tfSetDeepFreeze)); + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(tecFROZEN)); + } + // Destination deep freeze → self-withdraw unaffected + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + env(trust(issuer, asset(0), charlie, tfClearFreeze | tfClearDeepFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + env.close(); + + // Clawback works while frozen + env(fset(issuer, asfGlobalFreeze)); + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = asset(1)})); + env(fclear(issuer, asfGlobalFreeze)); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = asset(1)})); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + + // === MPT === + { + testcase("VaultWithdraw MPT lock checks"); + Env env{*this}; + Vault vault{env}; + + env.fund(XRP(100'000), issuer, owner); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth}); + PrettyAsset const mpt{mptt.issuanceID()}; + + mptt.authorize({.account = owner}); + mptt.authorize({.account = issuer, .holder = owner}); + env.close(); + env(pay(issuer, owner, mpt(100'000))); + env.close(); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = mpt}); + env(tx); + env.close(); + Account const vaultAcct("vault", env.le(keylet)->at(sfAccount)); + + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(100)})); + env.close(); + + Account const charlie{"charlie"}; + env.fund(XRP(10'000), charlie); + env.close(); + mptt.authorize({.account = charlie}); + mptt.authorize({.account = issuer, .holder = charlie}); + env.close(); + + auto runTests = [&]() { + auto const fix330Enabled = env.current()->rules().enabled(fixCleanup3_3_0); + + // Global lock + mptt.set({.flags = tfMPTLock}); + env.close(); + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + + // Global lock → withdraw to issuer + // Post-fix: bypasses freeze checks, but accountHolds + // on the pseudo returns 0 under global lock + // Pre-fix: checkFrozen(dst=issuer) catches global lock + { + auto withdrawToIssuer = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}); + withdrawToIssuer[sfDestination] = issuer.human(); + env(withdrawToIssuer, Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecLOCKED))); + } + mptt.set({.flags = tfMPTUnlock}); + env.close(); + if (fix330Enabled) + { + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + } + env.close(); + + // Vault pseudo-account individual lock + mptt.set({.holder = vaultAcct, .flags = tfMPTLock}); + env.close(); + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + mptt.set({.holder = vaultAcct, .flags = tfMPTUnlock}); + env.close(); + + // Depositor individual lock → self-withdraw blocked + // (isDeepFrozen == isFrozen for MPT) + mptt.set({.holder = owner, .flags = tfMPTLock}); + env.close(); + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}), + Ter(tecLOCKED)); + // Depositor lock → withdraw to 3rd party also blocked + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter(tecLOCKED)); + } + + // Depositor lock → withdraw to issuer + // Post-fix: issuer bypass in checkWithdrawFreezes + // Pre-fix: checkFrozen(depositor, share) blocks transitively + { + auto withdrawToIssuer = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}); + withdrawToIssuer[sfDestination] = issuer.human(); + env(withdrawToIssuer, Ter(fix330Enabled ? TER(tesSUCCESS) : TER(tecLOCKED))); + } + mptt.set({.holder = owner, .flags = tfMPTUnlock}); + env.close(); + if (fix330Enabled) + { + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + } + env.close(); + + // 3rd party destination lock → withdraw to 3rd party blocked + mptt.set({.holder = charlie, .flags = tfMPTLock}); + env.close(); + { + auto withdrawToCharlie = + vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)}); + withdrawToCharlie[sfDestination] = charlie.human(); + env(withdrawToCharlie, Ter{tecLOCKED}); + } + // 3rd party lock → self-withdraw unaffected + env(vault.withdraw({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + mptt.set({.holder = charlie, .flags = tfMPTUnlock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + env.close(); + + // Clawback works while locked + mptt.set({.flags = tfMPTLock}); + env.close(); + env(vault.clawback( + {.issuer = issuer, .id = keylet.key, .holder = owner, .amount = mpt(1)})); + mptt.set({.flags = tfMPTUnlock}); + env.close(); + env(vault.deposit({.depositor = owner, .id = keylet.key, .amount = mpt(1)})); + env.close(); + }; + + runTests(); + env.disableFeature(fixCleanup3_3_0); + runTests(); + env.enableFeature(fixCleanup3_3_0); + } + } + public: void run() override @@ -7864,6 +8083,10 @@ public: testWithdrawSoleShareholderPartialFixedSharesUsesFullPrice(); testWithdrawSoleShareholderLoanRepaymentExit(); + testVaultDepositFreeze(); + testVaultWithdrawFreeze(); + testVaultSelfWithdrawWhileFrozen(); + testReferenceHolding(); testHoldingDeletionBlocked(); } diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index 3d813d993c..aca6074c4a 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -96,6 +96,26 @@ testableAmendments() return kIds; } +/** + * Returns all 2^N permutations of a seed FeatureBitset with each subset of + * the given features excluded. The seed is included as the first element. + * + * Useful for running a test over every combination of optional amendments + * so that each case is exercised both with and without each feature. + */ +inline std::vector +amendmentCombinations(std::initializer_list features, FeatureBitset seed) +{ + std::vector result{seed}; + 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; +} + //------------------------------------------------------------------------------ class SuiteLogs : public Logs diff --git a/src/test/jtx/impl/utility.cpp b/src/test/jtx/impl/utility.cpp index 9256242417..7da419c6e7 100644 --- a/src/test/jtx/impl/utility.cpp +++ b/src/test/jtx/impl/utility.cpp @@ -100,5 +100,4 @@ cmdToJSONRPC(std::vector const& args, beast::Journal j, unsigned in jv[jss::id] = paramsObj[jss::id]; return jv; } - } // namespace xrpl::test::jtx diff --git a/src/test/jtx/utility.h b/src/test/jtx/utility.h index 535e14cf1d..c3ed91f672 100644 --- a/src/test/jtx/utility.h +++ b/src/test/jtx/utility.h @@ -7,6 +7,7 @@ #include #include +#include namespace xrpl::test::jtx { @@ -50,5 +51,4 @@ fillSeq(json::Value& jv, ReadView const& view); /** Given an xrpld unit test rpc command, return the corresponding JSON. */ json::Value cmdToJSONRPC(std::vector const& args, beast::Journal j, unsigned int apiVersion); - } // namespace xrpl::test::jtx