fixNFTokenRemint: prevent NFT re-mint: (#4406)

Without the protocol amendment introduced by this commit, an NFT ID can
be reminted in this manner:

1. Alice creates an account and mints an NFT.
2. Alice burns the NFT with an `NFTokenBurn` transaction.
3. Alice deletes her account with an `AccountDelete` transaction.
4. Alice re-creates her account.
5. Alice mints an NFT with an `NFTokenMint` transaction with params:
   `NFTokenTaxon` = 0, `Flags` = 9).

This will mint a NFT with the same `NFTokenID` as the one minted in step
1. The params that construct the NFT ID will cause a collision in
`NFTokenID` if their values are equal before and after the remint.

With the `fixNFTokenRemint` amendment, there is a new sequence number
construct which avoids this scenario:

- A new `AccountRoot` field, `FirstNFTSequence`, stays constant over
  time.
  - This field is set to the current account sequence when the account
    issues their first NFT.
  - Otherwise, it is not set.
- The sequence of a newly-minted NFT is computed by: `FirstNFTSequence +
  MintedNFTokens`.
  - `MintedNFTokens` is then incremented by 1 for each mint.

Furthermore, there is a new account deletion restriction:

- An account can only be deleted if `FirstNFTSequence + MintedNFTokens +
  256` is less than the current ledger sequence.
  - 256 was chosen because it already exists in the current account
    deletion constraint.

Without this restriction, an NFT may still be remintable. Example
scenario:

1. Alice's account sequence is at 1.
2. Bob is Alice's authorized minter.
3. Bob mints 500 NFTs for Alice. The NFTs will have sequences 1-501, as
   NFT sequence is computed by `FirstNFTokenSequence + MintedNFTokens`).
4. Alice deletes her account at ledger 257 (as required by the existing
   `AccountDelete` amendment).
5. Alice re-creates her account at ledger 258.
6. Alice mints an NFT. `FirstNFTokenSequence` initializes to her account
   sequence (258), and `MintedNFTokens` initializes as 0. This
   newly-minted NFT would have a sequence number of 258, which is a
   duplicate of what she issued through authorized minting before she
   deleted her account.

---------

Signed-off-by: Shawn Xie <[email protected]>

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

@@ -214,6 +214,23 @@ DeleteAccount::preclaim(PreclaimContext const& ctx)
     if ((*sleAccount)[sfSequence] + seqDelta > ctx.view.seq())
         return tecTOO_SOON;
 
+    // When fixNFTokenRemint is enabled, we don't allow an account to be
+    // deleted if <FirstNFTokenSequence + MintedNFTokens> is within 256 of the
+    // current ledger. This is to prevent having duplicate NFTokenIDs after
+    // account re-creation.
+    //
+    // Without this restriction, duplicate NFTokenIDs can be reproduced when
+    // authorized minting is involved. Because when the minter mints a NFToken,
+    // the issuer's sequence does not change. So when the issuer re-creates
+    // their account and mints a NFToken, it is possible that the
+    // NFTokenSequence of this NFToken is the same as the one that the
+    // authorized minter minted in a previous ledger.
+    if (ctx.view.rules().enabled(fixNFTokenRemint) &&
+        ((*sleAccount)[~sfFirstNFTokenSequence].value_or(0) +
+             (*sleAccount)[~sfMintedNFTokens].value_or(0) + seqDelta >
+         ctx.view.seq()))
+        return tecTOO_SOON;
+
     // Verify that the account does not own any objects that would prevent
     // the account from being deleted.
     Keylet const ownerDirKeylet{keylet::ownerDir(account)};

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

@@ -160,15 +160,66 @@ NFTokenMint::doApply()
             // Should not happen.  Checked in preclaim.
             return Unexpected(tecNO_ISSUER);
 
-        // Get the unique sequence number for this token:
-        std::uint32_t const tokenSeq = (*root)[~sfMintedNFTokens].value_or(0);
+        if (!ctx_.view().rules().enabled(fixNFTokenRemint))
         {
-            std::uint32_t const nextTokenSeq = tokenSeq + 1;
-            if (nextTokenSeq < tokenSeq)
-                return Unexpected(tecMAX_SEQUENCE_REACHED);
+            // Get the unique sequence number for this token:
+            std::uint32_t const tokenSeq =
+                (*root)[~sfMintedNFTokens].value_or(0);
+            {
+                std::uint32_t const nextTokenSeq = tokenSeq + 1;
+                if (nextTokenSeq < tokenSeq)
+                    return Unexpected(tecMAX_SEQUENCE_REACHED);
+
+                (*root)[sfMintedNFTokens] = nextTokenSeq;
+            }
+            ctx_.view().update(root);
+            return tokenSeq;
+        }
+
+        // With fixNFTokenRemint amendment enabled:
+        //
+        // If the issuer hasn't minted an NFToken before we must add a
+        // FirstNFTokenSequence field to the issuer's AccountRoot.  The
+        // value of the FirstNFTokenSequence must equal the issuer's
+        // current account sequence.
+        //
+        // There are three situations:
+        //  o If the first token is being minted by the issuer and
+        //     * If the transaction consumes a Sequence number, then the
+        //       Sequence has been pre-incremented by the time we get here in
+        //       doApply.  We must decrement the value in the Sequence field.
+        //     * Otherwise the transaction uses a Ticket so the Sequence has
+        //       not been pre-incremented.  We use the Sequence value as is.
+        //  o The first token is being minted by an authorized minter.  In
+        //    this case the issuer's Sequence field has been left untouched.
+        //    We use the issuer's Sequence value as is.
+        if (!root->isFieldPresent(sfFirstNFTokenSequence))
+        {
+            std::uint32_t const acctSeq = root->at(sfSequence);
 
-            (*root)[sfMintedNFTokens] = nextTokenSeq;
+            root->at(sfFirstNFTokenSequence) =
+                ctx_.tx.isFieldPresent(sfIssuer) ||
+                    ctx_.tx.getSeqProxy().isTicket()
+                ? acctSeq
+                : acctSeq - 1;
         }
+
+        std::uint32_t const mintedNftCnt =
+            (*root)[~sfMintedNFTokens].value_or(0u);
+
+        (*root)[sfMintedNFTokens] = mintedNftCnt + 1u;
+        if ((*root)[sfMintedNFTokens] == 0u)
+            return Unexpected(tecMAX_SEQUENCE_REACHED);
+
+        // Get the unique sequence number of this token by
+        // sfFirstNFTokenSequence + sfMintedNFTokens
+        std::uint32_t const offset = (*root)[sfFirstNFTokenSequence];
+        std::uint32_t const tokenSeq = offset + mintedNftCnt;
+
+        // Check for more overflow cases
+        if (tokenSeq + 1u == 0u || tokenSeq < offset)
+            return Unexpected(tecMAX_SEQUENCE_REACHED);
+
         ctx_.view().update(root);
         return tokenSeq;
     }();

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 = 57;
+static constexpr std::size_t numFeatures = 58;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -344,6 +344,7 @@ extern uint256 const featureDisallowIncoming;
 extern uint256 const featureXRPFees;
 extern uint256 const fixUniversalNumber;
 extern uint256 const fixNonFungibleTokensV1_2;
+extern uint256 const fixNFTokenRemint;
 
 }  // namespace ripple
 

src/ripple/protocol/SField.h

@@ -400,6 +400,7 @@ extern SF_UINT32 const sfMintedNFTokens;
 extern SF_UINT32 const sfBurnedNFTokens;
 extern SF_UINT32 const sfHookStateCount;
 extern SF_UINT32 const sfEmitGeneration;
+extern SF_UINT32 const sfFirstNFTokenSequence;
 
 // 64-bit integers (common)
 extern SF_UINT64 const sfIndexNext;

src/ripple/protocol/impl/Feature.cpp

@@ -454,6 +454,7 @@ REGISTER_FEATURE(DisallowIncoming,              Supported::yes, DefaultVote::no)
 REGISTER_FEATURE(XRPFees,                       Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixUniversalNumber,            Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixNonFungibleTokensV1_2,      Supported::yes, DefaultVote::no);
+REGISTER_FIX    (fixNFTokenRemint,              Supported::yes, DefaultVote::no);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/ripple/protocol/impl/LedgerFormats.cpp

@@ -55,6 +55,7 @@ LedgerFormats::LedgerFormats()
             {sfNFTokenMinter,        soeOPTIONAL},
             {sfMintedNFTokens,       soeDEFAULT},
             {sfBurnedNFTokens,       soeDEFAULT},
+            {sfFirstNFTokenSequence, soeOPTIONAL},
         },
         commonFields);
 

src/ripple/protocol/impl/SField.cpp

@@ -150,6 +150,9 @@ CONSTRUCT_TYPED_SFIELD(sfMintedNFTokens,        "MintedNFTokens",       UINT32,
 CONSTRUCT_TYPED_SFIELD(sfBurnedNFTokens,        "BurnedNFTokens",       UINT32,    44);
 CONSTRUCT_TYPED_SFIELD(sfHookStateCount,        "HookStateCount",       UINT32,    45);
 CONSTRUCT_TYPED_SFIELD(sfEmitGeneration,        "EmitGeneration",       UINT32,    46);
+// Three field values of 47, 48 and 49 are reserved for 
+// LockCount(Hooks), VoteWeight(AMM), DiscountedFee(AMM)
+CONSTRUCT_TYPED_SFIELD(sfFirstNFTokenSequence,  "FirstNFTokenSequence", UINT32,    50);
 
 // 64-bit integers (common)
 CONSTRUCT_TYPED_SFIELD(sfIndexNext,             "IndexNext",            UINT64,     1);

src/test/app/NFTokenBurn_test.cpp

@@ -380,8 +380,16 @@ class NFTokenBurn_test : public beast::unit_test::suite
             auto internalTaxon = [&env](
                                      Account const& acct,
                                      std::uint32_t taxon) -> std::uint32_t {
-                std::uint32_t const tokenSeq = {
-                    env.le(acct)->at(~sfMintedNFTokens).value_or(0)};
+                std::uint32_t tokenSeq =
+                    env.le(acct)->at(~sfMintedNFTokens).value_or(0);
+
+                // If fixNFTokenRemint amendment is on, we must
+                // add FirstNFTokenSequence.
+                if (env.current()->rules().enabled(fixNFTokenRemint))
+                    tokenSeq += env.le(acct)
+                                    ->at(~sfFirstNFTokenSequence)
+                                    .value_or(env.seq(acct));
+
                 return toUInt32(
                     nft::cipheredTaxon(tokenSeq, nft::toTaxon(taxon)));
             };
@@ -786,8 +794,10 @@ class NFTokenBurn_test : public beast::unit_test::suite
         FeatureBitset const all{supported_amendments()};
         FeatureBitset const fixNFTDir{fixNFTokenDirV1};
 
-        testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTDir);
-        testWithFeats(all - fixNonFungibleTokensV1_2);
+        testWithFeats(
+            all - fixNonFungibleTokensV1_2 - fixNFTDir - fixNFTokenRemint);
+        testWithFeats(all - fixNonFungibleTokensV1_2 - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };

src/test/app/NFTokenDir_test.cpp

@@ -190,8 +190,14 @@ class NFTokenDir_test : public beast::unit_test::suite
                     Account const& account = accounts.emplace_back(
                         Account::base58Seed, std::string(seed));
                     env.fund(XRP(10000), account);
-                    env.close();
+
+                    // Do not close the ledger inside the loop.  If
+                    // fixNFTokenRemint is enabled and accounts are initialized
+                    // at different ledgers, they will have different account
+                    // sequences.  That would cause the accounts to have
+                    // different NFTokenID sequence numbers.
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -408,8 +414,14 @@ class NFTokenDir_test : public beast::unit_test::suite
                     Account const& account = accounts.emplace_back(
                         Account::base58Seed, std::string(seed));
                     env.fund(XRP(10000), account);
-                    env.close();
+
+                    // Do not close the ledger inside the loop.  If
+                    // fixNFTokenRemint is enabled and accounts are initialized
+                    // at different ledgers, they will have different account
+                    // sequences.  That would cause the accounts to have
+                    // different NFTokenID sequence numbers.
                 }
+                env.close();
 
                 // All of the accounts create one NFT and and offer that NFT to
                 // buyer.
@@ -652,8 +664,14 @@ class NFTokenDir_test : public beast::unit_test::suite
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Do not close the ledger inside the loop.  If
+            // fixNFTokenRemint is enabled and accounts are initialized
+            // at different ledgers, they will have different account
+            // sequences.  That would cause the accounts to have
+            // different NFTokenID sequence numbers.
         }
+        env.close();
 
         // All of the accounts create one NFT and and offer that NFT to buyer.
         std::vector<uint256> nftIDs;
@@ -827,8 +845,14 @@ class NFTokenDir_test : public beast::unit_test::suite
             Account const& account =
                 accounts.emplace_back(Account::base58Seed, std::string(seed));
             env.fund(XRP(10000), account);
-            env.close();
+
+            // Do not close the ledger inside the loop.  If
+            // fixNFTokenRemint is enabled and accounts are initialized
+            // at different ledgers, they will have different account
+            // sequences.  That would cause the accounts to have
+            // different NFTokenID sequence numbers.
         }
+        env.close();
 
         // All of the accounts create seven consecutive NFTs and and offer
         // those NFTs to buyer.
@@ -1078,7 +1102,8 @@ class NFTokenDir_test : public beast::unit_test::suite
         FeatureBitset const fixNFTDir{
             fixNFTokenDirV1, featureNonFungibleTokensV1_1};
 
-        testWithFeats(all - fixNFTDir);
+        testWithFeats(all - fixNFTDir - fixNFTokenRemint);
+        testWithFeats(all - fixNFTokenRemint);
         testWithFeats(all);
     }
 };