fixNFTokenReserve: ensure NFT tx fails when reserve is not met (#4767)

Without this amendment, an NFTokenAcceptOffer transaction can succeed
even when the NFToken recipient does not have sufficient reserves for
the new NFTokenPage. This allowed accounts to accept NFT sell offers
without having a sufficient reserve. (However, there was no issue in
brokered mode or when a buy offer is involved.)

Instead, the transaction should fail with `tecINSUFFICIENT_RESERVE` as
appropriate. The `fixNFTokenReserve` amendment adds checks in the
NFTokenAcceptOffer transactor to check if the OwnerCount changed. If it
did, then it checks the new reserve requirement.

Fix #4679

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

@@ -301,6 +301,60 @@ NFTokenAcceptOffer::pay(
     return tesSUCCESS;
 }
 
+TER
+NFTokenAcceptOffer::transferNFToken(
+    AccountID const& buyer,
+    AccountID const& seller,
+    uint256 const& nftokenID)
+{
+    auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID);
+
+    if (!tokenAndPage)
+        return tecINTERNAL;
+
+    if (auto const ret = nft::removeToken(
+            view(), seller, nftokenID, std::move(tokenAndPage->page));
+        !isTesSuccess(ret))
+        return ret;
+
+    auto const sleBuyer = view().read(keylet::account(buyer));
+    if (!sleBuyer)
+        return tecINTERNAL;
+
+    std::uint32_t const buyerOwnerCountBefore =
+        sleBuyer->getFieldU32(sfOwnerCount);
+
+    auto const insertRet =
+        nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
+
+    // if fixNFTokenReserve is enabled, check if the buyer has sufficient
+    // reserve to own a new object, if their OwnerCount changed.
+    //
+    // There was an issue where the buyer accepts a sell offer, the ledger
+    // didn't check if the buyer has enough reserve, meaning that buyer can get
+    // NFTs free of reserve.
+    if (view().rules().enabled(fixNFTokenReserve))
+    {
+        // To check if there is sufficient reserve, we cannot use mPriorBalance
+        // because NFT is sold for a price. So we must use the balance after
+        // the deduction of the potential offer price. A small caveat here is
+        // that the balance has already deducted the transaction fee, meaning
+        // that the reserve requirement is a few drops higher.
+        auto const buyerBalance = sleBuyer->getFieldAmount(sfBalance);
+
+        auto const buyerOwnerCountAfter = sleBuyer->getFieldU32(sfOwnerCount);
+        if (buyerOwnerCountAfter > buyerOwnerCountBefore)
+        {
+            if (auto const reserve =
+                    view().fees().accountReserve(buyerOwnerCountAfter);
+                buyerBalance < reserve)
+                return tecINSUFFICIENT_RESERVE;
+        }
+    }
+
+    return insertRet;
+}
+
 TER
 NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> const& offer)
 {
@@ -333,17 +387,7 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> const& offer)
     }
 
     // Now transfer the NFT:
-    auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID);
-
-    if (!tokenAndPage)
-        return tecINTERNAL;
-
-    if (auto const ret = nft::removeToken(
-            view(), seller, nftokenID, std::move(tokenAndPage->page));
-        !isTesSuccess(ret))
-        return ret;
-
-    return nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
+    return transferNFToken(buyer, seller, nftokenID);
 }
 
 TER
@@ -431,17 +475,8 @@ NFTokenAcceptOffer::doApply()
                 return r;
         }
 
-        auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID);
-
-        if (!tokenAndPage)
-            return tecINTERNAL;
-
-        if (auto const ret = nft::removeToken(
-                view(), seller, nftokenID, std::move(tokenAndPage->page));
-            !isTesSuccess(ret))
-            return ret;
-
-        return nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
+        // Now transfer the NFT:
+        return transferNFToken(buyer, seller, nftokenID);
     }
 
     if (bo)

src/ripple/app/tx/impl/NFTokenAcceptOffer.h

@@ -38,6 +38,12 @@ class NFTokenAcceptOffer : public Transactor
         std::shared_ptr<SLE> const& buy,
         std::shared_ptr<SLE> const& sell);
 
+    TER
+    transferNFToken(
+        AccountID const& buyer,
+        AccountID const& seller,
+        uint256 const& nfTokenID);
+
 public:
     static constexpr ConsequencesFactoryType ConsequencesFactory{Normal};
 

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 = 65;
+static constexpr std::size_t numFeatures = 66;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -352,6 +352,7 @@ extern uint256 const featureXChainBridge;
 extern uint256 const fixDisallowIncomingV1;
 extern uint256 const featureDID;
 extern uint256 const fixFillOrKill;
+extern uint256 const fixNFTokenReserve;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -459,6 +459,7 @@ REGISTER_FEATURE(XChainBridge,                  Supported::yes, VoteBehavior::De
 REGISTER_FIX    (fixDisallowIncomingV1,         Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(DID,                           Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX(fixFillOrKill,                     Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixNFTokenReserve,             Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.