Introduce fixRemoveNFTokenAutoTrustLine amendment:

It turns out that the feature enabled by the tfTrustLine flag
on an NFTokenMint transaction could be used as a means to
attack the NFToken issuer.  Details are in
https://github.com/XRPLF/rippled/issues/4300

The fixRemoveNFTokenAutoTrustLine amendment removes the
ability to set the tfTrustLine flag on an NFTokenMint
transaction.

Closes 4300.

src/ripple/app/tx/impl/NFTokenMint.cpp

@@ -40,7 +40,23 @@ NFTokenMint::preflight(PreflightContext const& ctx)
     if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
         return ret;
 
-    if (ctx.tx.getFlags() & tfNFTokenMintMask)
+    // Prior to fixRemoveNFTokenAutoTrustLine, transfer of an NFToken between
+    // accounts allowed a TrustLine to be added to the issuer of that token
+    // without explicit permission from that issuer.  This was enabled by
+    // minting the NFToken with the tfTrustLine flag set.
+    //
+    // That capability could be used to attack the NFToken issuer.  It
+    // would be possible for two accounts to trade the NFToken back and forth
+    // building up any number of TrustLines on the issuer, increasing the
+    // issuer's reserve without bound.
+    //
+    // The fixRemoveNFTokenAutoTrustLine amendment disables minting with the
+    // tfTrustLine flag as a way to prevent the attack.  But until the
+    // amendment passes we still need to keep the old behavior available.
+    std::uint32_t const NFTokenMintMask =
+        ctx.rules.enabled(fixRemoveNFTokenAutoTrustLine) ? tfNFTokenMintMask
+                                                         : tfNFTokenMintOldMask;
+    if (ctx.tx.getFlags() & NFTokenMintMask)
         return temINVALID_FLAG;
 
     if (auto const f = ctx.tx[~sfTransferFee])

src/ripple/protocol/Feature.h

@@ -74,7 +74,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 = 50;
+static constexpr std::size_t numFeatures = 51;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -338,6 +338,7 @@ extern uint256 const featureExpandedSignerList;
 extern uint256 const fixNFTokenDirV1;
 extern uint256 const fixNFTokenNegOffer;
 extern uint256 const featureNonFungibleTokensV1_1;
+extern uint256 const fixRemoveNFTokenAutoTrustLine;
 
 }  // namespace ripple
 

src/ripple/protocol/TxFlags.h

@@ -120,9 +120,25 @@ constexpr std::uint32_t const tfOnlyXRP                = 0x00000002;
 constexpr std::uint32_t const tfTrustLine              = 0x00000004;
 constexpr std::uint32_t const tfTransferable           = 0x00000008;
 
-constexpr std::uint32_t const tfNFTokenMintMask =
+// Prior to fixRemoveNFTokenAutoTrustLine, transfer of an NFToken between
+// accounts allowed a TrustLine to be added to the issuer of that token
+// without explicit permission from that issuer.  This was enabled by
+// minting the NFToken with the tfTrustLine flag set.
+//
+// That capability could be used to attack the NFToken issuer.  It
+// would be possible for two accounts to trade the NFToken back and forth
+// building up any number of TrustLines on the issuer, increasing the
+// issuer's reserve without bound.
+//
+// The fixRemoveNFTokenAutoTrustLine amendment disables minting with the
+// tfTrustLine flag as a way to prevent the attack.  But until the
+// amendment passes we still need to keep the old behavior available.
+constexpr std::uint32_t const tfNFTokenMintOldMask =
     ~(tfUniversal | tfBurnable | tfOnlyXRP | tfTrustLine | tfTransferable);
 
+constexpr std::uint32_t const tfNFTokenMintMask =
+    ~(tfUniversal | tfBurnable | tfOnlyXRP | tfTransferable);
+
 // NFTokenCreateOffer flags:
 constexpr std::uint32_t const tfSellNFToken            = 0x00000001;
 constexpr std::uint32_t const tfNFTokenCreateOfferMask =

src/ripple/protocol/impl/Feature.cpp

@@ -447,6 +447,7 @@ REGISTER_FEATURE(ExpandedSignerList,            Supported::yes, DefaultVote::no)
 REGISTER_FIX    (fixNFTokenDirV1,               Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixNFTokenNegOffer,            Supported::yes, DefaultVote::no);
 REGISTER_FEATURE(NonFungibleTokensV1_1,         Supported::yes, DefaultVote::no);
+REGISTER_FIX    (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/test/app/NFToken_test.cpp

@@ -1574,7 +1574,6 @@ class NFToken_test : public beast::unit_test::suite
 
         using namespace test::jtx;
 
-        Env env{*this, features};
         Account const alice{"alice"};
         Account const becky{"becky"};
         Account const cheri{"cheri"};
@@ -1583,6 +1582,14 @@ class NFToken_test : public beast::unit_test::suite
         IOU const gwCAD(gw["CAD"]);
         IOU const gwEUR(gw["EUR"]);
 
+        // The behavior of this test changes dramatically based on the
+        // presence (or absence) of the fixRemoveNFTokenAutoTrustLine
+        // amendment.  So we test both cases here.
+        for (auto const& tweakedFeatures :
+             {features - fixRemoveNFTokenAutoTrustLine,
+              features | fixRemoveNFTokenAutoTrustLine})
+        {
+            Env env{*this, tweakedFeatures};
             env.fund(XRP(1000), alice, becky, cheri, gw);
             env.close();
 
@@ -1651,11 +1658,26 @@ class NFToken_test : public beast::unit_test::suite
 
                 uint256 const nftAutoTrustID{token::getNextID(
                     env, alice, 0u, tfTransferable | tfTrustLine, transferFee)};
+
+                // If the fixRemoveNFTokenAutoTrustLine amendment is active
+                // then this transaction fails.
+                {
+                    TER const mintTER =
+                        tweakedFeatures[fixRemoveNFTokenAutoTrustLine]
+                        ? static_cast<TER>(temINVALID_FLAG)
+                        : static_cast<TER>(tesSUCCESS);
+
                     env(token::mint(alice, 0u),
                         token::xferFee(transferFee),
-                txflags(tfTransferable | tfTrustLine));
+                        txflags(tfTransferable | tfTrustLine),
+                        ter(mintTER));
                     env.close();
 
+                    // If fixRemoveNFTokenAutoTrustLine is active the rest
+                    // of this test falls on its face.
+                    if (tweakedFeatures[fixRemoveNFTokenAutoTrustLine])
+                        break;
+                }
                 // becky buys the nft for 1 drop.
                 uint256 const beckyBuyOfferIndex =
                     keylet::nftoffer(becky, env.seq(becky)).key;
@@ -1690,12 +1712,12 @@ class NFToken_test : public beast::unit_test::suite
                 BEAST_EXPECT(env.balance(alice, gwAUD) == gwAUD(10));
                 BEAST_EXPECT(env.balance(alice, gwCAD) == gwCAD(5));
             }
-        // Now that alice has trust lines already established, an nft without
+            // Now that alice has trust lines preestablished, an nft without
             // flagCreateTrustLines will work for preestablished trust lines.
             {
                 std::uint16_t transferFee = 5000;  // 5%
-            uint256 const nftNoAutoTrustID{
+                uint256 const nftNoAutoTrustID{token::getNextID(
-                token::getNextID(env, alice, 0u, tfTransferable, transferFee)};
+                    env, alice, 0u, tfTransferable, transferFee)};
                 env(token::mint(alice, 0u),
                     token::xferFee(transferFee),
                     txflags(tfTransferable));
@@ -1734,6 +1756,7 @@ class NFToken_test : public beast::unit_test::suite
                 BEAST_EXPECT(env.balance(alice, gwCAD) == gwCAD(10));
             }
         }
+    }
 
     void
     testMintFlagTransferable(FeatureBitset features)