ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-31 06:05:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'origin/develop' into tapanito/vault-precision-transactor

Browse Source
This commit is contained in:
Vito
2026-08-24 16:12:06 +02:00
parent 67b0991fbc 520650081b
commit 37dbfc0447
18 changed files with 833 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.cspell.config.yaml
View File

@@ -366,6 +366,7 @@ words:
- venv
- vfalco
- vinnie
- vkeylet
- wasmi
- wextra
- wptr

27
include/xrpl/ledger/helpers/CredentialHelpers.h
View File

@@ -14,6 +14,7 @@
#include <xrpl/protocol/STVector256.h>
#include <xrpl/protocol/TER.h>
#include <cstdint>
#include <memory>
#include <set>
#include <utility>
@@ -33,6 +34,32 @@ checkExpired(SLE const& sleCredential, NetClock::time_point const& closed);
[[nodiscard]] TER
deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j);
/**
* @brief Remove credentials pinned to a pseudo-account's owner directory.
*
* Cleans up credentials that were linked to a pseudo-account (Vault, LoanBroker,
* AMM), which such an account can neither accept nor delete. Only credentials
* are removed; every other object is left in place. The walk visits at most
* @p maxNodesToDelete directory entries and charges the ones it leaves alone
* against that budget too, so a directory holding other objects yields fewer
* than @p maxNodesToDelete deletions. On reaching the bound the result is
* `tecINCOMPLETE` and the caller must propagate it so a later transaction
* resumes.
*
* @param view Mutable ledger view.
* @param pseudoAcct The pseudo-account whose directory is cleaned.
* @param maxNodesToDelete Upper bound on directory entries processed in one call.
* @param j Journal for diagnostics.
* @return tesSUCCESS once no credentials remain, tecINCOMPLETE if the bound was
* reached, or a deletion error.
*/
[[nodiscard]] TER
deletePseudoAccountCredentials(
ApplyView& view,
AccountID const& pseudoAcct,
std::uint16_t maxNodesToDelete,
beast::Journal j);
// Amendment and parameters checks for sfCredentialIDs field
NotTEC
checkFields(STTx const& tx, Rules const& rules, beast::Journal j);

37
include/xrpl/ledger/helpers/VaultHelpers.h
View File

@@ -7,6 +7,7 @@
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/TER.h>
#include <cstdint>
#include <optional>
@@ -261,4 +262,40 @@ getVaultPhase(
std::optional<std::uint32_t> subscriptionDate,
std::optional<std::uint32_t> redemptionDate);
/**
* Controls whether checkVaultDomain reports an expired credential as an
* error. A caller that deletes expired credentials later, in doApply, passes
* Yes and treats the subject as authorized; a caller with no such cleanup
* step must keep the error.
*/
enum class SuppressExpired : bool { No = false, Yes = true };
/**
* Checks that subject belongs to the permissioned domain governing a vault's
* shares.
*
* The domain is read from the share issuance rather than from the vault. Vault
* shares are issued by the vault's pseudo-account, which cannot grant an
* authorization explicitly, so domain membership is the only route to being
* authorized: a vault with no domain set has no authorized participants at
* all, and every subject fails with tecNO_AUTH.
*
* Which accounts to check, and whether to check at all, is left to the caller.
* This says nothing about vault privacy or about the roles of the accounts.
*
* @param view The ledger view.
* @param issuance The MPTokenIssuance SLE for the vault's shares.
* @param subject The account whose domain membership is checked.
* @param suppressExpired Whether an expired credential counts as authorized.
*
* @return tesSUCCESS if the subject is a domain member, otherwise the reason
* it is not.
*/
[[nodiscard]] TER
checkVaultDomain(
ReadView const& view,
SLE::const_ref issuance,
AccountID const& subject,
SuppressExpired suppressExpired);
} // namespace xrpl

10
include/xrpl/protocol/Protocol.h
View File

@@ -396,6 +396,16 @@ using TxID = uint256;
*/
constexpr std::uint16_t kMaxDeletableAmmTrustLines = 512;
/**
* The maximum number of owner-directory entries to walk when clearing
* credentials pinned to a pseudo-account, in a single transaction.
*
* The walk stops after this many entries whether or not each one turns out to
* be a credential, so a directory that also holds other objects yields fewer
* deletions per transaction.
*/
constexpr std::uint16_t kMaxDeletablePseudoAccountCredentials = 512;
/**
* The maximum length of a URI inside an Oracle
*/

14
src/libxrpl/ledger/helpers/AMMHelpers.cpp
View File

@@ -11,6 +11,7 @@
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/Sandbox.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/ledger/helpers/RippleStateHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
@@ -690,6 +691,12 @@ deleteAMMTrustLines(
return {deleteAMMTrustLine(sb, sleItem, ammAccountID, j), SkipEntry::No};
}
// A credential naming the pseudo-account as subject can't be
// accepted or deleted by it and would otherwise permanently pin the
// AMM. Clean it up here, inside the same bounded walk, so the
// pinned AMM can still be deleted.
if (sb.rules().enabled(fixCleanup3_4_0) && nodeType == ltCREDENTIAL)
return {credentials::deleteSLE(sb, sleItem, j), SkipEntry::No};
// LCOV_EXCL_START
JLOG(j.error()) << "deleteAMMObjects: deleting non-trustline or non-MPT " << nodeType;
return {tecINTERNAL, SkipEntry::No};
@@ -767,6 +774,8 @@ deleteAMMAccount(Sandbox& sb, Asset const& asset, Asset const& asset2, beast::Jo
// LCOV_EXCL_STOP
}
// deleteAMMTrustLines also removes any credentials pinned to the AMM
// pseudo-account, within its bounded walk.
if (auto const ter = deleteAMMTrustLines(sb, ammAccountID, kMaxDeletableAmmTrustLines, j);
!isTesSuccess(ter))
return ter;
@@ -908,6 +917,11 @@ isOnlyLiquidityProvider(ReadView const& view, Issue const& ammIssue, AccountID c
++nMPT;
continue;
}
// A credential naming the pseudo-account as subject can be pinned
// to its owner directory. Ignore it here; deleteAMMTrustLines
// removes it when the AMM is deleted.
if (view.rules().enabled(fixCleanup3_4_0) && entryType == ltCREDENTIAL)
continue;
if (entryType != ltRIPPLE_STATE)
return std::unexpected<TER>(tecINTERNAL); // LCOV_EXCL_LINE
auto const lowLimit = sle->getFieldAmount(sfLowLimit);

32
src/libxrpl/ledger/helpers/CredentialHelpers.cpp
View File

@@ -5,8 +5,10 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/basics/chrono.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
@@ -127,6 +129,36 @@ deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j)
return tesSUCCESS;
}
TER
deletePseudoAccountCredentials(
ApplyView& view,
AccountID const& pseudoAcct,
std::uint16_t maxNodesToDelete,
beast::Journal j)
{
XRPL_ASSERT(
isPseudoAccount(view.read(keylet::account(pseudoAcct))),
"xrpl::credentials::deletePseudoAccountCredentials : is a pseudo-account");
// Delete the credentials linked into the pseudo-account's owner directory,
// visiting at most maxNodesToDelete entries. Any other object is left in
// place; the caller's own checks decide whether the remaining directory
// blocks deletion. If the bound is reached, cleanupOnAccountDelete returns
// tecINCOMPLETE and the caller propagates it so a later transaction resumes.
return cleanupOnAccountDelete(
view,
keylet::ownerDir(pseudoAcct),
[&view, &j](LedgerEntryType nodeType, uint256 const&, SLE::pointer& sleItem)
-> std::pair<TER, SkipEntry> {
if (nodeType == ltCREDENTIAL)
return {deleteSLE(view, sleItem, j), SkipEntry::No};
return {tesSUCCESS, SkipEntry::Yes};
},
j,
maxNodesToDelete);
}
NotTEC
checkFields(STTx const& tx, Rules const& rules, beast::Journal j)
{

24
src/libxrpl/ledger/helpers/VaultHelpers.cpp
View File

@@ -4,6 +4,7 @@
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/LedgerFormats.h> // IWYU pragma: keep
@@ -13,6 +14,7 @@
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/STNumber.h> // IWYU pragma: keep
#include <xrpl/protocol/STTx.h>
#include <xrpl/protocol/TER.h>
#include <cstdint>
#include <optional>
@@ -258,4 +260,26 @@ getVaultPhase(
return VaultPhase::Redemption;
}
[[nodiscard]] TER
checkVaultDomain(
ReadView const& view,
SLE::const_ref issuance,
AccountID const& subject,
SuppressExpired suppressExpired)
{
XRPL_ASSERT(
issuance && issuance->getType() == ltMPTOKEN_ISSUANCE,
"xrpl::checkVaultDomain : valid issuance SLE");
auto const maybeDomainID = issuance->at(~sfDomainID);
if (!maybeDomainID)
return tecNO_AUTH;
auto const err = credentials::validDomain(view, *maybeDomainID, subject);
if (err == tecEXPIRED && suppressExpired == SuppressExpired::Yes)
return tesSUCCESS;
return err;
}
} // namespace xrpl

14
src/libxrpl/tx/Transactor.cpp
View File

@@ -1246,7 +1246,7 @@ removeExpiredNFTokenOffers(
}
static void
removeExpiredCredentials(ApplyView& view, std::vector<uint256> const& creds, beast::Journal viewJ)
removeDeletedCredentials(ApplyView& view, std::vector<uint256> const& creds, beast::Journal viewJ)
{
for (auto const& index : creds)
{
@@ -1255,7 +1255,7 @@ removeExpiredCredentials(ApplyView& view, std::vector<uint256> const& creds, bea
if (auto const ter = credentials::deleteSLE(view, sle, viewJ); !isTesSuccess(ter))
{
JLOG(viewJ.error())
<< "removeExpiredCredentials: failed to delete expired credential. Err: "
<< "removeDeletedCredentials: failed to delete credential. Err: "
<< transToken(ter);
}
}
@@ -1437,7 +1437,8 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
// should be used, making it possible to do more useful work
// when transactions fail with a `tec` code.
auto typesForResult = [](TER const ter) {
auto typesForResult = [credentialCleanup =
view().rules().enabled(fixCleanup3_4_0)](TER const ter) {
std::unordered_set<LedgerEntryType> types;
if ((ter == tecOVERSIZE) || (ter == tecKILLED))
{
@@ -1446,6 +1447,11 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
else if (ter == tecINCOMPLETE)
{
types.insert(ltRIPPLE_STATE);
// A bounded pseudo-account credential cleanup (VaultDelete /
// LoanBrokerDelete) persists its partial credential deletions so a
// later transaction can resume.
if (credentialCleanup)
types.insert(ltCREDENTIAL);
}
else if (ter == tecEXPIRED)
{
@@ -1523,7 +1529,7 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
removeDeletedTrustLines(view(), ids, viewJ);
break;
case ltCREDENTIAL:
removeExpiredCredentials(view(), ids, viewJ);
removeDeletedCredentials(view(), ids, viewJ);
break;
// LCOV_EXCL_START
default:

8
src/libxrpl/tx/invariants/MPTInvariant.cpp
View File

@@ -234,6 +234,14 @@ ValidMPTIssuance::finalize(
if (hasPrivilege(tx, Privilege::DestroyMptIssuance))
{
// A VaultDelete that is still cleaning up credentials pinned to its
// pseudo-account returns tecINCOMPLETE and has not yet reached the
// share issuance. Don't require the issuance to be removed until
// the deletion completes (a later transaction).
if (rules.enabled(fixCleanup3_4_0) && txnType == ttVAULT_DELETE &&
result == tecINCOMPLETE)
return mptIssuancesDeleted_ == 0 && mptIssuancesCreated_ == 0;
if (mptIssuancesDeleted_ == 0)
{
JLOG(j.fatal()) << "Invariant failed: MPT issuance deletion "

15
src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp
View File

@@ -4,11 +4,13 @@
#include <xrpl/basics/Number.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/LendingHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STLedgerEntry.h>
@@ -140,6 +142,19 @@ LoanBrokerDelete::doApply()
auto const brokerPseudoID = broker->at(sfAccount);
// Remove any credentials pinned to the broker pseudo-account before anything
// else. They would otherwise keep its owner directory alive and block
// deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded,
// tecINCOMPLETE cleanup can be resumed by a later transaction without having
// already torn down the broker.
if (view().rules().enabled(fixCleanup3_4_0))
{
if (auto const ter = credentials::deletePseudoAccountCredentials(
view(), brokerPseudoID, kMaxDeletablePseudoAccountCredentials, j_);
!isTesSuccess(ter))
return ter;
}
if (!view().dirRemove(
keylet::ownerDir(accountID_), broker->at(sfOwnerNode), broker->key(), false))
{

14
src/libxrpl/tx/transactors/vault/VaultDelete.cpp
View File

@@ -4,6 +4,7 @@
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/AccountID.h>
@@ -100,6 +101,19 @@ VaultDelete::doApply()
if (!vault)
return tefINTERNAL; // LCOV_EXCL_LINE
// Remove any credentials pinned to the vault pseudo-account before anything
// else. They would otherwise keep its owner directory alive and block
// deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded,
// tecINCOMPLETE cleanup can be resumed by a later transaction without having
// already torn down the vault.
if (view().rules().enabled(fixCleanup3_4_0))
{
if (auto const ter = credentials::deletePseudoAccountCredentials(
view(), vault->at(sfAccount), kMaxDeletablePseudoAccountCredentials, j_);
!isTesSuccess(ter))
return ter;
}
// Destroy the asset holding.
auto asset = vault->at(sfAsset);

24
src/libxrpl/tx/transactors/vault/VaultDeposit.cpp
View File

@@ -6,7 +6,6 @@
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/ledger/helpers/VaultHelpers.h>
@@ -175,26 +174,13 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
return tecLOCKED;
}
// The vault owner is authorized to deposit unconditionally. An expired
// credential is tolerated here because doApply deletes it.
if (vault->isFlag(lsfVaultPrivate) && account != vault->at(sfOwner))
{
auto const maybeDomainID = sleIssuance->at(~sfDomainID);
// Since this is a private vault and the account is not its owner, we
// perform authorization check based on DomainID read from sleIssuance.
// Had the vault shares been a regular MPToken, we would allow
// authorization granted by the Issuer explicitly, but Vault uses Issuer
// pseudo-account, which cannot grant an authorization.
if (maybeDomainID)
{
// As per validDomain documentation, we suppress tecEXPIRED error
// here, so we can delete any expired credentials inside doApply.
if (auto const err = credentials::validDomain(ctx.view, *maybeDomainID, account);
!isTesSuccess(err) && err != tecEXPIRED)
return err;
}
else
{
return tecNO_AUTH;
}
if (auto const err = checkVaultDomain(ctx.view, sleIssuance, account, SuppressExpired::Yes);
!isTesSuccess(err))
return err;
}
// Source MPToken must exist (if asset is an MPT)

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

@@ -7,6 +7,7 @@
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/ledger/helpers/VaultHelpers.h>
@@ -80,6 +81,7 @@ 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 fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0);
auto const vault = ctx.view.read(keylet::vault(ctx.tx[sfVaultID]));
if (!vault)
@@ -131,6 +133,17 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
if (auto const err = credentials::valid(ctx.tx, ctx.view, account, ctx.j); !isTesSuccess(err))
return err;
// A pseudo-account belongs to a ledger object rather than to a person and
// must never receive funds from a user-initiated transaction. Deposit
// authorization, which every pseudo-account carries, already refuses the
// payout, but it reports only that the destination declines deposits and
// leaves the real reason unsaid.
if (fix340Enabled && isPseudoAccount(ctx.view, dstAcct))
{
JLOG(ctx.j.debug()) << "VaultWithdraw: cannot withdraw into a pseudo-account.";
return tecPSEUDO_ACCOUNT;
}
if (fix313Enabled && amount.asset() == vaultShare)
{
// Post-fixCleanup3_1_3: if the user specified shares, convert
@@ -192,6 +205,39 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter))
return ter;
// The checks above only establish that an account may hold the asset. A
// private vault additionally restricts who may take part in it, so paying
// its asset out to a third party requires both ends of that payout to be
// inside the vault's permissioned domain. VaultDeposit applies the same
// domain check on the way in.
//
// Two cases deliberately skip the check. Withdrawing to self is never
// restricted: losing vault access must not strand funds already deposited.
// The asset issuer is always allowed to receive, which keeps the return
// path for frozen assets open even for a submitter who lost access.
if (fix340Enabled && vault->isFlag(lsfVaultPrivate) && dstAcct != account &&
dstAcct != vaultAsset.getIssuer())
{
auto const sleIssuance = ctx.view.read(keylet::mptokenIssuance(vaultShare));
if (!sleIssuance)
{
// LCOV_EXCL_START
JLOG(ctx.j.error()) << "VaultWithdraw: missing issuance of vault shares.";
return tefINTERNAL;
// LCOV_EXCL_STOP
}
// Unlike VaultDeposit we do not suppress tecEXPIRED: there is no
// doApply step here that would clean up the expired credential.
if (auto const ter = checkVaultDomain(ctx.view, sleIssuance, account, SuppressExpired::No);
!isTesSuccess(ter))
return ter;
if (auto const ter = checkVaultDomain(ctx.view, sleIssuance, dstAcct, SuppressExpired::No);
!isTesSuccess(ter))
return ter;
}
if (fix330Enabled)
{
// checkWithdrawFreeze checks the underlying asset on the source
@@ -241,7 +287,9 @@ VaultWithdraw::doApply()
// Note, we intentionally do not check lsfVaultPrivate flag on the Vault. If
// you have a share in the vault, it means you were at some point authorized
// to deposit into it, and this means you are also indefinitely authorized
// to withdraw from it.
// to withdraw it to yourself. Sending the proceeds to somebody else is a
// different matter, and preclaim checks such a withdrawal against the
// vault's permissioned domain.
auto const amount = ctx_.tx[sfAmount];
Asset const vaultAsset = vault->at(sfAsset);

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

@@ -4,6 +4,7 @@
#include <test/jtx/Env.h>
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/credentials.h>
#include <test/jtx/envconfig.h>
#include <test/jtx/escrow.h>
#include <test/jtx/fee.h>
@@ -5192,6 +5193,51 @@ private:
{features});
}
void
testCredentialPinsPseudoAccount()
{
testcase("Credential pins AMM pseudo-account");
using namespace jtx;
FeatureBitset const all{testableAmendments()};
// A credential issued to an AMM pseudo-account can't be accepted or
// deleted by it. A pin created before the cure activates stays pinned
// in the pseudo-account's owner directory and makes AMM deletion fail
// with tecINTERNAL (deleteAMMTrustLines rejects the unexpected
// directory entry).
Account const attacker{"attacker"};
char const credType[] = "FN36";
Env env(*this, all - fixCleanup3_3_0 - fixCleanup3_4_0);
fund(env, gw_, {alice_}, XRP(20'000), {USD(10'000)});
env.fund(XRP(1'000), attacker);
env.close();
AMM amm(env, alice_, XRP(10'000), USD(10'000));
Account const ammAcct{"amm pseudo-account", amm.ammAccount()};
env.memoize(ammAcct);
env(credentials::create(ammAcct, attacker, credType));
env.close();
auto const credKey = credentials::keylet(ammAcct, attacker, credType);
BEAST_EXPECT(env.le(credKey));
// Emptying the AMM would auto-delete it, but the pinned credential makes
// deleteAMMAccount fail; the withdraw is rolled back and the AMM stays.
amm.withdrawAll(alice_, std::nullopt, Ter(tecINTERNAL));
BEAST_EXPECT(amm.ammExists());
env.enableFeature(fixCleanup3_4_0);
env.close();
// The pre-existing pin is cleaned up and the AMM deletes.
amm.withdrawAll(alice_);
BEAST_EXPECT(!amm.ammExists());
BEAST_EXPECT(!env.le(credKey));
BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount())));
}
void
testAutoDelete()
{
@@ -7459,6 +7505,7 @@ private:
FeatureBitset const all{testableAmendments()};
testInvalidInstance();
testInstanceCreate();
testCredentialPinsPseudoAccount();
for (auto const& f : amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}))
testInvalidDeposit(f);
testDeposit();

123
src/test/app/lending/LoanBroker_test.cpp
View File

@@ -60,6 +60,7 @@
#include <functional>
#include <memory>
#include <optional>
#include <string>
#include <string_view>
#include <tuple>
#include <utility>
@@ -2871,6 +2872,126 @@ class LoanBroker_test : public beast::unit_test::Suite
runTestCases(all_ - fixCleanup3_2_0);
}
void
testCredentialPinsPseudoAccount()
{
using namespace test::jtx;
using namespace loan_broker;
// A credential issued to a LoanBroker pseudo-account can't be accepted
// or deleted by it, so it stays pinned in the pseudo-account's owner
// directory and blocks LoanBrokerDelete with tecHAS_OBLIGATIONS. A pin
// created before the cure activates is removed by LoanBrokerDelete once
// it does.
Account const alice{"alice"}; // vault & broker owner
Account const attacker{"attacker"};
char const credType[] = "FN36";
Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0};
env.fund(XRP(1'000'000), alice, attacker);
env.close();
Vault const vault{env};
auto [vtx, vkeylet] = vault.create({.owner = alice, .asset = xrpIssue()});
env(vtx);
env.close();
BEAST_EXPECT(env.le(vkeylet));
auto const brokerKeylet =
keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice)));
env(set(alice.id(), vkeylet.key));
env.close();
auto const broker = env.le(brokerKeylet);
BEAST_EXPECT(broker);
Account const pseudo{"broker pseudo-account", broker->at(sfAccount)};
env.memoize(pseudo);
testcase("Credential pins broker pseudo-account");
env(credentials::create(pseudo, attacker, credType));
env.close();
auto const credKey = credentials::keylet(pseudo, attacker, credType);
BEAST_EXPECT(env.le(credKey));
BEAST_EXPECT(ownerCount(env, attacker) == 1);
env(del(alice.id(), brokerKeylet.key), Ter(tecHAS_OBLIGATIONS));
env.close();
env.enableFeature(fixCleanup3_4_0);
env.close();
// The pre-existing pin no longer blocks deletion; the credential is
// cleaned up and the issuer's owner count is restored.
testcase("LoanBrokerDelete removes pinned credential");
env(del(alice.id(), brokerKeylet.key));
env.close();
BEAST_EXPECT(!env.le(credKey));
BEAST_EXPECT(!env.le(brokerKeylet));
BEAST_EXPECT(!env.le(keylet::account(pseudo.id())));
BEAST_EXPECT(ownerCount(env, attacker) == 0);
}
void
testCredentialPinOverflow()
{
using namespace test::jtx;
using namespace loan_broker;
testcase("Credential pin cleanup is bounded (tecINCOMPLETE)");
// A pseudo-account can be pinned with more credentials than one
// transaction is allowed to clean up. LoanBrokerDelete then removes
// them a bounded batch at a time, returning tecINCOMPLETE until the
// last batch.
Account const alice{"alice"};
Account const attacker{"attacker"};
Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0};
env.fund(XRP(10'000'000), alice, attacker);
env.close();
Vault const vault{env};
auto [vtx, vkeylet] = vault.create({.owner = alice, .asset = xrpIssue()});
env(vtx);
env.close();
BEAST_EXPECT(env.le(vkeylet));
auto const brokerKeylet =
keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice)));
env(set(alice.id(), vkeylet.key));
env.close();
auto const broker = env.le(brokerKeylet);
BEAST_EXPECT(broker);
Account const pseudo{"broker pseudo-account", broker->at(sfAccount)};
env.memoize(pseudo);
// Pin more than one cleanup batch's worth of credentials.
std::uint16_t const count = kMaxDeletablePseudoAccountCredentials + 3;
for (std::uint16_t i = 0; i < count; ++i)
env(credentials::create(pseudo, attacker, std::to_string(i)));
env.close();
BEAST_EXPECT(ownerCount(env, attacker) == count);
env.enableFeature(fixCleanup3_4_0);
env.close();
// First delete removes one bounded batch and reports it isn't finished.
env(del(alice.id(), brokerKeylet.key), Ter(tecINCOMPLETE));
env.close();
BEAST_EXPECT(env.le(brokerKeylet)); // broker still exists
auto const remaining = ownerCount(env, attacker);
BEAST_EXPECT(remaining > 0 && remaining < count);
// Second delete finishes the cleanup and removes the broker.
env(del(alice.id(), brokerKeylet.key));
env.close();
BEAST_EXPECT(!env.le(brokerKeylet));
BEAST_EXPECT(!env.le(keylet::account(pseudo.id())));
BEAST_EXPECT(ownerCount(env, attacker) == 0);
}
public:
void
run() override
@@ -2889,6 +3010,8 @@ public:
testDisabled();
testLifecycle();
testCredentialPinsPseudoAccount();
testCredentialPinOverflow();
testInvalidLoanBrokerDelete();
testInvalidLoanBrokerSet();
testRequireAuth();

114
src/test/app/vault/VaultBugs_test.cpp
View File

@@ -4,6 +4,7 @@
#include <test/jtx/Env.h>
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/credentials.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/pay.h>
@@ -1135,6 +1136,117 @@ private:
}
}
void
testCredentialPinsPseudoAccount()
{
using namespace test::jtx;
// A credential issued to a vault pseudo-account can't be accepted or
// deleted by it (pseudo-accounts can't sign), so it stays pinned in the
// pseudo-account's owner directory and blocks VaultDelete with
// tecHAS_OBLIGATIONS. A pin created before the cure activates is removed
// by VaultDelete once it does.
Account const owner{"owner"};
Account const attacker{"attacker"};
char const credType[] = "FN36";
Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0};
env.fund(XRP(1'000'000), owner, attacker);
env.close();
Vault const vault{env};
PrettyAsset const asset = xrpIssue();
auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
env(tx);
env.close();
auto const vaultSle = env.le(keylet);
BEAST_EXPECT(vaultSle);
Account const pseudo{"vault pseudo-account", vaultSle->at(sfAccount)};
env.memoize(pseudo);
// The pseudo-account owns the share issuance; the pin must not change
// its owner count (an unaccepted credential is owned by the issuer).
auto const pseudoOwnerCount = ownerCount(env, pseudo);
testcase("Credential pins vault pseudo-account");
env(credentials::create(pseudo, attacker, credType));
env.close();
auto const credKey = credentials::keylet(pseudo, attacker, credType);
BEAST_EXPECT(env.le(credKey));
BEAST_EXPECT(ownerCount(env, attacker) == 1);
BEAST_EXPECT(ownerCount(env, pseudo) == pseudoOwnerCount);
// The pin blocks deletion of an otherwise-empty vault.
env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecHAS_OBLIGATIONS));
env.close();
env.enableFeature(fixCleanup3_4_0);
env.close();
// The pre-existing pin no longer blocks deletion; the credential is
// cleaned up and the issuer's owner count is restored.
testcase("VaultDelete removes pinned credential");
env(vault.del({.owner = owner, .id = keylet.key}));
env.close();
BEAST_EXPECT(!env.le(credKey));
BEAST_EXPECT(!env.le(keylet));
BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id())));
BEAST_EXPECT(ownerCount(env, attacker) == 0);
}
void
testCredentialPinOverflow()
{
using namespace test::jtx;
testcase("Credential pin cleanup is bounded (tecINCOMPLETE)");
// A pseudo-account can be pinned with more credentials than one
// transaction is allowed to clean up. VaultDelete then removes them a
// bounded batch at a time, returning tecINCOMPLETE until the last batch.
Account const owner{"owner"};
Account const attacker{"attacker"};
Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0};
env.fund(XRP(10'000'000), owner, attacker);
env.close();
Vault const vault{env};
auto [tx, keylet] = vault.create({.owner = owner, .asset = xrpIssue()});
env(tx);
env.close();
auto const vaultSle = env.le(keylet);
BEAST_EXPECT(vaultSle);
Account const pseudo{"vault pseudo-account", vaultSle->at(sfAccount)};
env.memoize(pseudo);
// Pin more than one cleanup batch's worth of credentials.
std::uint16_t const count = kMaxDeletablePseudoAccountCredentials + 3;
for (std::uint16_t i = 0; i < count; ++i)
env(credentials::create(pseudo, attacker, std::to_string(i)));
env.close();
BEAST_EXPECT(ownerCount(env, attacker) == count);
env.enableFeature(fixCleanup3_4_0);
env.close();
// First delete removes one bounded batch and reports it isn't finished.
env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecINCOMPLETE));
env.close();
BEAST_EXPECT(env.le(keylet)); // vault still exists
auto const remaining = ownerCount(env, attacker);
BEAST_EXPECT(remaining > 0 && remaining < count);
// Second delete finishes the cleanup and removes the vault.
env(vault.del({.owner = owner, .id = keylet.key}));
env.close();
BEAST_EXPECT(!env.le(keylet));
BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id())));
BEAST_EXPECT(ownerCount(env, attacker) == 0);
}
public:
void
run() override
@@ -1150,6 +1262,8 @@ public:
testBugVaultDepositOvercreditsAcrossScaleBoundary();
testBugVaultLockedByPartialWithdraw();
testVaultDepositNegativeBalanceFromOppositeLimit();
testCredentialPinsPseudoAccount();
testCredentialPinOverflow();
testBug6LimitBypassWithShares();
}
};

191
src/test/app/vault/VaultDomain_test.cpp
View File

@@ -572,6 +572,195 @@ private:
}
}
// Withdrawing out of a private vault to a third party requires both the
// submitter and the destination to be members of the vault's permissioned
// domain. Withdrawal to self is exempt: revoking vault access must not
// trap already deposited funds. The asset issuer is exempt as a
// destination, so that frozen assets can always be returned.
void
testVaultWithdrawPrivateDestinationDomain(FeatureBitset features)
{
using namespace test::jtx;
bool const withFix = features[fixCleanup3_4_0];
testcase(
std::string{"VaultWithdraw private vault destination domain check"} +
(withFix ? " (fixCleanup3_4_0)" : " (pre-fix)"));
Account const issuer{"issuer"};
Account const owner{"owner"};
Account const depositor{"depositor"};
Account const beneficiary{"beneficiary"};
Account const outsider{"outsider"};
Account const pdOwner{"pdOwner"};
Account const credIssuer{"credIssuer"};
std::string const credType = "credential";
Env env{*this, features};
Vault const vault{env};
env.fund(
XRP(100'000), issuer, owner, depositor, beneficiary, outsider, pdOwner, credIssuer);
env.close();
PrettyAsset const asset = issuer["IOU"];
// Everyone holds Layer 1 (asset) permission, so anything blocked below
// is blocked by the Layer 2 (vault) check alone.
for (auto const& account : {owner, depositor, beneficiary, outsider})
{
env.trust(asset(1'000'000), account);
env(pay(issuer, account, asset(10'000)));
}
env.close();
auto const domainId = [&]() {
pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}};
env(pdomain::setTx(pdOwner, credentials));
env.close();
return pdomain::getNewDomain(env.meta());
}();
auto const joinDomain = [&](Account const& account) {
env(credentials::create(account, credIssuer, credType));
env(credentials::accept(account, credIssuer, credType));
env.close();
};
joinDomain(depositor);
joinDomain(beneficiary);
auto [createTx, keylet] =
vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate});
env(createTx);
env.close();
{
auto tx = vault.set({.owner = owner, .id = keylet.key});
tx[sfDomainID] = to_string(domainId);
env(tx);
env.close();
}
env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)}));
env.close();
auto const withdrawTo = [&, keylet = keylet](Account const& destination) {
auto tx =
vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)});
tx[sfDestination] = destination.human();
return tx;
};
{
// Destination holds both layers of permission.
env(withdrawTo(beneficiary));
env.close();
}
{
// Destination may hold the asset but was never let into the vault.
env(withdrawTo(outsider), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS)));
env.close();
}
{
// The asset issuer can always receive, to keep the recovery path
// for frozen assets open.
env(withdrawTo(issuer));
env.close();
}
{
// The vault owner gets no special treatment as a destination: it
// is a third party like any other and needs domain membership.
env(withdrawTo(owner), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS)));
env.close();
}
{
// Withdrawal to self needs no Destination and stays unaffected.
env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)}));
env.close();
}
{
// Naming yourself as the Destination is still a withdrawal to self.
env(withdrawTo(depositor));
env.close();
}
{
testcase(
std::string{"VaultWithdraw private vault submitter lost vault access"} +
(withFix ? " (fixCleanup3_4_0)" : " (pre-fix)"));
env(credentials::deleteCred(credIssuer, depositor, credIssuer, credType));
env.close();
// The exit of last resort: the submitter lost vault access but
// must still be able to redeem its own shares.
env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)}));
env.close();
// Moving funds to anyone else is not allowed any more, even to a
// destination that is itself a domain member.
env(withdrawTo(beneficiary), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS)));
env.close();
// Returning assets to the issuer stays open regardless.
env(withdrawTo(issuer));
env.close();
}
{
testcase(
std::string{"VaultWithdraw private vault with no domain set"} +
(withFix ? " (fixCleanup3_4_0)" : " (pre-fix)"));
// Give the submitter its vault access back first, so that the
// vault having no domain is the only reason left to refuse.
env(credentials::create(depositor, credIssuer, credType));
env(credentials::accept(depositor, credIssuer, credType));
env.close();
auto tx = vault.set({.owner = owner, .id = keylet.key});
tx[sfDomainID] = "0";
env(tx);
env.close();
// Clearing the domain leaves the vault with nobody it considers
// authorized, so a third-party destination cannot qualify even
// though both ends of the payout hold a credential.
env(withdrawTo(beneficiary), Ter(withFix ? TER(tecNO_AUTH) : TER(tesSUCCESS)));
env.close();
// The two exempt paths survive the domain going away.
env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)}));
env.close();
env(withdrawTo(issuer));
env.close();
}
{
testcase(
std::string{"VaultWithdraw public vault destination unaffected"} +
(withFix ? " (fixCleanup3_4_0)" : " (pre-fix)"));
auto [publicTx, publicKeylet] = vault.create({.owner = owner, .asset = asset});
env(publicTx);
env.close();
env(vault.deposit({.depositor = owner, .id = publicKeylet.key, .amount = asset(100)}));
env.close();
auto tx =
vault.withdraw({.depositor = owner, .id = publicKeylet.key, .amount = asset(1)});
tx[sfDestination] = outsider.human();
env(tx);
env.close();
}
}
void
testWithdrawCredentialDepositPreauth(FeatureBitset features)
{
@@ -686,6 +875,8 @@ public:
testDomainLossAfterAcquisition();
testDomainCheckBuyerSideOffer();
testWithDomainChecXRP();
testVaultWithdrawPrivateDestinationDomain(all_ - fixCleanup3_4_0);
testVaultWithdrawPrivateDestinationDomain(all_);
testWithdrawCredentialDepositPreauth(all_ - fixCleanup3_4_0);
testWithdrawCredentialDepositPreauth(all_);
}

112
src/test/app/vault/VaultValidation_test.cpp
View File

@@ -5,10 +5,12 @@
#include <test/jtx/Env.h>
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/credentials.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/mpt.h>
#include <test/jtx/pay.h>
#include <test/jtx/permissioned_domains.h>
#include <test/jtx/ter.h>
#include <test/jtx/trust.h>
#include <test/jtx/vault.h>
@@ -1068,6 +1070,113 @@ private:
}
}
// A pseudo-account belongs to a ledger object, so it must never be the
// destination of a withdrawal. The payout is refused either way, by the
// deposit authorization every pseudo-account carries, so the only change
// is a misleading tecNO_PERMISSION becoming tecPSEUDO_ACCOUNT. The check
// runs ahead of the private-vault domain check, which would otherwise
// report a domain problem against an account that can never join one.
void
testVaultWithdrawPseudoAccountDestination(FeatureBitset features)
{
using namespace test::jtx;
bool const withFix = features[fixCleanup3_4_0];
testcase(
std::string{"VaultWithdraw pseudo-account destination"} +
(withFix ? " (fixCleanup3_4_0)" : " (pre-fix)"));
Account const issuer{"issuer"};
Account const owner{"owner"};
Account const depositor{"depositor"};
Account const pdOwner{"pdOwner"};
Account const credIssuer{"credIssuer"};
std::string const credType = "credential";
Env env{*this, features};
Vault const vault{env};
env.fund(XRP(100'000), issuer, owner, depositor, pdOwner, credIssuer);
// Rippling plays no part in what is being tested here, and would
// otherwise stop the payout before it reaches the check under test.
env(fset(issuer, asfDefaultRipple));
env.close();
PrettyAsset const asset = issuer["IOU"];
for (auto const& account : {owner, depositor})
{
env.trust(asset(1'000'000), account);
env(pay(issuer, account, asset(10'000)));
}
env.close();
// Another vault over the same asset supplies the destination. Its
// pseudo-account holds a trust line for the asset from creation, so
// the payout is refused for being a pseudo-account and nothing else.
auto const pseudoDestination = [&]() {
auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
env(tx);
env.close();
return Account("otherVault", env.le(keylet)->at(sfAccount));
}();
TER const expected = withFix ? TER(tecPSEUDO_ACCOUNT) : TER(tecNO_PERMISSION);
auto const withdrawToPseudo = [&](uint256 const& vaultId) {
auto tx = vault.withdraw({.depositor = depositor, .id = vaultId, .amount = asset(1)});
tx[sfDestination] = pseudoDestination.human();
return tx;
};
{
auto [createTx, keylet] = vault.create({.owner = owner, .asset = asset});
env(createTx);
env.close();
env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)}));
env.close();
env(withdrawToPseudo(keylet.key), Ter(expected));
env.close();
// Withdrawing to self out of the same vault stays unaffected.
env(vault.withdraw({.depositor = depositor, .id = keylet.key, .amount = asset(1)}));
env.close();
}
{
auto const domainId = [&]() {
pdomain::Credentials const credentials{
{.issuer = credIssuer, .credType = credType}};
env(pdomain::setTx(pdOwner, credentials));
env.close();
return pdomain::getNewDomain(env.meta());
}();
env(credentials::create(depositor, credIssuer, credType));
env(credentials::accept(depositor, credIssuer, credType));
env.close();
auto [createTx, keylet] =
vault.create({.owner = owner, .asset = asset, .flags = tfVaultPrivate});
env(createTx);
env.close();
auto setTx = vault.set({.owner = owner, .id = keylet.key});
setTx[sfDomainID] = to_string(domainId);
env(setTx);
env.close();
env(vault.deposit({.depositor = depositor, .id = keylet.key, .amount = asset(1'000)}));
env.close();
// The domain check never gets a say: the destination is rejected
// for what it is, not for the domain it is missing.
env(withdrawToPseudo(keylet.key), Ter(expected));
env.close();
}
}
public:
void
run() override
@@ -1078,6 +1187,9 @@ public:
testCreateFailMPT();
testVaultDeleteMemoData();
testVaultCreateLEVersion();
testVaultWithdrawPseudoAccountDestination(all_ - fixCleanup3_4_0);
testVaultWithdrawPseudoAccountDestination(all_);
}
};