From 64f508fc27f5f76d87510c00df371854682a7b14 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 22 Jun 2022 17:15:23 -0700 Subject: [PATCH] Introduce fixNFTokenNegOffer amendment. It addresses two issues: - The primary point is to forbid negative amounts in NFTokenOffers. - The secondary point is to allow a Destination field in an NFToken buy offer so a specific broker can be specified. --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 23 ++ src/ripple/app/tx/impl/NFTokenCreateOffer.cpp | 17 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 3 +- src/test/app/NFToken_test.cpp | 350 +++++++++++++++++- 5 files changed, 371 insertions(+), 25 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 7c78f175f63..fb5f51c7251 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -75,6 +75,12 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration])) return {nullptr, tecEXPIRED}; + // The initial implementation had a bug that allowed a negative + // amount. The fixNFTokenNegOffer amendment fixes that. + if ((*offerSLE)[sfAmount].negative() && + ctx.view.rules().enabled(fixNFTokenNegOffer)) + return {nullptr, temBAD_OFFER}; + return {std::move(offerSLE), tesSUCCESS}; } return {nullptr, tesSUCCESS}; @@ -103,6 +109,14 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if ((*so)[sfAmount] > (*bo)[sfAmount]) return tecINSUFFICIENT_PAYMENT; + // If the buyer specified a destination, that destination must be + // the seller or the broker. + if (auto const dest = bo->at(~sfDestination)) + { + if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + return tecNFTOKEN_BUY_SELL_MISMATCH; + } + // If the seller specified a destination, that destination must be // the buyer or the broker. if (auto const dest = so->at(~sfDestination)) @@ -142,6 +156,15 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) !nft::findToken(ctx.view, ctx.tx[sfAccount], (*bo)[sfNFTokenID])) return tecNO_PERMISSION; + // If not in bridged mode... + if (!so) + { + // If the offer has a Destination field, the acceptor must be the + // Destination. + if (auto const dest = bo->at(~sfDestination); + dest.has_value() && *dest != ctx.tx[sfAccount]) + return tecNO_PERMISSION; + } // The account offering to buy must have funds: auto const needed = bo->at(sfAmount); diff --git a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index bf92472e2ce..80e4c3964a7 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -46,7 +46,11 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) auto const nftFlags = nft::getFlags(ctx.tx[sfNFTokenID]); { - auto const amount = ctx.tx[sfAmount]; + STAmount const amount = ctx.tx[sfAmount]; + + if (amount.negative() && ctx.rules.enabled(fixNFTokenNegOffer)) + // An offer for a negative amount makes no sense. + return temBAD_AMOUNT; if (!isXRP(amount)) { @@ -78,9 +82,14 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) if (auto dest = ctx.tx[~sfDestination]) { - // The destination field is only valid on a sell offer; it makes no - // sense in a buy offer. - if (!isSellOffer) + // Some folks think it makes sense for a buy offer to specify a + // specific broker using the Destination field. This change doesn't + // deserve it's own amendment, so we're piggy-backing on + // fixNFTokenNegOffer. + // + // Prior to fixNFTokenNegOffer any use of the Destination field on + // a buy offer was malformed. + if (!isSellOffer && !ctx.rules.enabled(fixNFTokenNegOffer)) return temMALFORMED; // The destination can't be the account executing the transaction. diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index b3ecb099bcc..7164cd46e4e 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 = 48; +static constexpr std::size_t numFeatures = 49; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -336,6 +336,7 @@ extern uint256 const featureCheckCashMakesTrustLine; extern uint256 const featureNonFungibleTokensV1; extern uint256 const featureExpandedSignerList; extern uint256 const fixNFTokenDirV1; +extern uint256 const fixNFTokenNegOffer; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index f6f67c003bc..1fcca56e884 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -431,7 +431,7 @@ REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes); REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes); -REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes); REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes); @@ -440,6 +440,7 @@ REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, DefaultVote::no) REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, DefaultVote::no); REGISTER_FEATURE(ExpandedSignerList, Supported::yes, DefaultVote::no); REGISTER_FIX (fixNFTokenDirV1, Supported::yes, DefaultVote::no); +REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 7dd4a781286..5a25b3670b8 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -2666,7 +2666,7 @@ class NFToken_test : public beast::unit_test::suite env.close(); // Test how adding a Destination field to an offer affects permissions - // for cancelling offers. + // for canceling offers. { uint256 const offerMinterToIssuer = keylet::nftoffer(minter, env.seq(minter)).key; @@ -2680,14 +2680,20 @@ class NFToken_test : public beast::unit_test::suite token::destination(buyer), txflags(tfSellNFToken)); - // buy offers cannot contain a Destination, so this attempt fails. + uint256 const offerIssuerToMinter = + keylet::nftoffer(issuer, env.seq(issuer)).key; env(token::createOffer(issuer, nftokenID, drops(1)), token::owner(minter), - token::destination(minter), - ter(temMALFORMED)); + token::destination(minter)); + + uint256 const offerIssuerToBuyer = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftokenID, drops(1)), + token::owner(minter), + token::destination(buyer)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == 2); BEAST_EXPECT(ownerCount(env, minter) == 3); BEAST_EXPECT(ownerCount(env, buyer) == 0); @@ -2702,8 +2708,12 @@ class NFToken_test : public beast::unit_test::suite ter(tecNO_PERMISSION)); env(token::cancelOffer(buyer, {offerMinterToIssuer}), ter(tecNO_PERMISSION)); + env(token::cancelOffer(buyer, {offerIssuerToMinter}), + ter(tecNO_PERMISSION)); + env(token::cancelOffer(minter, {offerIssuerToBuyer}), + ter(tecNO_PERMISSION)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == 2); BEAST_EXPECT(ownerCount(env, minter) == 3); BEAST_EXPECT(ownerCount(env, buyer) == 0); @@ -2711,6 +2721,8 @@ class NFToken_test : public beast::unit_test::suite // cancel the offers. env(token::cancelOffer(buyer, {offerMinterToBuyer})); env(token::cancelOffer(minter, {offerMinterToIssuer})); + env(token::cancelOffer(buyer, {offerIssuerToBuyer})); + env(token::cancelOffer(issuer, {offerIssuerToMinter})); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -2720,7 +2732,7 @@ class NFToken_test : public beast::unit_test::suite // Test how adding a Destination field to a sell offer affects // accepting that offer. { - uint256 const offerMinterToBuyer = + uint256 const offerMinterSellsToBuyer = keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID, drops(1)), token::destination(buyer), @@ -2732,7 +2744,7 @@ class NFToken_test : public beast::unit_test::suite // issuer cannot accept a sell offer where they are not the // destination. - env(token::acceptSellOffer(issuer, offerMinterToBuyer), + env(token::acceptSellOffer(issuer, offerMinterSellsToBuyer), ter(tecNO_PERMISSION)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); @@ -2740,36 +2752,61 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, buyer) == 0); // However buyer can accept the sell offer. - env(token::acceptSellOffer(buyer, offerMinterToBuyer)); + env(token::acceptSellOffer(buyer, offerMinterSellsToBuyer)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 0); BEAST_EXPECT(ownerCount(env, buyer) == 1); } - // You can't add a Destination field to a buy offer. + // Test how adding a Destination field to a buy offer affects + // accepting that offer. { + uint256 const offerMinterBuysFromBuyer = + keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID, drops(1)), token::owner(buyer), - token::destination(buyer), - ter(temMALFORMED)); + token::destination(buyer)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 1); - // However without the Destination the buy offer works fine. - uint256 const offerMinterToBuyer = - keylet::nftoffer(minter, env.seq(minter)).key; - env(token::createOffer(minter, nftokenID, drops(1)), - token::owner(buyer)); + // issuer cannot accept a buy offer where they are the + // destination. + env(token::acceptBuyOffer(issuer, offerMinterBuysFromBuyer), + ter(tecNO_PERMISSION)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 1); // Buyer accepts minter's offer. - env(token::acceptBuyOffer(buyer, offerMinterToBuyer)); + env(token::acceptBuyOffer(buyer, offerMinterBuysFromBuyer)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + + // If a destination other than the NFToken owner is set, that + // destination must act as a broker. The NFToken owner may not + // simply accept the offer. + uint256 const offerBuyerBuysFromMinter = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftokenID, drops(1)), + token::owner(minter), + token::destination(broker)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + + env(token::acceptBuyOffer(minter, offerBuyerBuysFromMinter), + ter(tecNO_PERMISSION)); + env.close(); + + // Clean up the unused offer. + env(token::cancelOffer(buyer, {offerBuyerBuysFromMinter})); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -2856,6 +2893,47 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, issuer) == 1); BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 0); + + // Clean out the unconsumed offer. + env(token::cancelOffer(issuer, {offerIssuerToBuyer})); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + } + + // Show that if a buy and a sell offer both have the same destination, + // then that destination can broker the offers. + { + uint256 const offerMinterToBroker = + keylet::nftoffer(minter, env.seq(minter)).key; + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(broker), + txflags(tfSellNFToken)); + + uint256 const offerBuyerToBroker = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftokenID, drops(1)), + token::owner(minter), + token::destination(broker)); + + // Cannot broker offers when the sell destination is not the buyer + // or the broker. + env(token::brokerOffers( + issuer, offerBuyerToBroker, offerMinterToBroker), + ter(tecNFTOKEN_BUY_SELL_MISMATCH)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + + // Broker is successful if they are the destination of both offers. + env(token::brokerOffers( + broker, offerBuyerToBroker, offerMinterToBroker)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == 1); } } @@ -4557,6 +4635,239 @@ class NFToken_test : public beast::unit_test::suite checkOffers("nft_buy_offers", 501, 2, __LINE__); } + void + testFixNFTokenNegOffer(FeatureBitset features) + { + // Exercise changes introduced by fixNFTokenNegOffer. + using namespace test::jtx; + + testcase("fixNFTokenNegOffer"); + + Account const issuer{"issuer"}; + Account const buyer{"buyer"}; + Account const gw{"gw"}; + IOU const gwXAU(gw["XAU"]); + + // Test both with and without fixNFTokenNegOffer + for (auto const& tweakedFeatures : + {features - fixNFTokenNegOffer, features | fixNFTokenNegOffer}) + { + // There was a bug in the initial NFT implementation that + // allowed offers to be placed with negative amounts. Verify + // that fixNFTokenNegOffer addresses the problem. + Env env{*this, tweakedFeatures}; + + env.fund(XRP(1000000), issuer, buyer, gw); + env.close(); + + env(trust(issuer, gwXAU(2000))); + env(trust(buyer, gwXAU(2000))); + env.close(); + + env(pay(gw, issuer, gwXAU(1000))); + env(pay(gw, buyer, gwXAU(1000))); + env.close(); + + // Create an NFT that we'll make XRP offers for. + uint256 const nftID0{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + // Create an NFT that we'll make IOU offers for. + uint256 const nftID1{ + token::getNextID(env, issuer, 1u, tfTransferable)}; + env(token::mint(issuer, 1), txflags(tfTransferable)); + env.close(); + + TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(temBAD_AMOUNT) + : static_cast(tesSUCCESS); + + // Make offers with negative amounts for the NFTs + uint256 const sellNegXrpOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID0, XRP(-2)), + txflags(tfSellNFToken), + ter(offerCreateTER)); + env.close(); + + uint256 const sellNegIouOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID1, gwXAU(-2)), + txflags(tfSellNFToken), + ter(offerCreateTER)); + env.close(); + + uint256 const buyNegXrpOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID0, XRP(-1)), + token::owner(issuer), + ter(offerCreateTER)); + env.close(); + + uint256 const buyNegIouOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID1, gwXAU(-1)), + token::owner(issuer), + ter(offerCreateTER)); + env.close(); + + { + // Now try to accept the offers. + // 1. If fixNFTokenNegOffer is NOT enabled get tecINTERNAL. + // 2. If fixNFTokenNegOffer IS enabled get tecOBJECT_NOT_FOUND. + TER const offerAcceptTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tecOBJECT_NOT_FOUND) + : static_cast(tecINTERNAL); + + // Sell offers. + env(token::acceptSellOffer(buyer, sellNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::acceptSellOffer(buyer, sellNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + + // Buy offers. + env(token::acceptBuyOffer(issuer, buyNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::acceptBuyOffer(issuer, buyNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + } + { + // 1. If fixNFTokenNegOffer is NOT enabled get tecSUCCESS. + // 2. If fixNFTokenNegOffer IS enabled get tecOBJECT_NOT_FOUND. + TER const offerAcceptTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tecOBJECT_NOT_FOUND) + : static_cast(tesSUCCESS); + + // Brokered offers. + env(token::brokerOffers( + gw, buyNegXrpOfferIndex, sellNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::brokerOffers( + gw, buyNegIouOfferIndex, sellNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + } + } + + // Test what happens if NFTokenOffers are created with negative amounts + // and then fixNFTokenNegOffer goes live. What does an acceptOffer do? + { + Env env{*this, features - fixNFTokenNegOffer}; + + env.fund(XRP(1000000), issuer, buyer, gw); + env.close(); + + env(trust(issuer, gwXAU(2000))); + env(trust(buyer, gwXAU(2000))); + env.close(); + + env(pay(gw, issuer, gwXAU(1000))); + env(pay(gw, buyer, gwXAU(1000))); + env.close(); + + // Create an NFT that we'll make XRP offers for. + uint256 const nftID0{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + // Create an NFT that we'll make IOU offers for. + uint256 const nftID1{ + token::getNextID(env, issuer, 1u, tfTransferable)}; + env(token::mint(issuer, 1), txflags(tfTransferable)); + env.close(); + + // Make offers with negative amounts for the NFTs + uint256 const sellNegXrpOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID0, XRP(-2)), + txflags(tfSellNFToken)); + env.close(); + + uint256 const sellNegIouOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID1, gwXAU(-2)), + txflags(tfSellNFToken)); + env.close(); + + uint256 const buyNegXrpOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID0, XRP(-1)), + token::owner(issuer)); + env.close(); + + uint256 const buyNegIouOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID1, gwXAU(-1)), + token::owner(issuer)); + env.close(); + + // Now the amendment passes. + env.enableFeature(fixNFTokenNegOffer); + env.close(); + + // All attempts to accept the offers with negative amounts + // should fail with temBAD_OFFER. + env(token::acceptSellOffer(buyer, sellNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::acceptSellOffer(buyer, sellNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + + // Buy offers. + env(token::acceptBuyOffer(issuer, buyNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::acceptBuyOffer(issuer, buyNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + + // Brokered offers. + env(token::brokerOffers( + gw, buyNegXrpOfferIndex, sellNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::brokerOffers( + gw, buyNegIouOfferIndex, sellNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + } + + // Test buy offers with a destination with and without + // fixNFTokenNegOffer. + for (auto const& tweakedFeatures : + {features - fixNFTokenNegOffer, features | fixNFTokenNegOffer}) + { + Env env{*this, tweakedFeatures}; + + env.fund(XRP(1000000), issuer, buyer); + + // Create an NFT that we'll make offers for. + uint256 const nftID{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tesSUCCESS) + : static_cast(temMALFORMED); + + env(token::createOffer(buyer, nftID, drops(1)), + token::owner(issuer), + token::destination(issuer), + ter(offerCreateTER)); + env.close(); + } + } + void testWithFeats(FeatureBitset features) { @@ -4584,6 +4895,7 @@ class NFToken_test : public beast::unit_test::suite testNFTokenWithTickets(features); testNFTokenDeleteAccount(features); testNftXxxOffers(features); + testFixNFTokenNegOffer(features); } public: