ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-19 05:10:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Fix assorted NFT and pDEX bugs (#7749)

Browse Source
This commit is contained in:
Shawn Xie
2026-08-18 17:56:33 +00:00
committed by GitHub
parent f5f47f1cf5
commit b21fd86f6e
5 changed files with 314 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
src/libxrpl/ledger/helpers/NFTokenHelpers.cpp
View File

@@ -12,6 +12,7 @@
#include <xrpl/ledger/helpers/RippleStateHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
@@ -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<Issue>().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<Issue>().currency, amount.getIssuer()))
return tecFROZEN;
// If this is an offer to buy the token, the account must have the

20
src/libxrpl/tx/paths/OfferStream.cpp
View File

@@ -4,6 +4,7 @@
#include <xrpl/basics/Number.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/basics/chrono.h>
#include <xrpl/basics/contract.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/beast/utility/instrumentation.h>
@@ -25,7 +26,9 @@
#include <xrpl/protocol/Quality.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/XRPAmount.h>
#include <xrpl/tx/paths/detail/Steps.h>
#include <algorithm>
#include <optional>
@@ -257,6 +260,23 @@ TOfferStreamBase<TIn, TOut>::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<FlowException>(
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

9
src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp
View File

@@ -8,12 +8,14 @@
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/NFTokenHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/Rate.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/STTx.h>
#include <xrpl/protocol/TER.h>
@@ -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;

125
src/test/app/NFToken_test.cpp
View File

@@ -36,6 +36,7 @@
#include <xrpl/protocol/SeqProxy.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/TxFlags.h>
#include <xrpl/protocol/UintTypes.h>
#include <xrpl/protocol/jss.h>
#include <xrpl/protocol/nft.h>
@@ -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:

145
src/test/app/PermissionedDEX_test.cpp
View File

@@ -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, 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, 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);
}