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.

Sign up for GitHub

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

Jump to bottom

Enforce non-standard currency rule of not starting with 0x00 #5123

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
+296 −1
Open

Enforce non-standard currency rule of not starting with 0x00 #5123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
519b6eb
Enforce non-standard currency rule of not starting with 0x00
gregtatcam Sep 9, 2024
8ef0445
Address reviewers feedback
gregtatcam Sep 10, 2024
4b529f1
Merge branch 'develop' into amendment/enforce-nonstd-currency
Bronek Sep 17, 2024
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
3 changes: 2 additions & 1 deletion include/xrpl/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 79;
static constexpr std::size_t numFeatures = 80;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -372,6 +372,7 @@ extern uint256 const fixEnforceNFTokenTrustline;
extern uint256 const fixInnerObjTemplate2;
extern uint256 const featureInvariantsV1_1;
extern uint256 const fixNFTokenPageLinks;
extern uint256 const fixNonStandardCurrency;

} // namespace ripple

Expand Down
7 changes: 7 additions & 0 deletions include/xrpl/protocol/UintTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ noCurrency();
Currency const&
badCurrency();

enum class PaymentTx : bool { Yes = true, No = false };
/** Valid currency as described in
* https://xrpl.org/docs/references/protocol/data-types
*/
bool
validCurrency(Currency const&, PaymentTx paymentTx = PaymentTx::No);

inline bool
isXRP(Currency const& c)
{
Expand Down
1 change: 1 addition & 0 deletions src/libxrpl/protocol/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::De
REGISTER_FIX (fixEnforceNFTokenTrustline, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixInnerObjTemplate2, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixNFTokenPageLinks, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX (fixNonStandardCurrency, Supported::yes, VoteBehavior::DefaultNo);
// InvariantsV1_1 will be changes to Supported::yes when all the
// invariants expected to be included under it are complete.
REGISTER_FEATURE(InvariantsV1_1, Supported::no, VoteBehavior::DefaultNo);
Expand Down
40 changes: 40 additions & 0 deletions src/libxrpl/protocol/UintTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,44 @@ badCurrency()
return currency;
}

bool
validCurrency(Currency const& currency, PaymentTx paymentTx)
{
// Allow payments for invalid non-standard currencies
// created pre fixNonStandardCurrency amendment
static std::set<Currency> whiteList = {
Currency{"0000000000000000000000000000000078415059"},
Currency{"00000000004150756E6B30310000000000000000"},
Currency{"0000000000D9A928EFBCBEE297A1EFBCBE29DBB6"},
Currency{"0000000000414C6F676F30330000000000000000"},
Currency{"0000000000000000000000005852500000000000"},
Currency{"000028E0B2A05FE0B2A029E2948CE288A9E29490"},
Currency{"00000028E2989EEFBE9FE28880EFBE9F29E2989E"},
Currency{"00000028E381A3E29795E280BFE2979529E381A3"},
Currency{"000000000000005C6D2F5F283E5F3C295F5C6D2F"},
Currency{"00000028E295AFC2B0E296A1C2B0EFBC89E295AF"},
Currency{"0000000000000000000000005852527570656500"},
Currency{"000000000000000000000000302E310000000000"},
Currency{"0000000000E1839A28E0B2A05FE0B2A0E1839A29"},
Currency{"0000000048617070794E6577596561725852504C"},
Currency{"0000E29D9AE29688E29590E29590E29688E29D9A"},
Currency{"000028E297A35FE297A229E2948CE288A9E29490"},
Currency{"00000000CA95E0B2A0E0B2BFE1B4A5E0B2A0CA94"},
Currency{"000000282D5F282D5F282D5F2D295F2D295F2D29"},
Currency{"0000000000000000000000000000000078415049"},
Currency{"00000000000028E295ADE0B2B05FE280A2CC8129"}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

An interesting thing might happen if someone creates a new invalid currency during the voting period of this amendment. Even worse if it happens during the two-week period after voting has completed. I do not think there's anything we can do about it, other than allow errors to happen on transactions with such an invalid currency in the future - I think there should be no future PR to extend this list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. There should be no future PR to extend the list. But it might make sense to check if there are more invalid currencies created before this PR is merged. This list is good as of couple of weeks ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the corollary from this is that in this PR we need to pay attention to enforce the rule across all possible transaction types (and also to pay attention to it in other work in progress), since it would be really "annoying" if there is found to be a transaction type that can succeed for such currencies, while other transaction types fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be trivial to create thousands of additional entries for this list until the PR is merged. The way currency codes are being interpreted is up to client applications and should not be part of the protocol.

See also the documentation: "To prevent this from being treated as a "standard" currency code" - there is nothing stating that "standard currency codes" are a rule.

If you want to enforce standards, you could also start blocking non-ISO-4217 currency codes. This would include BTC, ETH and XRP by the way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are about 140,000 currencies codes out of which ~4,700 are standard and ~135,300 are non-standard. The 22 non-standard currencies that don't follow the rule were clearly created unintentionally. Creating thousands more doesn't serve any purpose and is a violation of the rule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a guideline for interpreting a 160 bit field when using some APIs, not a rule. I also don't see any reason or benefit of limiting this in the protocol (especially with such a clunky allowlist approach, considering that there are more than one ledger out there using this software).

Copy link
Collaborator

@ximinez ximinez Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What if, instead of a hard-coded whitelist, you put the whitelist on the ledger in a new singleton object that just holds an array of Currencies, much like the Amendments singleton?

It could use the LedgerFix transaction introduced in #4945 with a new LedgerFixType. Add fields to LedgerFix to specify the currency code, and one account that has an existing trust line (thus ensuring that this is a currency that already exists on the ledger, and nobody can whitelist a currency preemptively). Use the default base fee, since this isn't doing a lot of work. You've already got an amendment gating all of this, so you wouldn't need another one. It might be worth reusing the ideas from Rules so that the object doesn't have to keep being loaded, but this doesn't have to do the extra work that Rules does to handle presets, so the existing object caches may be enough - it isn't likely to expire from the cache since so many transactions will read it.

This would

  1. Get rid of the hard-coded list.
  2. Allow the whitelist to work across all networks.
  3. Allow any new invalid currencies created before the amendment is enabled to also be whitelisted.
  4. Keeping it cheap should encourage issuers to pay the few drops to fix their currencies, but if they're not, anyone can do it. Whitelisting all the current ones would only cost 200 drops total.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good idea, but it looks like we will go with the longer serialization of STIssue in order to include MPT issuances on a shorter timeline with more community buy-in. Once that happens, we will likely never again need to fix the currency serialization.

static constexpr Currency sIsoBits(
"FFFFFFFFFFFFFFFFFFFFFFFF000000FFFFFFFFFF");

// XRP or standard currency
if (currency == xrpCurrency() || ((currency & sIsoBits).isZero()))
return true;

// Non-standard currency must not start with 0x00
if (*currency.cbegin() != 0x00)
return true;

return paymentTx == PaymentTx::Yes && whiteList.contains(currency);
}

} // namespace ripple
22 changes: 22 additions & 0 deletions src/test/app/AMM_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,28 @@ struct AMM_test : public jtx::AMMTest
// Can't be cleared
AMM amm2(env, gw, XRP(100), USD(100), ter(tecNO_PERMISSION));
}

// Invalid non-standard currency
auto const invalid =
gw["0011111111111111111111111111111111111111"](100);
auto const valid = gw["0111111111111111111111111111111111111111"](100);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
FeatureBitset const all{jtx::supported_amendments()};
for (auto const& features : {all, all - fixNonStandardCurrency})
{
Env env(*this, features);
env.fund(XRP(1'000), gw);
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
auto const err = features[fixNonStandardCurrency] &&
(amt == invalid || amt == invalidWhiteListed)
? ter(temBAD_CURRENCY)
: ter(tesSUCCESS);
AMM amm(env, gw, USD(100), amt, err);
AMM amm1(env, gw, amt, EUR(100), err);
}
}
}

void
Expand Down
30 changes: 30 additions & 0 deletions src/test/app/Check_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2693,6 +2693,35 @@ class Check_test : public beast::unit_test::suite
}
}

void
testInvalidNonStandardCurrency(FeatureBitset features)
{
testcase("Invalid Non-Standard Currency");
using namespace test::jtx;
auto const gw = Account("gw");
auto const alice = Account("alice");
auto const invalid =
gw["0011111111111111111111111111111111111111"](100);
auto const valid = gw["0111111111111111111111111111111111111111"](100);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
auto const USD = gw["USD"];
for (auto const& features_ :
{features, features - fixNonStandardCurrency})
{
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
Env env(*this, features_);
env.fund(XRP(1'000), gw, alice);
auto const err = features_[fixNonStandardCurrency] &&
(amt == invalid || amt == invalidWhiteListed)
? ter(temBAD_CURRENCY)
: ter(tesSUCCESS);
env(check::create(gw, alice, amt), err);
}
}
}

void
testWithFeats(FeatureBitset features)
{
Expand All @@ -2709,6 +2738,7 @@ class Check_test : public beast::unit_test::suite
testCancelInvalid(features);
testFix1623Enable(features);
testWithTickets(features);
testInvalidNonStandardCurrency(features);
}

public:
Expand Down
32 changes: 32 additions & 0 deletions src/test/app/Flow_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,37 @@ struct Flow_test : public beast::unit_test::suite
env.require(balance(alice, XRP(9000) - drops(20)));
}

void
testInvalidNonStandardCurrency(FeatureBitset features)
{
testcase("Invalid Non-Standard Currency");
using namespace test::jtx;
auto const gw = Account("gw");
auto const alice = Account("alice");
auto const bob = Account("bob");
auto const invalid =
gw["0011111111111111111111111111111111111111"](100);
auto const valid = gw["0111111111111111111111111111111111111111"](100);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
Env env(*this, features - fixNonStandardCurrency);
env.fund(XRP(1'000), gw, alice, bob);
env(offer(gw, XRP(100), amt));
env(trust(alice, amt));
env(trust(bob, amt));
env(pay(gw, alice, amt));
auto const err =
amt == invalid ? ter(temBAD_CURRENCY) : ter(tesSUCCESS);
env.enableFeature(fixNonStandardCurrency);
env(pay(alice, bob, amt),
sendmax(XRP(10)),
txflags(tfPartialPayment),
err);
}
}

void
testWithFeats(FeatureBitset features)
{
Expand All @@ -1427,6 +1458,7 @@ struct Flow_test : public beast::unit_test::suite
testReexecuteDirectStep(features);
testSelfPayLowQualityOffer(features);
testTicketPay(features);
testInvalidNonStandardCurrency(features);
}

void
Expand Down
56 changes: 56 additions & 0 deletions src/test/app/NFToken_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7744,6 +7744,61 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite
}
}

void
testInvalidNonStandardCurrency(FeatureBitset features)
{
testcase("Invalid Non-Standard Currency");
using namespace test::jtx;
Account const alice{"alice"};
Account const buyer{"buyer"};
Account const gw{"gw"};
auto const invalid =
gw["0011111111111111111111111111111111111111"](1000);
auto const valid = gw["0111111111111111111111111111111111111111"](1000);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
auto const USD = gw["USD"];
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
auto features_ = features - fixNonStandardCurrency;
Env env{*this, features_};

env.fund(XRP(1'000), alice, buyer, gw);
env.close();

env(trust(buyer, amt));
env(pay(gw, buyer, amt));
env.close();
env(trust(alice, amt));
env(pay(gw, alice, amt));
env.close();

env.enableFeature(fixNonStandardCurrency);

auto const err = (amt == invalid || amt == invalidWhiteListed)
? ter(temBAD_CURRENCY)
: ter(tesSUCCESS);

uint256 nftId = token::getNextID(env, alice, 0, tfTransferable, 10);
env(token::mint(alice, 0u),
txflags(tfTransferable),
token::xferFee(10));
env.close();

env(token::createOffer(buyer, nftId, amt),
token::owner(alice),
err);
env.close();

if (features_[featureNFTokenMintOffer])
{
nftId = token::getNextID(env, alice, 0u);
env(token::mint(alice, 0u), token::amount(amt), err);
env.close();
}
}
}

void
testWithFeats(FeatureBitset features)
{
Expand Down Expand Up @@ -7781,6 +7836,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite
testFixNFTokenBuyerReserve(features);
testUnaskedForAutoTrustline(features);
testNFTIssuerIsIOUIssuer(features);
testInvalidNonStandardCurrency(features);
}

public:
Expand Down
30 changes: 30 additions & 0 deletions src/test/app/Offer_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5299,6 +5299,35 @@ class OfferBaseUtil_test : public beast::unit_test::suite
env.balance(taker, XRP) == takerXRPBalance);
}

void
testInvalidNonStandardCurrency(FeatureBitset features)
{
testcase("Invalid Non-Standard Currency");
using namespace jtx;
auto const gw = Account("gw");
auto const invalid =
gw["0011111111111111111111111111111111111111"](100);
auto const valid = gw["0111111111111111111111111111111111111111"](100);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
auto const USD = gw["USD"];
for (auto const& features_ :
{features, features - fixNonStandardCurrency})
{
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
Env env(*this, features_);
env.fund(XRP(1'000), gw);
auto const err = features_[fixNonStandardCurrency] &&
(amt == invalid || amt == invalidWhiteListed)
? ter(temBAD_CURRENCY)
: ter(tesSUCCESS);
env(offer(gw, amt, USD(100)), err);
env(offer(gw, USD(100), amt), err);
}
}
}

void
testAll(FeatureBitset features)
{
Expand Down Expand Up @@ -5361,6 +5390,7 @@ class OfferBaseUtil_test : public beast::unit_test::suite
testRmSmallIncreasedQOffersXRP(features);
testRmSmallIncreasedQOffersIOU(features);
testFillOrKill(features);
testInvalidNonStandardCurrency(features);
}

void
Expand Down
30 changes: 30 additions & 0 deletions src/test/app/SetTrust_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,35 @@ class SetTrust_test : public beast::unit_test::suite
env.close();
}

void
testInvalidNonStandardCurrency(FeatureBitset features)
{
testcase("Invalid Non-Standard Currency");
using namespace test::jtx;
auto const gw = Account("gw");
auto const alice = Account("alice");
auto const invalid =
gw["0011111111111111111111111111111111111111"](100);
auto const valid = gw["0111111111111111111111111111111111111111"](100);
auto const invalidWhiteListed =
gw["0000000000000000000000000000000078415059"](100);
auto const USD = gw["USD"];
for (auto const& features_ :
{features, features - fixNonStandardCurrency})
{
for (auto const& amt : {valid, invalid, invalidWhiteListed})
{
Env env(*this, features_);
env.fund(XRP(1'000), gw, alice);
auto const err = features_[fixNonStandardCurrency] &&
(amt == invalid || amt == invalidWhiteListed)
? ter(temBAD_CURRENCY)
: ter(tesSUCCESS);
env(trust(alice, amt), err);
}
}
}

void
testWithFeats(FeatureBitset features)
{
Expand All @@ -639,6 +668,7 @@ class SetTrust_test : public beast::unit_test::suite
testExceedTrustLineLimit();
testAuthFlagTrustLines();
testTrustLineLimitsWithRippling();
testInvalidNonStandardCurrency(features);
}

public:
Expand Down
8 changes: 8 additions & 0 deletions src/xrpld/app/tx/detail/AMMCreate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,14 @@ AMMCreate::preflight(PreflightContext const& ctx)
return temBAD_FEE;
}

if (ctx.rules.enabled(fixNonStandardCurrency) &&
(!validCurrency(amount.getCurrency()) ||
!validCurrency(amount2.getCurrency())))
{
JLOG(ctx.j.debug()) << "AMM Instance: bad non-standard currency";
return temBAD_CURRENCY;
}

return preflight2(ctx);
}

Expand Down
8 changes: 8 additions & 0 deletions src/xrpld/app/tx/detail/CreateCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ CreateCheck::preflight(PreflightContext const& ctx)
JLOG(ctx.j.warn()) << "Malformed transaction: Bad currency.";
return temBAD_CURRENCY;
}

if (ctx.rules.enabled(fixNonStandardCurrency) &&
!validCurrency(sendMax.getCurrency()))
{
JLOG(ctx.j.debug())
<< "Malformed transaction: bad non-standard currency";
return temBAD_CURRENCY;
}
}

if (auto const optExpiry = ctx.tx[~sfExpiration])
Expand Down
Loading
Loading