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

Account has issued a currency with non standard currency code "XRP". #5298

Open
donovanhide opened this issue Feb 19, 2025 · 10 comments
Open

Comments

@donovanhide
Copy link
Contributor

donovanhide commented Feb 19, 2025

Issue Description

Account has issued a currency with non standard currency code "XRP".

Steps to Reproduce

https://xrpscan.com/account/rPogU2XdDeiycaos1toSLFgoscca2qG2H

Expected Result

temBAD_CURRENCY would be the expected result for a TrustSet with "XRP" as currency

if (badCurrency() == saLimitAmount.getCurrency())
{
JLOG(j.trace()) << "Malformed transaction: specifies XRP as IOU";
return temBAD_CURRENCY;
}

Currency const&
badCurrency()
{
static Currency const currency(0x5852500000000000);
return currency;
}

Actual Result

Accounts on the live network have trustlines for XRP/rPogU2XdDeiycaos1toSLFgoscca2qG2H

Environment

Live network as of 19th Feb 2025

Supporting Files

@donovanhide
Copy link
Contributor Author

donovanhide commented Feb 19, 2025

The check for a standard currency code of "XRP" is satisfied, but a check for a non-standard currency code of "XRP" is not. The suffix of 00000000000 in the quoted code above, rather than just 0x58525 with the correct offset for a standard currency code does lend a bit of ambiguity as to what it is trying to test.
Either way, in my opinion, both non-standard and standard currency codes of "XRP" should be disallowed.

@ximinez
Copy link
Collaborator

ximinez commented Feb 19, 2025

I could make an argument that this is a display issue on the part of the explorers and/or wallets. This is a non-standard currency, which you pointed out. rippled is not returning "XRP" for these trust lines.

$ rippled -q account_lines rPogU2XdDeiycaos1toSLFgoscca2qG2H | jq '.result.lines | map(.currency)'
[
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000",
  "5852500000000000000000000000000000000000"
]

Any modification after that is out of our hands. As far as I know there is no XLS or other spec describing the "correct" way to parse these kinds of codes - everybody has converged on interpreting the non-zero pairs as ASCII codes. Strictly speaking, this is a bug in every wallet or explorer that does that.

That said, there is a high potential that this could lead to people getting scammed, so I think it's worth changing. It will require an amendment.

@ximinez
Copy link
Collaborator

ximinez commented Feb 19, 2025

Outline of a solution:

  1. New function bool isBadCurrency(Currency) that returns true if the only non-zero bytes are 0x58525.
  2. If the amendment is enabled, return temBAD_CURRENCY if isBadCurrency is true for any of the currencies in the transaction in any of the following situations.
    • Payment with a path.
    • OfferCreate
      • maybe create an exception if the bad currency is in TakerGets and TakerPays is real XRP
      • Or vice versa if the transaction is from the issuer
    • CheckCreate, or Cash
    • NFTokenCreateOffer or AcceptOffer
    • AMMCreate or Deposit
    • XChainCommit or anything else related to XChain
    • TrustSet with a non-zero limit.
  3. If the amendment is enabled, return a tec code (TBD, preferably an existing one, may vary by scenario) if isBadCurrency is true for any of the currencies in the transaction or the relevant ledger objects in any of the following situations.
    • Payment doing anything other than sending all or part of a positive balance back to the issuer (but not more). (return tecPATH_DRY?)
  4. I'm going to go a step further, and say that invalid codes shouldn't be allowed in the metadata for MPTs, too. That'll use the same type of check, but not isBadCurrency(Currency) because the metadata is not a currency.
    • MPTokenIssuanceCreate
    • MPTokenAuthorize except deletion

Taken together, these should allow anybody holding these coins to get rid of them and delete their trust lines while preventing anybody from creating new trustlines (or MPTs) or sending them to anybody other than the issuer.

@Tapanito
Copy link

Regarding 4. @ximinez, are you suggesting forbidding the creation (or setting) of MPT metadata to 0x58525?

MPTs might be trickier. A recent proposal (XRPLF/XRPL-Standards#264) provides a standard structure for MPT metadata. In such structured metadata, someone may still XRP as the Identifier field, and rippled couldn't do anything about it. (Well, technically it could, but rippled would have to parse that metadata, but I don't think that's a good idea).

@ximinez
Copy link
Collaborator

ximinez commented Feb 19, 2025

Regarding 4. @ximinez, are you suggesting forbidding the creation (or setting) of MPT metadata to 0x58525?

Yes, but only if that's the entire metadata.

MPTs might be trickier. A recent proposal (XRPLF/XRPL-Standards#264) provides a standard structure for MPT metadata. In such structured metadata, someone may still XRP as the Identifier field, and rippled couldn't do anything about it. (Well, technically it could, but rippled would have to parse that metadata, but I don't think that's a good idea).

Something structured should be clear that it's not trying to impersonate XRP. More specific checks might need to be addressed in the proposal.

@mDuo13
Copy link
Collaborator

mDuo13 commented Feb 19, 2025

There isn't exactly an XLS for this, but it's not a completely new problem.

One of the recommended Code Samples on XRPL is "Normalize Currency Codes", which correctly handles this scenario—it specifically checks that non-standard codes cannot be deserialized to "xrp", case-insensitive. So I agree, this is a bug in the display function. Unfortunately not only does XRPScan get it wrong, but the Livenet explorer gets it extra wrong, incorrectly showing the XRP symbol in the "other balances" dropdown in addition to showing "XRP" in the TrustSet summary.

Livenet explorer inappropriately showing XRP X for this account's balance of issued 'XRP' token

XLS-21d also mentioned ASCII codes, and explicitly discouraged other asset codes that start with bytes that could indicate common ASCII characters (0x200x7E).

I have also in the past suggested an "extended" currency code spec with the following properties:

"Extended" codes. These are 4 to 20 characters in length. These are encoded as ASCII starting with the first byte. Uppercase letters (A-Z), lowercase letters (a-z), the digits 0-9, and the dollar symbol ($) are allowed. To avoid conflicts with ISO-like codes, codes less than 4 characters long are disallowed.

I chose the allowed characters based on non-standard codes currently in use. Also, the 4 character minimum is necessary to avoid overlap with ISO-like currency codes including but not limited to "XRP" which are (currently) returned as three letters instead of hex by the rippled APIs.

We should avoid this confusion by implementing conversions for "extended" codes such as the above into rippled and/or the client libraries, instead of leaving it to each individual project to build the conversion and fall into this pitfall if they don't follow best practices.

@ximinez
Copy link
Collaborator

ximinez commented Feb 19, 2025

I chose the allowed characters based on non-standard codes currently in use. Also, the 4 character minimum is necessary to avoid overlap with ISO-like currency codes including but not limited to "XRP" which are (currently) returned as three letters instead of hex by the rippled APIs.

Another thought is to extend the isBadCurrency I suggested above to return true if the currency code only has 3 or fewer non-zero bytes, regardless of position, unless it is a correctly formatted ISO currency code, or if that ISO code converts to "XRP" case-insensitive.

We should avoid this confusion by implementing conversions for "extended" codes such as the above into rippled and/or the client libraries, instead of leaving it to each individual project to build the conversion and fall into this pitfall if they don't follow best practices.

I'm a little leery of adding more conversions to rippled. I'd rather have better validation and a good spec for what libraries and projects should be doing.

@mvadari
Copy link
Collaborator

mvadari commented Feb 19, 2025

Another thought is to extend the isBadCurrency I suggested above to return true if the currency code only has 3 or fewer non-zero bytes

It has to be exactly 3 letters, not including fewer - standard currency codes are exactly 3 letters.
https://xrpl.org/docs/references/protocol/data-types/currency-formats#standard-currency-codes

We should avoid this confusion by implementing conversions for "extended" codes such as the above into rippled and/or the client libraries, instead of leaving it to each individual project to build the conversion and fall into this pitfall if they don't follow best practices.

I'm a little leery of adding more conversions to rippled. I'd rather have better validation and a good spec for what libraries and projects should be doing.

Personally I would be open to adding this update to the RPCs but not to transaction processing. Though I agree, that's a larger-scale change.

@ximinez
Copy link
Collaborator

ximinez commented Feb 20, 2025

It has to be exactly 3 letters, not including fewer - standard currency codes are exactly 3 letters.
xrpl.org/docs/references/protocol/data-types/currency-formats#standard-currency-codes

Copying what I said here:

Yes, you are correct about it being exactly 3. I was thinking of making the check more aggressive than that, though, because the standard allows (maybe requires?) that shorter codes be padded out with leading "X"s. So if you had a currency you wanted to call "RP", the ISO code would be "XRP". 😁 I imagine a currency called "A" would get an ISO code of "XXA", but I haven't read the RFC to verify that.

The point is that there is a standards compliant way for someone to use a currency code of less than 3 characters, so IMHO we shouldn't allow them to do otherwise.

@sappenin
Copy link
Collaborator

sappenin commented Feb 21, 2025

@Tapanito: ...are you suggesting forbidding the creation (or setting) of MPT metadata to 0x58525?
@ximinez: Yes, but only if that's the entire metadata.

Having rippled parse or interpret MPT metadata (at all) feels "off" to me.

But to this specific idea, firstly 0x58525 seems like an unlikely value on its own for MPT Metadata (so do we really need to check?). Secondly, even if an issuer decided to use this as the sole value of their MPT metadata, it's up to each issuer to choose the meaning of this data, so I don't think we should restrict it. Contrived example: imagine an issuer using 0x58525 to mean something else, like a 3-character RWA serial number.

(Caveat: If some metadata spec does get codified around how to use metadata, I might change my opinion here).

In general, if UIs aren't properly communicating the Issuer part of an IOU or MPT, then we have a bigger problem than just XRP (i.e., any asset/currency code needs to be considered and displayed properly in the context of an issuer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants