fixNFTokenReserve: Throw error when NFT buyer does not meet reserve requirement

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

@@ -301,6 +301,30 @@ NFTokenAcceptOffer::pay(
     return tesSUCCESS;
 }
 
+TER
+NFTokenAcceptOffer::checkBuyerReserve(
+    std::shared_ptr<SLE const> const& sleBuyer,
+    std::uint32_t const buyerOwnerCountBefore)
+{
+    // 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 tesSUCCESS;
+}
+
 TER
 NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> const& offer)
 {
@@ -343,7 +367,34 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> const& offer)
         !isTesSuccess(ret))
         return ret;
 
-    return nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
+    auto const sleBuyer = view().read(keylet::account(buyer));
+    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.
+    //
+    // But, this isn't a problem when a buy offer is involved, because a buy
+    // offer is already taking up reserve. Since the buy offer is deleted
+    // near the end of the transaction, its reserve will be freed up, and at the
+    // same time, the new NFT that the buyer bought could take up that reserve
+    // again. So it's guareenteed that there is enough reserve as long the
+    // transactor pass the preclaim.
+    if (view().rules().enabled(fixNFTokenReserve))
+    {
+        if (auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore);
+            !isTesSuccess(ret))
+            return ret;
+    }
+
+    return insertRet;
 }
 
 TER
@@ -441,7 +492,28 @@ NFTokenAcceptOffer::doApply()
             !isTesSuccess(ret))
             return ret;
 
-        return nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
+        auto const sleBuyer = view().read(keylet::account(buyer));
+        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.
+        //
+        // However, this check is merely for safety and should never trigger,
+        // because, in brokered mode, the buy offer's reserve will be freed up,
+        // which can be taken up by the new NFT again. So buyer is always
+        // guarenteed to have enough reserve.
+        if (view().rules().enabled(fixNFTokenReserve))
+        {
+            if (auto const ret =
+                    checkBuyerReserve(sleBuyer, buyerOwnerCountBefore);
+                !isTesSuccess(ret))
+                return ret;
+        }
+        return insertRet;
     }
 
     if (bo)

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

@@ -38,6 +38,11 @@ class NFTokenAcceptOffer : public Transactor
         std::shared_ptr<SLE> const& buy,
         std::shared_ptr<SLE> const& sell);
 
+    TER
+    checkBuyerReserve(
+        std::shared_ptr<SLE const> const& sleBuyer,
+        std::uint32_t const buyerOwnerCountBefore);
+
 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 = 64;
+static constexpr std::size_t numFeatures = 65;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -351,6 +351,7 @@ extern uint256 const featureClawback;
 extern uint256 const featureXChainBridge;
 extern uint256 const fixDisallowIncomingV1;
 extern uint256 const featureDID;
+extern uint256 const fixNFTokenReserve;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -458,6 +458,7 @@ REGISTER_FEATURE(AMM,                           Supported::yes, VoteBehavior::De
 REGISTER_FEATURE(XChainBridge,                  Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixDisallowIncomingV1,         Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(DID,                           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.