From b21fd86f6ec879828daff1379fefd83fd7ce3bef Mon Sep 17 00:00:00 2001 From: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:56:33 +0000 Subject: [PATCH] fix: Fix assorted NFT and pDEX bugs (#7749) --- src/libxrpl/ledger/helpers/NFTokenHelpers.cpp | 16 +- src/libxrpl/tx/paths/OfferStream.cpp | 20 +++ .../tx/transactors/nft/NFTokenAcceptOffer.cpp | 9 ++ src/test/app/NFToken_test.cpp | 125 +++++++++++++++ src/test/app/PermissionedDEX_test.cpp | 145 ++++++++++++++++++ 5 files changed, 314 insertions(+), 1 deletion(-) diff --git a/src/libxrpl/ledger/helpers/NFTokenHelpers.cpp b/src/libxrpl/ledger/helpers/NFTokenHelpers.cpp index f3e4597558..ebe5271765 100644 --- a/src/libxrpl/ledger/helpers/NFTokenHelpers.cpp +++ b/src/libxrpl/ledger/helpers/NFTokenHelpers.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -773,6 +774,13 @@ tokenOfferCreatePreflight( return temBAD_AMOUNT; } + if (rules.enabled(fixCleanup3_4_0)) + { + // We don't allow a non-native currency to use the currency code XRP. + if (badAsset() == amount.asset()) + return temBAD_CURRENCY; + } + if (!isXRP(amount)) { if ((nftFlags & nft::kFlagOnlyXrp) != 0) @@ -851,7 +859,13 @@ tokenOfferCreatePreclaim( return tefNFTOKEN_IS_NOT_TRANSFERABLE; } - if (isFrozen(view, acctID, amount.get().currency, amount.getIssuer())) + // The IOU issuer is not subject to their own global freeze when the offer + // is denominated in their own IOU (e.g. receiving their own transfer fees), + // and they cannot hold a trust line to themselves. + bool const acctIsIouIssuer = + view.rules().enabled(fixCleanup3_4_0) && acctID == amount.getIssuer(); + if (!acctIsIouIssuer && + isFrozen(view, acctID, amount.get().currency, amount.getIssuer())) return tecFROZEN; // If this is an offer to buy the token, the account must have the diff --git a/src/libxrpl/tx/paths/OfferStream.cpp b/src/libxrpl/tx/paths/OfferStream.cpp index 2f2fef49f0..6884a113bd 100644 --- a/src/libxrpl/tx/paths/OfferStream.cpp +++ b/src/libxrpl/tx/paths/OfferStream.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -25,7 +26,9 @@ #include #include #include +#include #include +#include #include #include @@ -257,6 +260,23 @@ TOfferStreamBase::step() continue; } + // Post-fixCleanup3_4_0 defensive check: an offer indexed in a domain + // book must claim that same domain. This can only happen if the book + // directory is corrupt (i.e. a separate book indexing bug). An offer + // with no sfDomainID at all is just as wrong here: the domain + // membership check below is gated on that field being present, so + // such an offer would otherwise be consumed from a domain book + // without any credential check. + if (view_.rules().enabled(fixCleanup3_4_0) && book_.domain.has_value() && + (!entry->isFieldPresent(sfDomainID) || + entry->getFieldH256(sfDomainID) != *book_.domain)) + { + JLOG(j_.error()) << "Offer " << entry->key() + << " domain missing or does not match book domain"; + Throw( + tecINTERNAL, "Offer domain missing or does not match book domain."); + } + // Pre-fixCleanup3_3_0: validate domain membership for any book. // Post-fixCleanup3_3_0: only validate when walking a domain book. // Hybrid offers carry sfDomainID but also participate in the open diff --git a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp index 41bb051768..0cf7af1463 100644 --- a/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp +++ b/src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp @@ -8,12 +8,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -46,6 +48,13 @@ NFTokenAcceptOffer::preflight(PreflightContext const& ctx) if (*bf <= beast::kZero) return temMALFORMED; + + if (ctx.rules.enabled(fixCleanup3_4_0)) + { + // We don't allow a non-native currency to use the currency code XRP. + if (badAsset() == bf->asset()) + return temBAD_CURRENCY; + } } return tesSUCCESS; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 7fcd34640b..08c12e94d1 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -7355,6 +7356,127 @@ class NFTokenBaseUtil_test : public beast::unit_test::Suite } } + void + testCreateOfferInvalidAmount(FeatureBitset features) + { + testcase("Invalid NFT offer create amount"); + + using namespace test::jtx; + + // Before fixCleanup3_4_0, a fake-XRP offer amount (an IOU using the + // "XRP" currency code) is not rejected in preflight. With the amendment + // enabled, preflight rejects it with temBAD_CURRENCY. + for (bool const withFix : {false, true}) + { + Env env{*this, withFix ? features | fixCleanup3_4_0 : features - fixCleanup3_4_0}; + + Account const alice{"alice"}; + Account const gw{"gw"}; + + env.fund(XRP(1000), alice, gw); + env.close(); + + uint256 const nftID = token::getNextID(env, alice, 0, tfTransferable); + env(token::mint(alice, 0u), Txflags(tfTransferable)); + env.close(); + + // Fake XRP (an IOU using the "XRP" currency code) sell offer + // amount. + auto const bad = IOU(gw, badCurrency()); + env(token::createOffer(alice, nftID, bad(1)), + Txflags(tfSellNFToken), + Ter(withFix ? TER{temBAD_CURRENCY} : TER{tesSUCCESS})); + env.close(); + } + } + + void + testAcceptOfferInvalidBrokerFee(FeatureBitset features) + { + testcase("Invalid NFT offer accept broker fee"); + + using namespace test::jtx; + + // Before fixCleanup3_4_0, a fake-XRP broker fee (an IOU using the "XRP" + // currency code) is not rejected in preflight and reaches later offer + // validation instead. With the amendment enabled, preflight rejects it + // with temBAD_CURRENCY. + for (bool const withFix : {false, true}) + { + Env env{*this, withFix ? features | fixCleanup3_4_0 : features - fixCleanup3_4_0}; + + Account const alice{"alice"}; + Account const buyer{"buyer"}; + Account const broker{"broker"}; + Account const gw{"gw"}; + + env.fund(XRP(1000), alice, buyer, broker, gw); + env.close(); + + uint256 const nftID = token::getNextID(env, alice, 0, tfTransferable); + env(token::mint(alice, 0u), Txflags(tfTransferable)); + env.close(); + + uint256 const sellOfferIndex = + keylet::nftokenOffer(alice, SeqProxy::rawSequence(env.seq(alice))).key; + env(token::createOffer(alice, nftID, XRP(10)), Txflags(tfSellNFToken)); + env.close(); + + uint256 const buyOfferIndex = + keylet::nftokenOffer(buyer, SeqProxy::rawSequence(env.seq(buyer))).key; + env(token::createOffer(buyer, nftID, XRP(40)), token::Owner(alice)); + env.close(); + + // Fake XRP (an IOU using the "XRP" currency code) broker fee. + auto const bad = IOU(gw, badCurrency()); + env(token::brokerOffers(broker, buyOfferIndex, sellOfferIndex), + token::BrokerFee(bad(1)), + Ter(withFix ? TER{temBAD_CURRENCY} : TER{tecNFTOKEN_BUY_SELL_MISMATCH})); + env.close(); + } + } + + void + testCreateOfferIouIssuerGlobalFreeze(FeatureBitset features) + { + testcase("Create NFT offer by IOU issuer under global freeze"); + + using namespace test::jtx; + + // Before fixCleanup3_4_0, an IOU issuer that has set a global freeze on + // their own currency cannot create an NFToken offer denominated in that + // currency; the offer is rejected with tecFROZEN. With the amendment + // enabled, the issuer is not subject to their own global freeze when the + // offer is denominated in their own IOU (e.g. to receive their own + // transfer fees), so the offer succeeds. + for (bool const withFix : {false, true}) + { + Env env{*this, withFix ? features | fixCleanup3_4_0 : features - fixCleanup3_4_0}; + + Account const issuer{"issuer"}; + IOU const isISU(issuer["ISU"]); + + env.fund(XRP(1000), issuer); + env.close(); + + // issuer mints a transferable NFToken. + uint256 const nftID = token::getNextID(env, issuer, 0, tfTransferable); + env(token::mint(issuer, 0u), Txflags(tfTransferable)); + env.close(); + + // issuer sets a global freeze on their own IOU. + env(fset(issuer, asfGlobalFreeze)); + env.close(); + + // issuer creates a sell offer for the NFToken denominated in their + // own (globally frozen) IOU. + env(token::createOffer(issuer, nftID, isISU(100)), + Txflags(tfSellNFToken), + Ter(withFix ? TER{tesSUCCESS} : TER{tecFROZEN})); + env.close(); + } + } + protected: FeatureBitset const allFeatures_{test::jtx::testableAmendments()}; @@ -7397,6 +7519,9 @@ protected: testUnaskedForAutoTrustline(features); testNFTIssuerIsIOUIssuer(features); testNFTokenModify(features); + testCreateOfferInvalidAmount(features); + testAcceptOfferInvalidBrokerFee(features); + testCreateOfferIouIssuerGlobalFreeze(features); } public: diff --git a/src/test/app/PermissionedDEX_test.cpp b/src/test/app/PermissionedDEX_test.cpp index a7e4cd7615..ddb56a1480 100644 --- a/src/test/app/PermissionedDEX_test.cpp +++ b/src/test/app/PermissionedDEX_test.cpp @@ -2008,6 +2008,143 @@ class PermissionedDEX_test : public beast::unit_test::Suite } } + void + testDomainOfferInWrongBook(FeatureBitset features) + { + bool const fixEnabled = features[fixCleanup3_4_0]; + + testcase << "Domain offer indexed in the wrong domain book" + << (fixEnabled ? " (fixCleanup3_4_0 enabled)" : " (fixCleanup3_4_0 disabled)"); + + // Bob (a member of domains A and B) places an offer in domain A's + // book, which we then corrupt to claim domain B while it stays in + // domain A's book. A payment routed through domain A meets this offer. + // + // - With fixCleanup3_4_0: OfferStream sees the offer's domain (B) + // mismatch the book (A) and errors out -> tecPATH_PARTIAL. + // - Without it: OfferStream only checks the offer's own domain (B, + // which Bob is in), so it is used; the invariant then catches the + // mismatch -> tecINVARIANT_FAILED. + // + // Either way the payment fails and the offer is left untouched. + + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + // A second domain that Bob also belongs to. + Account const bobAcct = bob; + auto const domainID2 = + setupDomain(env, {bobAcct}, Account("permdex-domainOwner2"), "permdex-cred2"); + + // Bob places a domain offer in domain A's book. + auto const bobOfferSeq{env.seq(bob)}; + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + BEAST_EXPECT(checkOffer(env, bob, bobOfferSeq, XRP(10), USD(10), 0, true)); + + // Corrupt the offer: point its sfDomainID at domain B while it stays + // indexed in domain A's book directory. + auto const offerKey = keylet::offer(bob.id(), SeqProxy::rawSequence(bobOfferSeq)); + env.app().getOpenLedger().modify([&offerKey, &domainID2](OpenView& view, beast::Journal) { + auto const sle = view.read(offerKey); + if (!sle) + return false; + auto replacement = std::make_shared(*sle, sle->key()); + replacement->setFieldH256(sfDomainID, domainID2); + view.rawReplace(replacement); + return true; + }); + + if (fixEnabled) + { + // With the fix: OfferStream rejects the mismatched offer. + env(pay(alice, carol, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(tecPATH_PARTIAL)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + } + else + { + // Without the fix: the offer is used, then the invariant + // rejects the whole transaction. + env(pay(alice, carol, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(tecINVARIANT_FAILED)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + } + } + + void + testDomainBookOfferMissingDomain(FeatureBitset features) + { + bool const fixEnabled = features[fixCleanup3_4_0]; + + testcase << "Offer without a domain indexed in a domain book" + << (fixEnabled ? " (fixCleanup3_4_0 enabled)" : " (fixCleanup3_4_0 disabled)"); + + // Same corruption as testDomainOfferInWrongBook, except the offer + // loses sfDomainID entirely instead of pointing at another domain + // while it stays indexed in domain A's book. + // + // - With fixCleanup3_4_0: OfferStream sees an offer that claims no + // domain in a domain book and errors out -> tecPATH_PARTIAL. + // - Without it: neither the domain mismatch check nor the domain + // membership check fires (both are gated on sfDomainID being + // present), and the invariant does not catch it either because the + // offer is fully consumed and deleted. The payment succeeds using an + // offer that was never credential checked. + + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + // Bob places a domain offer in domain A's book. + auto const bobOfferSeq{env.seq(bob)}; + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + BEAST_EXPECT(checkOffer(env, bob, bobOfferSeq, XRP(10), USD(10), 0, true)); + + // Corrupt the offer: drop sfDomainID while it stays indexed in domain + // A's book directory. + auto const offerKey = keylet::offer(bob.id(), SeqProxy::rawSequence(bobOfferSeq)); + env.app().getOpenLedger().modify([&offerKey](OpenView& view, beast::Journal) { + auto const sle = view.read(offerKey); + if (!sle) + return false; + auto replacement = std::make_shared(*sle, sle->key()); + replacement->makeFieldAbsent(sfDomainID); + view.rawReplace(replacement); + return true; + }); + + auto const carolBefore = env.balance(carol, USD); + + if (fixEnabled) + { + // With the fix: OfferStream rejects the domainless offer. + env(pay(alice, carol, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(tecPATH_PARTIAL)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + BEAST_EXPECT(env.balance(carol, USD) - carolBefore == USD(0)); + } + else + { + // Without the fix: the offer is silently usable in the domain + // book, and the payment goes through. + env(pay(alice, carol, USD(10)), Path(~USD), Sendmax(XRP(10)), Domain(domainID)); + BEAST_EXPECT(!offerExists(env, bob, bobOfferSeq)); + BEAST_EXPECT(env.balance(carol, USD) - carolBefore == USD(10)); + } + } + void testReplaceDomainOfferWithOtherDomainOffer(FeatureBitset features) { @@ -2100,6 +2237,14 @@ public: // only after fixCleanup3_2_0. testCancelRegularOfferWithDomainCreate(all); testCancelRegularOfferWithDomainCreate(all - fixCleanup3_2_0); + + // A domain offer indexed in the wrong domain book is caught only + // after fixCleanup3_4_0. (Not an existing bug, but defensive testing) + testDomainOfferInWrongBook(all); + testDomainOfferInWrongBook(all - fixCleanup3_4_0); + testDomainBookOfferMissingDomain(all); + testDomainBookOfferMissingDomain(all - fixCleanup3_4_0); + testReplaceDomainOfferWithOtherDomainOffer(all); testReplaceDomainOfferWithOtherDomainOffer(all - fixCleanup3_4_0); }