Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/xrpl/protocol/detail/features.macro
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// Add new amendments to the top of this list.
// Keep it sorted in reverse chronological order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really need a separate amendment? Since Lending Protocol is not supported, it can be part of the same LendingProtocol amendment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Let's discuss this in the meeting in 15 minutes. My thinking was that since this is related to Freeze, which we happened to touch during Lending testing, it should be a freeze-related amendment. But I also see an argument including it with Lending.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Let's discuss this in the meeting in 15 minutes. My thinking was that since this is related to Freeze, which we happened to touch during Lending testing, it should be a freeze-related amendment. But I also see an argument including it with Lending.

You don't need a new amendment - you can reuse LendingProtocol and/or SingleAssetVault. This bug only affects transactions related to those amendments.

XRPL_FIX (GlobalFreezeIssuer, Supported::no, VoteBehavior::DefaultNo)
XRPL_FEATURE(LendingProtocol, Supported::no, VoteBehavior::DefaultNo)
XRPL_FEATURE(PermissionDelegationV1_1, Supported::no, VoteBehavior::DefaultNo)
XRPL_FIX (DirectoryLimit, Supported::yes, VoteBehavior::DefaultNo)
Expand Down
5 changes: 4 additions & 1 deletion src/libxrpl/ledger/View.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,10 @@ isFrozen(
return false;
auto sle = view.read(keylet::account(issuer));
if (sle && sle->isFlag(lsfGlobalFreeze))
return true;
{
if (!view.rules().enabled(fixGlobalFreezeIssuer) || issuer != account)
return true;
}
Comment on lines 238 to 241
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it really be a separate amendment or should it just be fixed as part of lending? cc @ximinez

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're thinking on the same line of thought: #6113 (comment)

if (issuer != account)
{
// Check if the issuer froze the line
Expand Down
121 changes: 121 additions & 0 deletions src/test/app/Freeze_test.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <test/jtx.h>
#include <test/jtx/AMM.h>

#include <xrpl/ledger/View.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/SField.h>
Expand Down Expand Up @@ -2002,6 +2003,124 @@ class Freeze_test : public beast::unit_test::suite
}
}

void
testIsFrozenDirectly(FeatureBitset features)
{
testcase("isFrozen function issuer exemption");

using namespace test::jtx;
Env env(*this, features);

Account issuer{"issuer"};
Account holder{"holder"};

env.fund(XRP(10000), issuer, holder);
env.close();

auto const USD = issuer["USD"];
env.trust(USD(1000), holder);
env.close();

env(pay(issuer, holder, USD(100)));
env.close();

// Before global freeze, neither account is frozen
BEAST_EXPECT(
!isFrozen(*env.current(), issuer.id(), USD.currency, issuer.id()));
BEAST_EXPECT(
!isFrozen(*env.current(), holder.id(), USD.currency, issuer.id()));

// Enable global freeze
env(fset(issuer, asfGlobalFreeze));
env.close();

// After global freeze, issuer is NOT frozen for their own currency
BEAST_EXPECT(
!isFrozen(*env.current(), issuer.id(), USD.currency, issuer.id()));

// After global freeze, holder IS frozen
BEAST_EXPECT(
isFrozen(*env.current(), holder.id(), USD.currency, issuer.id()));

// Verify issuer can still receive payments (uses isFrozen internally)
env(pay(holder, issuer, USD(10)));
env.close();

Comment on lines +2045 to +2048
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also verify that the issuer can still send payments.

        // Verify issuer can still send payments (uses isFrozen internally)
        env(pay(issuer, holder, USD(10)));
        env.close();

// Verify holder cannot send to other accounts
Account other{"other"};
env.fund(XRP(10000), other);
env.trust(USD(1000), other);
env.close();
env(pay(holder, other, USD(10)), ter(tecPATH_DRY));
}

void
testGlobalFreezeIssuerAmendment(FeatureBitset features)
{
testcase("Global Freeze Issuer Amendment");

using namespace test::jtx;

Account issuer{"issuer"};
Account holder{"holder"};

// Test pre-amendment behavior (issuer IS frozen under global freeze)
{
Env env_pre(*this, features - fixGlobalFreezeIssuer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is still good, but will need to be rewritten to use LP and SAV.


env_pre.fund(XRP(10000), issuer, holder);
env_pre.close();

auto const USD = issuer["USD"];
env_pre.trust(USD(1000), holder);
env_pre.close();

env_pre(pay(issuer, holder, USD(100)));
env_pre.close();

env_pre(fset(issuer, asfGlobalFreeze));
env_pre.close();

BEAST_EXPECT(isFrozen(
*env_pre.current(), issuer.id(), USD.currency, issuer.id()));

BEAST_EXPECT(isFrozen(
*env_pre.current(), holder.id(), USD.currency, issuer.id()));
}

// Test post-amendment behavior (issuer is NOT frozen under global
// freeze)
if (features[fixGlobalFreezeIssuer])
{
Env env_post(*this, features);

env_post.fund(XRP(10000), issuer, holder);
env_post.close();

auto const USD = issuer["USD"];
env_post.trust(USD(1000), holder);
env_post.close();

env_post(pay(issuer, holder, USD(100)));
env_post.close();

env_post(fset(issuer, asfGlobalFreeze));
env_post.close();

BEAST_EXPECT(!isFrozen(
*env_post.current(), issuer.id(), USD.currency, issuer.id()));

BEAST_EXPECT(isFrozen(
*env_post.current(), holder.id(), USD.currency, issuer.id()));

env_post(pay(issuer, holder, USD(10)));
env_post.close();

env_post(pay(holder, issuer, USD(10)));
env_post.close();
}
}

// Helper function to extract trustline flags from open ledger
uint32_t
getTrustlineFlags(
Expand Down Expand Up @@ -2065,6 +2184,8 @@ class Freeze_test : public beast::unit_test::suite
testCreateFrozenTrustline(features);
testSetAndClear(features);
testGlobalFreeze(features);
testIsFrozenDirectly(features);
testGlobalFreezeIssuerAmendment(features);
testNoFreeze(features);
testOffersWhenFrozen(features);
testOffersWhenDeepFrozen(features);
Expand Down
58 changes: 58 additions & 0 deletions src/test/app/LoanBroker_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,63 @@ class LoanBroker_test : public beast::unit_test::suite
});
}

void
testIssuerCoverDepositDuringGlobalFreeze()
{
testcase("Issuer can deposit to broker during global freeze");
using namespace jtx;
using namespace loanBroker;

Account const issuer{"issuer"};
Account const holder{"holder"};
Env env(*this);
Vault vault{env};

env.fund(XRP(100'000), issuer, holder);
env.close();

PrettyAsset const asset = issuer["IOU"];
env.trust(asset(1'000'000), holder);
env.close();
env(pay(issuer, holder, asset(100'000)));
env.close();

auto [tx, vaultKeylet] =
vault.create({.owner = issuer, .asset = asset});
env(tx);
env.close();

env(vault.deposit(
{.depositor = holder, .id = vaultKeylet.key, .amount = asset(50)}));
env.close();

auto const brokerKeylet =
keylet::loanbroker(issuer.id(), env.seq(issuer));
env(set(issuer, vaultKeylet.key));
env.close();

auto broker = env.le(brokerKeylet);
if (!BEAST_EXPECT(broker))
return;

env(fset(issuer, asfGlobalFreeze));
env.close();

// Issuer CAN deposit to their own broker during global freeze
// This is the issuer exemption - issuer can always send (issue) their
// own tokens Per spec: "Counterparties of the frozen issuer can still
// send and receive payments directly to and from the issuing address."
env(coverDeposit(issuer, brokerKeylet.key, asset(10)));
env.close();

// Verify the deposit succeeded
broker = env.le(brokerKeylet);
if (BEAST_EXPECT(broker))
{
BEAST_EXPECT(broker->at(sfCoverAvailable) == asset(10).number());
}
Comment on lines +1481 to +1493
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add tests to check that the broker can withdraw the cover, too?

}

public:
void
run() override
Expand All @@ -1450,6 +1507,7 @@ class LoanBroker_test : public beast::unit_test::suite
testInvalidLoanBrokerDelete();
testInvalidLoanBrokerSet();
testRequireAuth();
testIssuerCoverDepositDuringGlobalFreeze();

// TODO: Write clawback failure tests with an issuer / MPT that doesn't
// have the right flags set.
Expand Down
41 changes: 41 additions & 0 deletions src/test/app/Vault_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5243,6 +5243,46 @@ class Vault_test : public beast::unit_test::suite
});
}

void
testFrozenWithdrawToIssuer()
{
using namespace test::jtx;

testcase("frozen IOU can be withdrawn to issuer");

Env env{*this, testable_amendments() | featureSingleAssetVault};
Account issuer{"issuer"};
Account owner{"owner"};
Account depositor{"depositor"};
env.fund(XRP(1000), issuer, owner, depositor);
env.close();

PrettyAsset asset = issuer["IOU"];
env.trust(asset(1000), owner);
env.trust(asset(1000), depositor);
env(pay(issuer, owner, asset(100)));
env(pay(issuer, depositor, asset(200)));
env.close();

Vault vault{env};
auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
env(tx);
env.close();

env(vault.deposit(
{.depositor = depositor, .id = keylet.key, .amount = asset(50)}));
env.close();

env(fset(issuer, asfGlobalFreeze));
env.close();

auto withdraw = vault.withdraw(
{.depositor = depositor, .id = keylet.key, .amount = asset(10)});
withdraw[sfDestination] = issuer.human();
env(withdraw, ter{tesSUCCESS});
env.close();
}

public:
void
run() override
Expand All @@ -5261,6 +5301,7 @@ class Vault_test : public beast::unit_test::suite
testScaleIOU();
testRPC();
testDelegate();
testFrozenWithdrawToIssuer();
}
};

Expand Down
9 changes: 8 additions & 1 deletion src/xrpld/app/tx/detail/LoanBrokerCoverDeposit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,14 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx)
requireAuth(ctx.view, vaultAsset, account, AuthType::StrongAuth))
return ret;

if (accountHolds(
// IOU issuers have infinite issuance ability and don't have a "balance"
// of their own tokens (accountHolds returns 0 for them). Skip the balance
// check for issuers. Note: issuer freeze exemption is handled by the
// isFrozen() function. This exemption does not apply to MPTs.
bool const isIssuer =
vaultAsset.holds<Issue>() && account == vaultAsset.getIssuer();
if (!isIssuer &&
accountHolds(
ctx.view,
account,
vaultAsset,
Expand Down
33 changes: 26 additions & 7 deletions src/xrpld/app/tx/detail/VaultWithdraw.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,23 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
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;
if (!vaultAsset.holds<Issue>() ||
(dstAcct != vaultAsset.getIssuer() &&
account != vaultAsset.getIssuer()))
{
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, vaultShare))
return ret;
if (!vaultAsset.holds<Issue>() ||
(dstAcct != vaultAsset.getIssuer() &&
account != vaultAsset.getIssuer()))
{
if (auto const ret = checkFrozen(ctx.view, account, vaultShare))
return ret;
}

return tesSUCCESS;
}
Expand Down Expand Up @@ -115,6 +125,7 @@ VaultWithdraw::doApply()

auto const amount = ctx_.tx[sfAmount];
Asset const vaultAsset = vault->at(sfAsset);
auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);
MPTIssue const share{mptIssuanceID};
STAmount sharesRedeemed = {share};
STAmount assetsWithdrawn;
Expand Down Expand Up @@ -165,11 +176,21 @@ VaultWithdraw::doApply()
return tecPATH_DRY;
}

// When withdrawing IOU to the issuer, ignore freeze since spec allows
// returning frozen IOU assets to their issuer. MPTs don't have this
// exemption - MPT locks function like "deep freeze" with no issuer
// exception.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vlntb, do I understand correctly that, if the depositor is the issuer of an MPT, and the asset is locked, the issue will still be able to withdraw from the vault?

FreezeHandling const freezeHandling = (vaultAsset.holds<Issue>() &&
(dstAcct == vaultAsset.getIssuer() ||
account_ == vaultAsset.getIssuer()))
? FreezeHandling::fhIGNORE_FREEZE
: FreezeHandling::fhZERO_IF_FROZEN;

if (accountHolds(
view(),
account_,
share,
FreezeHandling::fhZERO_IF_FROZEN,
freezeHandling,
AuthHandling::ahIGNORE_AUTH,
j_) < sharesRedeemed)
{
Expand Down Expand Up @@ -237,8 +258,6 @@ VaultWithdraw::doApply()
// else quietly ignore, account balance is not zero
}

auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_);

return doWithdraw(
view(),
ctx_.tx,
Expand Down