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 @@ private:
         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 = 88;
+static constexpr std::size_t numFeatures = 89;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -376,6 +376,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

@@ -482,6 +482,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.

src/test/app/NFToken_test.cpp

@@ -6806,6 +6806,320 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testFixNFTokenBuyerReserve(FeatureBitset features)
+    {
+        testcase("Test buyer reserve when accepting an offer");
+
+        using namespace test::jtx;
+
+        // Lambda that mints an NFT and then creates a sell offer
+        auto mintAndCreateSellOffer = [](test::jtx::Env& env,
+                                         test::jtx::Account const& acct,
+                                         STAmount const amt) -> uint256 {
+            // acct mints a NFT
+            uint256 const nftId{
+                token::getNextID(env, acct, 0u, tfTransferable)};
+            env(token::mint(acct, 0u), txflags(tfTransferable));
+            env.close();
+
+            // acct makes an sell offer
+            uint256 const sellOfferIndex =
+                keylet::nftoffer(acct, env.seq(acct)).key;
+            env(token::createOffer(acct, nftId, amt), txflags(tfSellNFToken));
+            env.close();
+
+            return sellOfferIndex;
+        };
+
+        // Test the behaviors when the buyer makes an accept offer, both before
+        // and after enabling the amendment. Exercises the precise number of
+        // reserve in drops that's required to accept the offer
+        {
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+
+            Env env{*this, features};
+            auto const acctReserve = env.current()->fees().accountReserve(0);
+            auto const incReserve = env.current()->fees().increment;
+
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            // Bob is funded with minimum XRP reserve
+            env.fund(acctReserve, bob);
+            env.close();
+
+            // alice mints an NFT and create a sell offer for 0 XRP
+            auto const sellOfferIndex =
+                mintAndCreateSellOffer(env, alice, XRP(0));
+
+            // Bob owns no object
+            BEAST_EXPECT(ownerCount(env, bob) == 0);
+
+            // Without fixNFTokenReserve amendment, when bob accepts an NFT sell
+            // offer, he can get the NFT free of reserve
+            if (!features[fixNFTokenReserve])
+            {
+                // Bob is able to accept the offer
+                env(token::acceptSellOffer(bob, sellOfferIndex));
+                env.close();
+
+                // Bob now owns an extra objects
+                BEAST_EXPECT(ownerCount(env, bob) == 1);
+
+                // This is the wrong behavior, since Bob should need at least
+                // one incremental reserve.
+            }
+            // With fixNFTokenReserve, bob can no longer accept the offer unless
+            // there is enough reserve. A detail to note is that NFTs(sell
+            // offer) will not allow one to go below the reserve requirement,
+            // because buyer's balance is computed after the transaction fee is
+            // deducted. This means that the reserve requirement will be 10
+            // drops higher than normal.
+            else
+            {
+                // Bob is not able to accept the offer with only the account
+                // reserve (200,000,000 drops)
+                env(token::acceptSellOffer(bob, sellOfferIndex),
+                    ter(tecINSUFFICIENT_RESERVE));
+                env.close();
+
+                // after prev transaction, Bob owns 199,999,990 drops due to
+                // burnt tx fee
+
+                BEAST_EXPECT(ownerCount(env, bob) == 0);
+
+                // Send bob an increment reserve and 10 drops (to make up for
+                // the transaction fee burnt from the prev failed tx) Bob now
+                // owns 250,000,000 drops
+                env(pay(env.master, bob, incReserve + drops(10)));
+                env.close();
+
+                // However, this transaction will still fail because the reserve
+                // requirement is 10 drops higher
+                env(token::acceptSellOffer(bob, sellOfferIndex),
+                    ter(tecINSUFFICIENT_RESERVE));
+                env.close();
+
+                // Send bob an increment reserve and 20 drops
+                // Bob now owns 250,000,010 drops
+                env(pay(env.master, bob, drops(20)));
+                env.close();
+
+                // Bob is now able to accept the offer
+                env(token::acceptSellOffer(bob, sellOfferIndex));
+                env.close();
+
+                BEAST_EXPECT(ownerCount(env, bob) == 1);
+            }
+        }
+
+        // Now exercise the scenario when the buyer accepts
+        // many sell offers
+        {
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+
+            Env env{*this, features};
+            auto const acctReserve = env.current()->fees().accountReserve(0);
+            auto const incReserve = env.current()->fees().increment;
+
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            env.fund(acctReserve + XRP(1), bob);
+            env.close();
+
+            if (!features[fixNFTokenReserve])
+            {
+                // Bob can accept many NFTs without having a single reserve!
+                for (size_t i = 0; i < 200; i++)
+                {
+                    // alice mints an NFT and creates a sell offer for 0 XRP
+                    auto const sellOfferIndex =
+                        mintAndCreateSellOffer(env, alice, XRP(0));
+
+                    // Bob is able to accept the offer
+                    env(token::acceptSellOffer(bob, sellOfferIndex));
+                    env.close();
+                }
+            }
+            else
+            {
+                // alice mints the first NFT and creates a sell offer for 0 XRP
+                auto const sellOfferIndex1 =
+                    mintAndCreateSellOffer(env, alice, XRP(0));
+
+                // Bob cannot accept this offer because he doesn't have the
+                // reserve for the NFT
+                env(token::acceptSellOffer(bob, sellOfferIndex1),
+                    ter(tecINSUFFICIENT_RESERVE));
+                env.close();
+
+                // Give bob enough reserve
+                env(pay(env.master, bob, drops(incReserve)));
+                env.close();
+
+                BEAST_EXPECT(ownerCount(env, bob) == 0);
+
+                // Bob now owns his first NFT
+                env(token::acceptSellOffer(bob, sellOfferIndex1));
+                env.close();
+
+                BEAST_EXPECT(ownerCount(env, bob) == 1);
+
+                // alice now mints 31 more NFTs and creates an offer for each
+                // NFT, then sells to bob
+                for (size_t i = 0; i < 31; i++)
+                {
+                    // alice mints an NFT and creates a sell offer for 0 XRP
+                    auto const sellOfferIndex =
+                        mintAndCreateSellOffer(env, alice, XRP(0));
+
+                    // Bob can accept the offer because the new NFT is stored in
+                    // an existing NFTokenPage so no new reserve is requried
+                    env(token::acceptSellOffer(bob, sellOfferIndex));
+                    env.close();
+                }
+
+                BEAST_EXPECT(ownerCount(env, bob) == 1);
+
+                // alice now mints the 33rd NFT and creates an sell offer for 0
+                // XRP
+                auto const sellOfferIndex33 =
+                    mintAndCreateSellOffer(env, alice, XRP(0));
+
+                // Bob fails to accept this NFT because he does not have enough
+                // reserve for a new NFTokenPage
+                env(token::acceptSellOffer(bob, sellOfferIndex33),
+                    ter(tecINSUFFICIENT_RESERVE));
+                env.close();
+
+                // Send bob incremental reserve
+                env(pay(env.master, bob, drops(incReserve)));
+                env.close();
+
+                // Bob now has enough reserve to accept the offer and now
+                // owns one more NFTokenPage
+                env(token::acceptSellOffer(bob, sellOfferIndex33));
+                env.close();
+
+                BEAST_EXPECT(ownerCount(env, bob) == 2);
+            }
+        }
+
+        // Test the behavior when the seller accepts a buy offer.
+        // The behavior should not change regardless whether fixNFTokenReserve
+        // is enabled or not, since the ledger is able to guard against
+        // free NFTokenPages when buy offer is accepted. This is merely an
+        // additional test to exercise existing offer behavior.
+        {
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+
+            Env env{*this, features};
+            auto const acctReserve = env.current()->fees().accountReserve(0);
+            auto const incReserve = env.current()->fees().increment;
+
+            env.fund(XRP(10000), alice);
+            env.close();
+
+            // Bob is funded with account reserve + increment reserve + 1 XRP
+            // increment reserve is for the buy offer, and 1 XRP is for offer
+            // price
+            env.fund(acctReserve + incReserve + XRP(1), bob);
+            env.close();
+
+            // Alice mints a NFT
+            uint256 const nftId{
+                token::getNextID(env, alice, 0u, tfTransferable)};
+            env(token::mint(alice, 0u), txflags(tfTransferable));
+            env.close();
+
+            // Bob makes a buy offer for 1 XRP
+            auto const buyOfferIndex = keylet::nftoffer(bob, env.seq(bob)).key;
+            env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice));
+            env.close();
+
+            // accepting the buy offer fails because bob's balance is 10 drops
+            // lower than the required amount, since the previous tx burnt 10
+            // drops for tx fee.
+            env(token::acceptBuyOffer(alice, buyOfferIndex),
+                ter(tecINSUFFICIENT_FUNDS));
+            env.close();
+
+            // send Bob 10 drops
+            env(pay(env.master, bob, drops(10)));
+            env.close();
+
+            // Now bob can buy the offer
+            env(token::acceptBuyOffer(alice, buyOfferIndex));
+            env.close();
+        }
+
+        // Test the reserve behavior in brokered mode.
+        // The behavior should not change regardless whether fixNFTokenReserve
+        // is enabled or not, since the ledger is able to guard against
+        // free NFTokenPages in brokered mode. This is merely an
+        // additional test to exercise existing offer behavior.
+        {
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const broker{"broker"};
+
+            Env env{*this, features};
+            auto const acctReserve = env.current()->fees().accountReserve(0);
+            auto const incReserve = env.current()->fees().increment;
+
+            env.fund(XRP(10000), alice, broker);
+            env.close();
+
+            // Bob is funded with account reserve + incr reserve + 1 XRP(offer
+            // price)
+            env.fund(acctReserve + incReserve + XRP(1), bob);
+            env.close();
+
+            // Alice mints a NFT
+            uint256 const nftId{
+                token::getNextID(env, alice, 0u, tfTransferable)};
+            env(token::mint(alice, 0u), txflags(tfTransferable));
+            env.close();
+
+            // Alice creates sell offer and set broker as destination
+            uint256 const offerAliceToBroker =
+                keylet::nftoffer(alice, env.seq(alice)).key;
+            env(token::createOffer(alice, nftId, XRP(1)),
+                token::destination(broker),
+                txflags(tfSellNFToken));
+            env.close();
+
+            // Bob creates buy offer
+            uint256 const offerBobToBroker =
+                keylet::nftoffer(bob, env.seq(bob)).key;
+            env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice));
+            env.close();
+
+            // broker offers.
+            // Returns insufficient funds, because bob burnt tx fee when he
+            // created his buy offer, which makes his spendable balance to be
+            // less than the required amount.
+            env(token::brokerOffers(
+                    broker, offerBobToBroker, offerAliceToBroker),
+                ter(tecINSUFFICIENT_FUNDS));
+            env.close();
+
+            // send Bob 10 drops
+            env(pay(env.master, bob, drops(10)));
+            env.close();
+
+            // broker offers.
+            env(token::brokerOffers(
+                broker, offerBobToBroker, offerAliceToBroker));
+            env.close();
+        }
+    }
+
     void
     testWithFeats(FeatureBitset features)
     {
@@ -6839,6 +7153,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite
         testBrokeredSaleToSelf(features);
         testFixNFTokenRemint(features);
         testTxJsonMetaFields(features);
+        testFixNFTokenBuyerReserve(features);
     }
 
 public:
@@ -6849,12 +7164,15 @@ public:
         static FeatureBitset const all{supported_amendments()};
         static FeatureBitset const fixNFTDir{fixNFTokenDirV1};
 
-        static std::array<FeatureBitset, 5> const feats{
-            all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint,
+        static std::array<FeatureBitset, 6> const feats{
+            all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint -
+                fixNFTokenReserve,
             all - disallowIncoming - fixNonFungibleTokensV1_2 -
-                fixNFTokenRemint,
-            all - fixNonFungibleTokensV1_2 - fixNFTokenRemint,
-            all - fixNFTokenRemint,
+                fixNFTokenRemint - fixNFTokenReserve,
+            all - fixNonFungibleTokensV1_2 - fixNFTokenRemint -
+                fixNFTokenReserve,
+            all - fixNFTokenRemint - fixNFTokenReserve,
+            all - fixNFTokenReserve,
             all};
 
         if (BEAST_EXPECT(instance < feats.size()))
@@ -6898,12 +7216,21 @@ class NFTokenWOTokenRemint_test : public NFTokenBaseUtil_test
     }
 };
 
+class NFTokenWOTokenReserve_test : public NFTokenBaseUtil_test
+{
+    void
+    run() override
+    {
+        NFTokenBaseUtil_test::run(4);
+    }
+};
+
 class NFTokenAllFeatures_test : public NFTokenBaseUtil_test
 {
     void
     run() override
     {
-        NFTokenBaseUtil_test::run(4, true);
+        NFTokenBaseUtil_test::run(5, true);
     }
 };
 
@@ -6911,6 +7238,7 @@ BEAST_DEFINE_TESTSUITE_PRIO(NFTokenBaseUtil, tx, ripple, 2);
 BEAST_DEFINE_TESTSUITE_PRIO(NFTokenDisallowIncoming, tx, ripple, 2);
 BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOfixV1, tx, ripple, 2);
 BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenRemint, tx, ripple, 2);
+BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenReserve, tx, ripple, 2);
 BEAST_DEFINE_TESTSUITE_PRIO(NFTokenAllFeatures, tx, ripple, 2);
 
 }  // namespace ripple