WietseWind commented 2 months ago

Abstract

Clawback can only be set on an account with an empty directory.

This allows for someone willing to lock up some reserve (which isn't even spent, just locked) to spam TrustLines to all proposed & created accounts, rendering Clawback unavialable for any account with the purpose to be a token issuer.

The Bug

The Clawback amendment describes setting the Flag on AccountRoot (AccountSet, 16):

https://xrpl.org/docs/references/protocol/transactions/types/clawback:

Clawback is disabled by default. To use clawback, you must send an AccountSet transaction to enable the Allow Trust Line Clawback setting. An issuer with any existing tokens cannot enable Clawback. You can only enable Allow Trust Line Clawback if you have a completely empty owner directory, meaning you must do so before you set up any trust lines, offers, escrows, payment channels, checks, or signer lists.

Here, 'completely empty owner directory" is somewhat ambiguous. Does it refer to the owned objects in own directory or to the directory itself? Then the use of "you" in the line:

[...] you must do so before you set up any trust lines [...]

Which made me wonder: what if something exists in your directory which isn't technically owned by "you", but "done to you". Say a TrustLine that is in default state for you, but not for someone else.

This means that one could "take one for the team" and spend some reserve on creating a TrustLine for an abritrary asset code towards any account, e.g. right when it gets activated, by listening to the account propose stream. When doing this, it will prevent anyone from using the Clawback feature.

Proof of concept (testnet)

1. Set TL to new unused only activated account

2. Try to set Clawback on the account the TL was just set to:

Proposed solution

Check on TLs with an actual balance, instead of any TL / owned object at all: allow for setting the flag if none of the existing RippleStates have balance.

While this is very inefficient (as it would require iterating over the ownerdirectory instead of just looking at count, then to take into account only RippleState with non zero balance, providing the Flag can be set only once, it could be worth iterating.

If AcccountRoot Flag 16 is not set (otherwise: don't set again, and it can't be unset anyway so this is a max. 1x per account)
Iterate over RippleStates
If any of them non-zero: reject
Otherwise: accept

ximinez commented 2 months ago

This kind of problem, unfortunately, has existed for a while with other features where the owner directory has to be empty. Fortunately, people haven't seemed to be too interested in doing it, I think partly because the reserve requirements can get expensive pretty quickly.

A problem with only checking the balance of a trust line is that it may not be sufficient. I think we'd also need to check the limit, because the limit is saying that the user already trusts the issuer for that amount, and changing parameters when trust has already been extended is not great. But if the limit is zero, then the only way to have a non-default trust line is with flags or quality, and what does that say about the user creating the trust line?

Another possible solution is the proposed Atomic transaction. One could fund the issuing account, then set all the flags in one atomic transaction. That would solve this entire class of problems.

Clearly the documentation needs to be updated, regardless of how this is addressed.