Address reviewers feedback

* Fail NFTokenCreateOffer, NFTokenMint for invalid non-standard currency * Allow Payment for white-listed invalid non-standard currency
gregtatcam · Sep 10, 2024 · 8ef0445 · 8ef0445
1 parent 519b6eb
commit 8ef0445
Show file tree

Hide file tree

Showing 10 changed files with 116 additions and 17 deletions.
diff --git a/include/xrpl/protocol/UintTypes.h b/include/xrpl/protocol/UintTypes.h
@@ -71,8 +71,12 @@ 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&);
+validCurrency(Currency const&, PaymentTx paymentTx = PaymentTx::No);
 
 inline bool
 isXRP(Currency const& c)

diff --git a/src/libxrpl/protocol/UintTypes.cpp b/src/libxrpl/protocol/UintTypes.cpp
@@ -133,8 +133,31 @@ badCurrency()
 }
 
 bool
-validCurrency(Currency const& currency)
+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"}};
     static constexpr Currency sIsoBits(
         "FFFFFFFFFFFFFFFFFFFFFFFF000000FFFFFFFFFF");
 
@@ -146,7 +169,7 @@ validCurrency(Currency const& currency)
     if (*currency.cbegin() != 0x00)
         return true;
 
-    return false;
+    return paymentTx == PaymentTx::Yes && whiteList.contains(currency);
 }
 
 }  // namespace ripple
diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp
@@ -436,15 +436,17 @@ struct AMM_test : public jtx::AMMTest
         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})
+            for (auto const& amt : {valid, invalid, invalidWhiteListed})
             {
-                auto const err =
-                    features[fixNonStandardCurrency] && amt == invalid
+                auto const err = features[fixNonStandardCurrency] &&
+                        (amt == invalid || amt == invalidWhiteListed)
                     ? ter(temBAD_CURRENCY)
                     : ter(tesSUCCESS);
                 AMM amm(env, gw, USD(100), amt, err);

diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp
@@ -2703,16 +2703,18 @@ class Check_test : public beast::unit_test::suite
         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})
+            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
+                auto const err = features_[fixNonStandardCurrency] &&
+                        (amt == invalid || amt == invalidWhiteListed)
                     ? ter(temBAD_CURRENCY)
                     : ter(tesSUCCESS);
                 env(check::create(gw, alice, amt), err);

diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp
@@ -1415,7 +1415,9 @@ struct Flow_test : public beast::unit_test::suite
         auto const invalid =
             gw["0011111111111111111111111111111111111111"](100);
         auto const valid = gw["0111111111111111111111111111111111111111"](100);
-        for (auto const& amt : {valid, invalid})
+        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);

diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp
@@ -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)
     {
@@ -7781,6 +7836,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite
         testFixNFTokenBuyerReserve(features);
         testUnaskedForAutoTrustline(features);
         testNFTIssuerIsIOUIssuer(features);
+        testInvalidNonStandardCurrency(features);
     }
 
 public:

diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp
@@ -5308,16 +5308,18 @@ class OfferBaseUtil_test : public beast::unit_test::suite
         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})
+            for (auto const& amt : {valid, invalid, invalidWhiteListed})
             {
                 Env env(*this, features_);
                 env.fund(XRP(1'000), gw);
-                auto const err =
-                    features_[fixNonStandardCurrency] && amt == invalid
+                auto const err = features_[fixNonStandardCurrency] &&
+                        (amt == invalid || amt == invalidWhiteListed)
                     ? ter(temBAD_CURRENCY)
                     : ter(tesSUCCESS);
                 env(offer(gw, amt, USD(100)), err);

diff --git a/src/test/app/SetTrust_test.cpp b/src/test/app/SetTrust_test.cpp
@@ -628,16 +628,18 @@ class SetTrust_test : public beast::unit_test::suite
         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})
+            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
+                auto const err = features_[fixNonStandardCurrency] &&
+                        (amt == invalid || amt == invalidWhiteListed)
                     ? ter(temBAD_CURRENCY)
                     : ter(tesSUCCESS);
                 env(trust(alice, amt), err);

diff --git a/src/xrpld/app/tx/detail/NFTokenUtils.cpp b/src/xrpld/app/tx/detail/NFTokenUtils.cpp
@@ -850,6 +850,11 @@ tokenOfferCreatePreflight(
         if (dest == acctID)
             return temMALFORMED;
     }
+
+    if (rules.enabled(fixNonStandardCurrency) &&
+        !validCurrency(amount.getCurrency()))
+        return temBAD_CURRENCY;
+
     return tesSUCCESS;
 }
 

diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp
@@ -120,7 +120,8 @@ Payment::preflight(PreflightContext const& ctx)
     }
 
     if (ctx.rules.enabled(fixNonStandardCurrency) &&
-        (!validCurrency(uSrcCurrency) || !validCurrency(uDstCurrency)))
+        (!validCurrency(uSrcCurrency, PaymentTx::Yes) ||
+         !validCurrency(uDstCurrency, PaymentTx::Yes)))
     {
         JLOG(j.trace()) << "Malformed transaction: "
                         << "Bad non-standard currency.";