diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 02471c1d482..c27fb188028 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -270,6 +270,34 @@ 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(fixNFTokenTrustlineSurprise)) + { + std::shared_ptr const& offer = bo ? bo : so; + if (!offer) + // Should be caught in preflight. + return tecINTERNAL; + + // - If the NFToken has a transfer fee, and + // - If the NFToken doesn't have the flagCreateTrustLines flag set, and + // - If the Amount is not XRP, and + // - If the NFToken issuer is not the Amount issuer, and + // - If the NFToken issuer does not have a trust line for the Amount + // - Then reject the token accept. + 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/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index 22eca2dffdd..aab306c6b34 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -123,8 +123,19 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx) if (!ctx.view.exists(keylet::account(issuer))) return tecNO_ISSUER; - if (!ctx.view.exists(keylet::line(issuer, amount.issue()))) + // There was a bug in a corner case that fixNFTokenTrustlineSurprise + // addresses. If the IOU issuer and the NFToken issuer are the same, + // then that issuer does not need a trust line to accept their fee. + if (ctx.view.rules().enabled(fixNFTokenTrustlineSurprise)) + { + if (issuer != amount.getIssuer() && + !ctx.view.read(keylet::line(issuer, amount.issue()))) + return tecNO_LINE; + } + else if (!ctx.view.exists(keylet::line(issuer, amount.issue()))) + { return tecNO_LINE; + } if (isFrozen( ctx.view, issuer, amount.getCurrency(), amount.getIssuer())) diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 95a13d44f0e..29022498068 100644 --- a/src/ripple/protocol/Feature.h +++ b/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 = 68; +static constexpr std::size_t numFeatures = 69; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -355,6 +355,7 @@ extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; extern uint256 const fixInnerObjTemplate; extern uint256 const featurePriceOracle; +extern uint256 const fixNFTokenTrustlineSurprise; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 526ef5982fc..daeeb2baf90 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -460,8 +460,9 @@ REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::De REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); -REGISTER_FIX(fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixInnerObjTemplate, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(PriceOracle, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNFTokenTrustlineSurprise, 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 8740b521132..10df546bd9e 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -7112,6 +7112,342 @@ 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 fixNFTokenTrustlineSurprise amendment addresses this oversight. + // + // We run this test case both with and without + // fixNFTokenTrustlineSurprise 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 - fixNFTokenTrustlineSurprise, + localFeatures | fixNFTokenTrustlineSurprise}) + { + 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 fixNFTokenTrustlineSurprise is enabled. + if (feats[fixNFTokenTrustlineSurprise]) + { + // With fixNFTokenTrustlineSurprise 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 fixNFTokenTrustlineSurprise 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) + { + testcase("Test fix NFT issuer is IOU issuer"); + + using namespace test::jtx; + + Account const issuer{"issuer"}; + Account const becky{"becky"}; + Account const cheri{"cheri"}; + IOU const isISU(issuer["ISU"]); + + // This test case covers issue... + // https://github.com/XRPLF/rippled/issues/4941 + // + // If an NFToken has a transfer fee then, when an offer is accepted, + // a portion of the sale price goes to the issuer. + // + // It is possible for an issuer to issue both an IOU (for remittances) + // and NFTokens. If the issuer's IOU is used to pay for the transfer + // of one of the issuer's NFTokens, then paying the fee for that + // transfer will fail with a tecNO_LINE. + // + // The problem occurs because the NFT code looks for a trust line to + // pay the transfer fee. However the issuer of an IOU does not need + // a trust line to accept their own issuance and, in fact, is not + // allowed to have a trust line to themselves. + // + // This test looks at a situation where transfer of an NFToken is + // prevented by this bug: + // 1. Issuer issues an IOU (e.g, isISU). + // 2. Becky and Cheri get trust lines for, and acquire, some isISU. + // 3. Issuer mints NFToken with transfer fee. + // 4. Becky acquires the NFToken, paying with XRP. + // 5. Becky attempts to create an offer to sell the NFToken for + // isISU(100). The attempt fails with `tecNO_LINE`. + // + // The fixNFTokenTrustlineSurprise amendment addresses this oversight. + // + // We run this test case both with and without + // fixNFTokenTrustlineSurprise 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 - fixNFTokenTrustlineSurprise, + localFeatures | fixNFTokenTrustlineSurprise}) + { + Env env{*this, feats}; + env.fund(XRP(1000), issuer, becky, cheri); + env.close(); + + // Set trust lines so becky and cheri can use isISU. + env(trust(becky, isISU(1000))); + env(trust(cheri, isISU(1000))); + env.close(); + env(pay(issuer, cheri, isISU(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(); + } + + // Behavior from here down diverges significantly based on + // fixNFTokenTrustlineSurprise. + if (!feats[fixNFTokenTrustlineSurprise]) + { + // Without fixNFTokenTrustlineSurprise becky simply can't + // create an offer for a non-tfTrustLine NFToken that would + // pay the transfer fee in issuer's own IOU. + env(token::createOffer(becky, nftNoAutoTrustID, isISU(100)), + txflags(tfSellNFToken), + ter(tecNO_LINE)); + env.close(); + + // And issuer can't create a trust line to themselves. + env(trust(issuer, isISU(1000)), ter(temDST_IS_SRC)); + env.close(); + + // However if the NFToken has the tfTrustLine flag set, + // then becky can create the offer. + uint256 const beckyAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + + // And cheri can accept the offer. + env(token::acceptSellOffer(cheri, beckyAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // ISU(5) has disappeared out of cheri's and becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(95)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(400)); + } + else + { + // With fixNFTokenTrustlineSurprise things go better. + // becky creates offers to sell the nfts for ISU. + uint256 const beckyNoAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftNoAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + uint256 const beckyAutoTrustOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftAutoTrustID, isISU(100)), + txflags(tfSellNFToken)); + env.close(); + + // cheri accepts becky's offers. Behavior is uniform: + // issuer gets paid. + env(token::acceptSellOffer(cheri, beckyAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // ISU(5) has disappeared out of cheri's and becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(95)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(400)); + + env(token::acceptSellOffer(cheri, beckyNoAutoTrustOfferIndex)); + env.close(); + + // We verify that issuer got their transfer fee by seeing that + // an additional ISU(5) has disappeared out of cheri's and + // becky's balances. + BEAST_EXPECT(env.balance(becky, isISU) == isISU(190)); + BEAST_EXPECT(env.balance(cheri, isISU) == isISU(300)); + } + } // for feats + } + void testWithFeats(FeatureBitset features) { @@ -7146,6 +7482,8 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite testFixNFTokenRemint(features); testTxJsonMetaFields(features); testFixNFTokenBuyerReserve(features); + testUnaskedForAutoTrustline(features); + testNFTIssuerIsIOUIssuer(features); } public: @@ -7226,11 +7564,11 @@ class NFTokenAllFeatures_test : public NFTokenBaseUtil_test } }; -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); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenBaseUtil, tx, ripple, 3); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenDisallowIncoming, tx, ripple, 3); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOfixV1, tx, ripple, 3); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenRemint, tx, ripple, 3); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenReserve, tx, ripple, 3); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenAllFeatures, tx, ripple, 3); } // namespace ripple diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 95ffd9f3aee..d3af09c0517 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5439,12 +5439,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