diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 3b6aaf60ea1..0b61799ac35 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -270,6 +270,28 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) } } + // Fix a bug where the transfer of an NFToken with a transfer fee could + // give the NFToken issuer an undesired trust line. + if (ctx.view.rules().enabled(fixEnforceNFTokenTrustline)) + { + std::shared_ptr const& offer = bo ? bo : so; + if (!offer) + // Should be caught in preflight. + return tecINTERNAL; + + uint256 const& tokenID = offer->at(sfNFTokenID); + STAmount const& amount = offer->at(sfAmount); + if (nft::getTransferFee(tokenID) != 0 && + (nft::getFlags(tokenID) & nft::flagCreateTrustLines) == 0 && + !amount.native()) + { + auto const issuer = nft::getIssuer(tokenID); + // Issuer doesn't need a trust line to accept their own currency. + if (issuer != amount.getIssuer() && + !ctx.view.read(keylet::line(issuer, amount.issue()))) + return tecNO_LINE; + } + } return tesSUCCESS; } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 2119a954ca9..e0864540cc5 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -80,7 +80,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 = 75; +static constexpr std::size_t numFeatures = 76; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -368,6 +368,7 @@ extern uint256 const fixPreviousTxnID; extern uint256 const fixAMMv1_1; extern uint256 const featureNFTokenMintOffer; extern uint256 const fixReducedOffersV2; +extern uint256 const fixEnforceNFTokenTrustline; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index a1b73dbea8b..4434a7fbdc3 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -495,6 +495,7 @@ REGISTER_FIX (fixPreviousTxnID, Supported::yes, VoteBehavior::De REGISTER_FIX (fixAMMv1_1, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(NFTokenMintOffer, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixEnforceNFTokenTrustline, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index c8531273917..69432c85d17 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -7416,6 +7416,176 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } } + void + testUnaskedForAutoTrustline(FeatureBitset features) + { + testcase("Test fix unasked for auto-trustline."); + + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const becky{"becky"}; + Account const cheri{"cheri"}; + Account const gw("gw"); + IOU const gwAUD(gw["AUD"]); + + // This test case covers issue... + // https://github.com/XRPLF/rippled/issues/4925 + // + // For an NFToken with a transfer fee, the issuer must be able to + // accept the transfer fee or else a transfer should fail. If the + // NFToken is transferred for a non-XRP asset, then the issuer must + // have a trustline to that asset to receive the fee. + // + // This test looks at a situation where issuer would get a trustline + // for the fee without the issuer's consent. Here are the steps: + // 1. Issuer has a trustline (i.e., USD) + // 2. Issuer mints NFToken with transfer fee. + // 3. Becky acquires the NFToken, paying with XRP. + // 4. Becky creates offer to sell NFToken for USD(100). + // 5. Issuer deletes trustline for USD. + // 6. Carol buys NFToken from Becky for USD(100). + // 7. The transfer fee from Carol's purchase re-establishes issuer's + // USD trustline. + // + // The fixEnforceNFTokenTrustline amendment addresses this oversight. + // + // We run this test case both with and without + // fixEnforceNFTokenTrustline enabled so we can see the change + // in behavior. + // + // In both cases we remove the fixRemoveNFTokenAutoTrustLine amendment. + // Otherwise we can't create NFTokens with tfTrustLine enabled. + FeatureBitset const localFeatures = + features - fixRemoveNFTokenAutoTrustLine; + for (FeatureBitset feats : + {localFeatures - fixEnforceNFTokenTrustline, + localFeatures | fixEnforceNFTokenTrustline}) + { + Env env{*this, feats}; + env.fund(XRP(1000), issuer, becky, cheri, gw); + env.close(); + + // Set trust lines so becky and cheri can use gw's currency. + env(trust(becky, gwAUD(1000))); + env(trust(cheri, gwAUD(1000))); + env.close(); + env(pay(gw, cheri, gwAUD(500))); + env.close(); + + // issuer creates two NFTs: one with and one without AutoTrustLine. + std::uint16_t xferFee = 5000; // 5% + uint256 const nftAutoTrustID{token::getNextID( + env, issuer, 0u, tfTransferable | tfTrustLine, xferFee)}; + env(token::mint(issuer, 0u), + token::xferFee(xferFee), + txflags(tfTransferable | tfTrustLine)); + env.close(); + + uint256 const nftNoAutoTrustID{ + token::getNextID(env, issuer, 0u, tfTransferable, xferFee)}; + env(token::mint(issuer, 0u), + token::xferFee(xferFee), + txflags(tfTransferable)); + env.close(); + + // becky buys the nfts for 1 drop each. + { + uint256 const beckyBuyOfferIndex1 = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, drops(1)), + token::owner(issuer)); + + uint256 const beckyBuyOfferIndex2 = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftNoAutoTrustID, drops(1)), + token::owner(issuer)); + + env.close(); + env(token::acceptBuyOffer(issuer, beckyBuyOfferIndex1)); + env(token::acceptBuyOffer(issuer, beckyBuyOfferIndex2)); + env.close(); + } + + // becky creates offers to sell the nfts for AUD. + uint256 const beckyAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, gwAUD(100)), + txflags(tfSellNFToken)); + env.close(); + + // Creating an offer for the NFToken without tfTrustLine fails + // because issuer does not have a trust line for AUD. + env(token::createOffer(becky, nftNoAutoTrustID, gwAUD(100)), + txflags(tfSellNFToken), + ter(tecNO_LINE)); + env.close(); + + // issuer creates a trust line. Now the offer create for the + // NFToken without tfTrustLine succeeds. + BEAST_EXPECT(ownerCount(env, issuer) == 0); + env(trust(issuer, gwAUD(1000))); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 1); + + uint256 const beckyNoAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftNoAutoTrustID, gwAUD(100)), + txflags(tfSellNFToken)); + env.close(); + + // Now that the offers are in place, issuer removes the trustline. + BEAST_EXPECT(ownerCount(env, issuer) == 1); + env(trust(issuer, gwAUD(0))); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + + // cheri attempts to accept becky's offers. Behavior with the + // AutoTrustline NFT is uniform: issuer gets a new trust line. + env(token::acceptSellOffer(cheri, beckyAutoTrustOfferIndex)); + env.close(); + + // Here's evidence that issuer got the new AUD trust line. + BEAST_EXPECT(ownerCount(env, issuer) == 1); + BEAST_EXPECT(env.balance(issuer, gwAUD) == gwAUD(5)); + + // issuer once again removes the trust line for AUD. + env(pay(issuer, gw, gwAUD(5))); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + + // cheri attempts to accept the NoAutoTrustLine NFT. Behavior + // depends on whether fixEnforceNFTokenTrustline is enabled. + if (feats[fixEnforceNFTokenTrustline]) + { + // With fixEnforceNFTokenTrustline cheri can't accept the + // offer because issuer could not get their transfer fee + // without the appropriate trustline. + env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex), + ter(tecNO_LINE)); + env.close(); + + // But if issuer re-establishes the trustline then the offer + // can be accepted. + env(trust(issuer, gwAUD(1000))); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 1); + + env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex)); + env.close(); + } + else + { + // Without fixEnforceNFTokenTrustline the offer just works + // and issuer gets a trustline that they did not request. + env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex)); + env.close(); + } + BEAST_EXPECT(ownerCount(env, issuer) == 1); + BEAST_EXPECT(env.balance(issuer, gwAUD) == gwAUD(5)); + } // for feats + } + void testNFTIssuerIsIOUIssuer(FeatureBitset features) { @@ -7609,6 +7779,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite testFeatMintWithOffer(features); testTxJsonMetaFields(features); testFixNFTokenBuyerReserve(features); + testUnaskedForAutoTrustline(features); testNFTIssuerIsIOUIssuer(features); } diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index add436d5c59..3956adfbe0d 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5467,12 +5467,12 @@ class Offer_manual_test : public OfferBaseUtil_test } }; -BEAST_DEFINE_TESTSUITE_PRIO(OfferBaseUtil, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(OfferWOFlowCross, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(OfferWTakerDryOffer, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(OfferWOSmallQOffers, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(OfferWOFillOrKill, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(OfferAllFeatures, tx, ripple, 4); +BEAST_DEFINE_TESTSUITE_PRIO(OfferBaseUtil, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(OfferWOFlowCross, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(OfferWTakerDryOffer, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(OfferWOSmallQOffers, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(OfferWOFillOrKill, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(OfferAllFeatures, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(Offer_manual, tx, ripple, 20); } // namespace test