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 6 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
144 changes: 144 additions & 0 deletions src/ripple/app/tx/impl/Clawback.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
//------------------------------------------------------------------------------
/*
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/Directory.h>
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
#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.getAccountID(sfAccount);
STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount));
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

// The issuer field is used for the token holder instead
AccountID const holder = clawAmount.getIssuer();
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

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.getAccountID(sfAccount);
STAmount const clawAmount(ctx.tx.getFieldAmount(sfAmount));
AccountID const holder = clawAmount.getIssuer();
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

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->getFieldAmount(sfBalance);
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

// 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
Contributor

Choose a reason for hiding this comment

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

You're doing precisely what you advise should not be done in your comment a few lines down (starting on line 89).

I think the better path forward is to add a flag to accountHolds similar to fhIGNORE_FREEZE, called fhINCLUDE_ESCROW which instructs that function to return the full balance, including any escrowed amounts (since, IMO, the issuer should be able to clawback escrowed amounts), and leaving it up to the IOU escrow code to handle the situation in accountHolds when/if it gets merged.

Also, I think that tecINSUFFICIENT_FUNDS is a better error code here.

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 if-condition is purely used to check that the account of this transaction is the issuer, by comparing high/low account with the polarity of the balance. It's not used to check the balance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, if we want to be able to claw back escrowed amounts, I think it would be better to propose a separate amendment after both PRs are merged. I'm don't think it is a good idea to have two PRs making changes for it in parallel

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think escrow or pay channels should be included in the clawback if AMM is excluded.

The justification is "it only allows an issuer to claw back the funds that are spendable".

Escrowed or Paychan iou tokens are locked and are not spendable. Therefore escrow and paychannel should be excluded.


// 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::clawback(
AccountID const& issuer,
AccountID const& holder,
STAmount const& amount)
{
// This should never happen
if (amount <= beast::zero || issuer != amount.getIssuer())
return tecINTERNAL;

return accountSend(view(), holder, issuer, amount, j_);
}

TER
Clawback::doApply()
{
AccountID const issuer = account_;
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
STAmount clawAmount(ctx_.tx.getFieldAmount(sfAmount));
AccountID const holder = clawAmount.getIssuer();
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved

// Replace the `issuer` field with issuer's account
clawAmount.setIssuer(issuer);

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

return clawback(issuer, holder, std::min(spendableAmount, clawAmount));
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
}

} // namespace ripple
55 changes: 55 additions & 0 deletions src/ripple/app/tx/impl/Clawback.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//------------------------------------------------------------------------------
/*
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
{
private:
TER
clawback(
AccountID const& issuer,
AccountID const& holder,
STAmount const& amount);

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
60 changes: 60 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,63 @@ NFTokenCountTracking::finalize(
return true;
}

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

void
ValidClawback::visitEntry(
bool,
std::shared_ptr<SLE const> const& before,
std::shared_ptr<SLE const> const& after)
{
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)
{
// a successful clawback transaction will ALWAYS claw back some funds
// from ONE trustline
if (trustlinesChanged != 1)
{
JLOG(j.fatal())
<< "Invariant failed: wrong number of trustline changed.";
return false;
}

AccountID const issuer = tx.getAccountID(sfAccount);
STAmount const amount = tx.getFieldAmount(sfAmount);
AccountID const holder = amount.getIssuer();
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
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: trustline changed.";
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
}

return true;
}

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

/**
* @brief Invariant: Trust line balance cannot be negative after Clawback.
shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
*
* We iterate all the trust lines affected by this transaction and ensure
* that they have non-negative balance.
*/
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 @@ -378,7 +404,8 @@ using InvariantChecks = std::tuple<
NoZeroEscrow,
ValidNewAccountRoot,
ValidNFTokenPage,
NFTokenCountTracking>;
NFTokenCountTracking,
ValidClawback>;

/**
* @brief get a tuple of all invariant checks
Expand Down
35 changes: 35 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,25 @@ SetAccount::preclaim(PreclaimContext const& ctx)
}
}

//
// Clawback
//
if (ctx.view.rules().enabled(featureClawback) &&
(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)))
{
JLOG(ctx.j.trace()) << "Owner directory not empty.";
return tecOWNERS;
}
}

return tesSUCCESS;
}

Expand Down Expand Up @@ -361,6 +380,14 @@ SetAccount::doApply()
return tecNEED_MASTER_KEY;
}

// Cannot set NoFreeze if clawback is enabled
if (ctx_.view().rules().enabled(featureClawback) &&
(uFlagsIn & lsfAllowClawback))
{
JLOG(j_.trace()) << "Can't set NoFreeze if clawback is enabled";
return tecNO_PERMISSION;
}

shawnxie999 marked this conversation as resolved.
Show resolved Hide resolved
JLOG(j_.trace()) << "Set NoFreeze flag";
uFlagsOut |= lsfNoFreeze;
}
Expand Down Expand Up @@ -562,6 +589,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