Fix 3 issues around NFToken offer acceptance (#4380)

Fixes 3 issues:

In the following scenario, an account cannot perform NFTokenAcceptOffer even though it should be allowed to:

- BROKER has < S
- ALICE offers to sell token for S
- BOB offers to buy token for > S
- BROKER tries to bridge the two offers

This currently results in `tecINSUFFICIENT_FUNDS`, but should not because BROKER is not spending any funds in this transaction, beyond the transaction fee.

When trading an NFT using IOUs, and when the issuer of the IOU has any non-zero value set for TransferFee on their account via AccountSet (not a TransferFee on the NFT), and when the sale amount is equal to the total balance of that IOU that the buyer has, the resulting balance for the issuer of the IOU will become positive. This means that the buyer of the NFT was supposed to have caused a certain amount of IOU to be burned. That amount was unable to be burned because the buyer couldn't cover it. This results in the buyer owing this amount back to the issuer. In a real world scenario, this is appropriate and can be settled off-chain.

Currency issuers could not make offers for NFTs using their own currency, receiving `tecINSUFFICIENT_FUNDS` if they tried to do so.

With this fix, they are now able to buy/sell NFTs using their own currency.

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

@@ -168,10 +168,21 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
                 dest.has_value() && *dest != ctx.tx[sfAccount])
                 return tecNO_PERMISSION;
         }
+
         // The account offering to buy must have funds:
+        //
+        // After this amendment, we allow an IOU issuer to buy an NFT with their
+        // own currency
         auto const needed = bo->at(sfAmount);
-
-        if (accountHolds(
+        if (ctx.view.rules().enabled(fixUnburnableNFToken))
+        {
+            if (accountFunds(
+                    ctx.view, (*bo)[sfOwner], needed, fhZERO_IF_FROZEN, ctx.j) <
+                needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
+        else if (
+            accountHolds(
                 ctx.view,
                 (*bo)[sfOwner],
                 needed.getCurrency(),
@@ -206,15 +217,39 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
 
         // The account offering to buy must have funds:
         auto const needed = so->at(sfAmount);
-
-        if (accountHolds(
-                ctx.view,
-                ctx.tx[sfAccount],
-                needed.getCurrency(),
-                needed.getIssuer(),
-                fhZERO_IF_FROZEN,
-                ctx.j) < needed)
-            return tecINSUFFICIENT_FUNDS;
+        if (!ctx.view.rules().enabled(fixUnburnableNFToken))
+        {
+            if (accountHolds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    needed.getCurrency(),
+                    needed.getIssuer(),
+                    fhZERO_IF_FROZEN,
+                    ctx.j) < needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
+        else if (!bo)
+        {
+            // After this amendment, we allow buyers to buy with their own
+            // issued currency.
+            //
+            // In the case of brokered mode, this check is essentially
+            // redundant, since we have already confirmed that buy offer is >
+            // than the sell offer, and that the buyer can cover the buy
+            // offer.
+            //
+            // We also _must not_ check the tx submitter in brokered
+            // mode, because then we are confirming that the broker can
+            // cover what the buyer will pay, which doesn't make sense, causes
+            // an unncessary tec, and is also resolved with this amendment.
+            if (accountFunds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    needed,
+                    fhZERO_IF_FROZEN,
+                    ctx.j) < needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
     }
 
     return tesSUCCESS;
@@ -230,7 +265,22 @@ NFTokenAcceptOffer::pay(
     if (amount < beast::zero)
         return tecINTERNAL;
 
-    return accountSend(view(), from, to, amount, j_);
+    auto const result = accountSend(view(), from, to, amount, j_);
+
+    // After this amendment, if any payment would cause a non-IOU-issuer to
+    // have a negative balance, or an IOU-issuer to have a positive balance in
+    // their own currency, we know that something went wrong. This was
+    // originally found in the context of IOU transfer fees. Since there are
+    // several payouts in this tx, just confirm that the end state is OK.
+    if (!view().rules().enabled(fixUnburnableNFToken))
+        return result;
+    if (result != tesSUCCESS)
+        return result;
+    if (accountFunds(view(), from, amount, fhZERO_IF_FROZEN, j_).signum() < 0)
+        return tecINSUFFICIENT_FUNDS;
+    if (accountFunds(view(), to, amount, fhZERO_IF_FROZEN, j_).signum() < 0)
+        return tecINSUFFICIENT_FUNDS;
+    return tesSUCCESS;
 }
 
 TER

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

@@ -153,15 +153,28 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx)
     // offer may later become unfunded.
     if (!isSellOffer)
     {
-        auto const funds = accountHolds(
-            ctx.view,
-            ctx.tx[sfAccount],
-            amount.getCurrency(),
-            amount.getIssuer(),
-            FreezeHandling::fhZERO_IF_FROZEN,
-            ctx.j);
-
-        if (funds.signum() <= 0)
+        // After this amendment, we allow an IOU issuer to make a buy offer
+        // using their own currency.
+        if (ctx.view.rules().enabled(fixUnburnableNFToken))
+        {
+            if (accountFunds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    amount,
+                    FreezeHandling::fhZERO_IF_FROZEN,
+                    ctx.j)
+                    .signum() <= 0)
+                return tecUNFUNDED_OFFER;
+        }
+        else if (
+            accountHolds(
+                ctx.view,
+                ctx.tx[sfAccount],
+                amount.getCurrency(),
+                amount.getIssuer(),
+                FreezeHandling::fhZERO_IF_FROZEN,
+                ctx.j)
+                .signum() <= 0)
             return tecUNFUNDED_OFFER;
     }
 

src/ripple/ledger/View.h

@@ -97,6 +97,11 @@ accountHolds(
     FreezeHandling zeroIfFrozen,
     beast::Journal j);
 
+// Returns the amount an account can spend of the currency type saDefault, or
+// returns saDefault if this account is the issuer of the the currency in
+// question. Should be used in favor of accountHolds when questioning how much
+// an account can spend while also allowing currency issuers to spend
+// unlimited amounts of their own currency (since they can always issue more).
 [[nodiscard]] STAmount
 accountFunds(
     ReadView const& view,