Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XLS-39 Clawback: #4553

Merged
merged 19 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions Builds/CMake/RippledCore.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ target_sources (rippled PRIVATE
src/ripple/app/tx/impl/CancelOffer.cpp
src/ripple/app/tx/impl/CashCheck.cpp
src/ripple/app/tx/impl/Change.cpp
src/ripple/app/tx/impl/Clawback.cpp
src/ripple/app/tx/impl/CreateCheck.cpp
src/ripple/app/tx/impl/CreateOffer.cpp
src/ripple/app/tx/impl/CreateTicket.cpp
Expand Down Expand Up @@ -692,6 +693,7 @@ if (tests)
src/test/app/AccountTxPaging_test.cpp
src/test/app/AmendmentTable_test.cpp
src/test/app/Check_test.cpp
src/test/app/Clawback_test.cpp
src/test/app/CrossingLimits_test.cpp
src/test/app/DeliverMin_test.cpp
src/test/app/DepositAuth_test.cpp
Expand Down
138 changes: 138 additions & 0 deletions src/ripple/app/tx/impl/Clawback.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
//------------------------------------------------------------------------------
/*
This file is part of rippled: https://github.com/ripple/rippled
Copyright (c) 2023 Ripple Labs Inc.

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
//==============================================================================

#include <ripple/app/tx/impl/Clawback.h>
#include <ripple/ledger/View.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/Indexes.h>
#include <ripple/protocol/Protocol.h>
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
#include <ripple/protocol/TxFlags.h>
#include <ripple/protocol/st.h>

namespace ripple {

NotTEC
Clawback::preflight(PreflightContext const& ctx)
{
if (!ctx.rules.enabled(featureClawback))
return temDISABLED;

if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
return ret;

if (ctx.tx.getFlags() & tfClawbackMask)
return temINVALID_FLAG;

AccountID const issuer = ctx.tx[sfAccount];
STAmount const clawAmount = ctx.tx[sfAmount];

// The issuer field is used for the token holder instead
AccountID const& holder = clawAmount.getIssuer();

if (issuer == holder || isXRP(clawAmount) || clawAmount <= beast::zero)
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
return temBAD_AMOUNT;

return preflight2(ctx);
}

TER
Clawback::preclaim(PreclaimContext const& ctx)
{
AccountID const issuer = ctx.tx[sfAccount];
STAmount const clawAmount = ctx.tx[sfAmount];
AccountID const& holder = clawAmount.getIssuer();

auto const sleIssuer = ctx.view.read(keylet::account(issuer));
auto const sleHolder = ctx.view.read(keylet::account(holder));
if (!sleIssuer || !sleHolder)
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
return terNO_ACCOUNT;

std::uint32_t const issuerFlagsIn = sleIssuer->getFieldU32(sfFlags);

// If AllowClawback is not set or NoFreeze is set, return no permission
if (!(issuerFlagsIn & lsfAllowClawback) || (issuerFlagsIn & lsfNoFreeze))
return tecNO_PERMISSION;
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

auto const sleRippleState =
ctx.view.read(keylet::line(holder, issuer, clawAmount.getCurrency()));
if (!sleRippleState)
return tecNO_LINE;
Comment on lines +72 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

Loading a SLE is an expensive operation, and you could avoid it, since accountHolds will also do it. Of course, it won't tell you if the trust line doesn't exist, but you could (a) either improve the interface of the (legacy) accountHolds to return an std::optional<STAmount>; or (b) by returning a more generic error code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The SLE of the trustline is fetched to check the account must be the issuer in the trustline. Without loading it and purely by using accountHolds, I would not be able to know that

    // If balance is positive, issuer must have higher address than holder
    if (balance > beast::zero && issuer < holder)
        return tecNO_PERMISSION;

    // If balance is negative, issuer must have lower address than holder
    if (balance < beast::zero && issuer > holder)
        return tecNO_PERMISSION;

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jun 1, 2023

Choose a reason for hiding this comment

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

i would like to explicitly check the high/low account and the balance polarity to verify that the issuer is the one who submitted this transaction. Even though checking the sign of what accountHolds returns could work too, but that is not the purpose of this function, and behaviors could possibly change too. I think making an extra call is worth it in this case, because I would like to be fully confident that the real issuer is the one submitting this transaction, not the token holder.


STAmount const balance = (*sleRippleState)[sfBalance];

// If balance is positive, issuer must have higher address than holder
if (balance > beast::zero && issuer < holder)
return tecNO_PERMISSION;

// If balance is negative, issuer must have lower address than holder
if (balance < beast::zero && issuer > holder)
return tecNO_PERMISSION;

// At this point, we know that issuer and holder accounts
// are correct and a trustline exists between them.
//
// Must now explicitly check the balance to make sure
// available balance is non-zero.
//
// We can't directly check the balance of trustline because
// the available balance of a trustline is prone to new changes (eg.
// XLS-34). So we must use `accountHolds`.
if (accountHolds(
ctx.view,
holder,
clawAmount.getCurrency(),
issuer,
fhIGNORE_FREEZE,
ctx.j) <= beast::zero)
return tecINSUFFICIENT_FUNDS;

return tesSUCCESS;
}

TER
Clawback::doApply()
{
AccountID const& issuer = account_;
STAmount clawAmount = ctx_.tx[sfAmount];
AccountID const holder = clawAmount.getIssuer(); // cannot be reference

// Replace the `issuer` field with issuer's account
clawAmount.setIssuer(issuer);
if (holder == issuer)
return tecINTERNAL;

// Get the spendable balance. Must use `accountHolds`.
STAmount const spendableAmount = accountHolds(
view(),
holder,
clawAmount.getCurrency(),
clawAmount.getIssuer(),
fhIGNORE_FREEZE,
j_);

return rippleCredit(
view(),
holder,
issuer,
std::min(spendableAmount, clawAmount),
true,
j_);
}

} // namespace ripple
48 changes: 48 additions & 0 deletions src/ripple/app/tx/impl/Clawback.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
//------------------------------------------------------------------------------
/*
This file is part of rippled: https://github.com/ripple/rippled
Copyright (c) 2023 Ripple Labs Inc.

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
//==============================================================================

#ifndef RIPPLE_TX_CLAWBACK_H_INCLUDED
#define RIPPLE_TX_CLAWBACK_H_INCLUDED

#include <ripple/app/tx/impl/Transactor.h>

namespace ripple {

class Clawback : public Transactor
{
public:
static constexpr ConsequencesFactoryType ConsequencesFactory{Normal};

explicit Clawback(ApplyContext& ctx) : Transactor(ctx)
{
}

static NotTEC
preflight(PreflightContext const& ctx);

static TER
preclaim(PreclaimContext const& ctx);

TER
doApply() override;
};

} // namespace ripple

#endif
59 changes: 59 additions & 0 deletions src/ripple/app/tx/impl/InvariantCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/basics/FeeUnits.h>
#include <ripple/basics/Log.h>
#include <ripple/ledger/ReadView.h>
#include <ripple/ledger/View.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/STArray.h>
#include <ripple/protocol/SystemParameters.h>
Expand Down Expand Up @@ -717,4 +718,62 @@ NFTokenCountTracking::finalize(
return true;
}

//------------------------------------------------------------------------------

void
ValidClawback::visitEntry(
bool,
std::shared_ptr<SLE const> const& before,
std::shared_ptr<SLE const> const&)
{
if (before && before->getType() == ltRIPPLE_STATE)
trustlinesChanged++;
}

bool
ValidClawback::finalize(
STTx const& tx,
TER const result,
XRPAmount const,
ReadView const& view,
beast::Journal const& j)
{
if (tx.getTxnType() != ttCLAWBACK)
return true;

if (result == tesSUCCESS)
{
if (trustlinesChanged > 1)
{
JLOG(j.fatal())
<< "Invariant failed: more than one trustline changed.";
return false;
}

AccountID const issuer = tx.getAccountID(sfAccount);
STAmount const amount = tx.getFieldAmount(sfAmount);
AccountID const& holder = amount.getIssuer();
STAmount const holderBalance = accountHolds(
view, holder, amount.getCurrency(), issuer, fhIGNORE_FREEZE, j);

if (holderBalance.signum() < 0)
{
JLOG(j.fatal())
<< "Invariant failed: trustline balance is negative";
return false;
}
}
else
{
if (trustlinesChanged != 0)
{
JLOG(j.fatal()) << "Invariant failed: some trustlines were changed "
"despite failure of the transaction.";
return false;
}
}

return true;
}

} // namespace ripple
31 changes: 30 additions & 1 deletion src/ripple/app/tx/impl/InvariantCheck.h
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,34 @@ class NFTokenCountTracking
beast::Journal const&);
};

/**
* @brief Invariant: Token holder's trustline balance cannot be negative after
* Clawback.
*
* We iterate all the trust lines affected by this transaction and ensure
* that no more than one trustline is modified, and also holder's balance is
* non-negative.
*/
class ValidClawback
{
std::uint32_t trustlinesChanged = 0;
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

public:
void
visitEntry(
bool,
std::shared_ptr<SLE const> const&,
std::shared_ptr<SLE const> const&);

bool
finalize(
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
STTx const&,
TER const,
XRPAmount const,
ReadView const&,
beast::Journal const&);
};

// additional invariant checks can be declared above and then added to this
// tuple
using InvariantChecks = std::tuple<
Expand All @@ -402,7 +430,8 @@ using InvariantChecks = std::tuple<
NoZeroEscrow,
ValidNewAccountRoot,
ValidNFTokenPage,
NFTokenCountTracking>;
NFTokenCountTracking,
ValidClawback>;

/**
* @brief get a tuple of all invariant checks
Expand Down
39 changes: 39 additions & 0 deletions src/ripple/app/tx/impl/SetAccount.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,37 @@ SetAccount::preclaim(PreclaimContext const& ctx)
}
}

//
// Clawback
//
if (ctx.view.rules().enabled(featureClawback))
{
if (uSetFlag == asfAllowClawback)
{
if (uFlagsIn & lsfNoFreeze)
{
JLOG(ctx.j.trace()) << "Can't set Clawback if NoFreeze is set";
return tecNO_PERMISSION;
}

if (!dirIsEmpty(ctx.view, keylet::ownerDir(id)))
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
{
JLOG(ctx.j.trace()) << "Owner directory not empty.";
return tecOWNERS;
}
}
else if (uSetFlag == asfNoFreeze)
{
// Cannot set NoFreeze if clawback is enabled
if (uFlagsIn & lsfAllowClawback)
{
JLOG(ctx.j.trace())
<< "Can't set NoFreeze if clawback is enabled";
return tecNO_PERMISSION;
}
}
}

return tesSUCCESS;
}

Expand Down Expand Up @@ -562,6 +593,14 @@ SetAccount::doApply()
uFlagsOut &= ~lsfDisallowIncomingTrustline;
}

// Set flag for clawback
if (ctx_.view().rules().enabled(featureClawback) &&
uSetFlag == asfAllowClawback)
{
JLOG(j_.trace()) << "set allow clawback";
uFlagsOut |= lsfAllowClawback;
}
Comment on lines +597 to +602
Copy link
Contributor

Choose a reason for hiding this comment

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

The code to clear the lsfAllowClawback flag (i.e. to disclaim your ability to clawback) is missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this flag is never meant to be cleared. Once set, it cannot be cleared

Copy link
Contributor

Choose a reason for hiding this comment

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

But why? This is the inverse of "no freeze" and it should definitely be a flag you can clear.

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jun 1, 2023

Choose a reason for hiding this comment

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

@nbougalis The reason for making clawback opt-out by default was purely because, having clawback opt-in was a poor idea and would raise a lot of concerns, just like how having freeze opt-in by default was a bad choice. The verdict is that we would like "opt-out" these kind of regulatory features in the future, and clawback would be the first one to follow that.

What is really the incentive for making allow clawback a mutable field? If they really want to mutate it, then I don't see why can't they create a new account. It would also create unnecessary complexity and confusion as well, since AllowClawback and NoFreeze are interdependent on each other.

But I think it would be a good practice to stick with immutable fields for regulatory features like freeze or clawback.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you misunderstand: I'm not suggesting that clawback should be opt-out, that would be a bad idea. I was just saying that I would allow it to be unset, but I don't feel strongly enough about it.

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jun 2, 2023

Choose a reason for hiding this comment

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

I'm not quite sure if there is any need for this. But if we find out that unsetting the flag is really wanted somehow, I think it would be fine to add it through an amendment later. This would be a rather small change. But I think it would be a good idea to keep it simple for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going on a tangent here: Since Clawback and NoFreeze are complimentary to one another, is it a good idea to frame this condition as an invariant? I would eliminate such a check in other parts of the code right?

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jun 12, 2023

Choose a reason for hiding this comment

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

@ckeshava I don't think this change would eliminate such checks in other parts of the code. Invariant check is executed after a transaction.

And it will still be mandatory to check this complementary property during the preclaim (before an transaction) of an AccountSet.


if (uFlagsIn != uFlagsOut)
sle->setFieldU32(sfFlags, uFlagsOut);

Expand Down
Loading