Skip to content

Commit

Permalink
[Amendment] If you set a destination on an NFT offer, only that desti…
Browse files Browse the repository at this point in the history 
…nation can settle through brokerage (fix #4373) (#4399)

Without this amendment, for NFTs using broker mode, if the sell offer contains a destination and that destination is the buyer account, anyone can broker the transaction. Also, if a buy offer contains a destination and that destination is the seller account, anyone can broker the transaction. This is not ideal and is misleading.

Instead, with this amendment: If you set a destination, that destination needs to be the account settling the transaction. So, the broker must be the destination if they want to settle. If the buyer is the destination, then the buyer must accept the sell offer, as you cannot broker your own offers.

If users want their offers open to the public, then they should not set a destination. On the other hand, if users want to limit who can settle the offers, then they would set a destination.

Unit tests:

1. The broker cannot broker a destination offer to the buyer and the buyer must accept the sell offer. (0 transfer)
2. If the broker is the destination, the broker will take the difference. (broker mode)
  • Loading branch information
@dangell7
dangell7 authored Feb 13, 2023
1 parent 40c55e9 commit 4ea7d56
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 48 deletions.
36 changes: 28 additions & 8 deletions src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,20 +118,40 @@ 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 the buyer specified a destination
if (auto const dest = bo->at(~sfDestination))
{
if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount])
return tecNFTOKEN_BUY_SELL_MISMATCH;
// fixUnburnableNFToken
if (ctx.view.rules().enabled(fixUnburnableNFToken))
{
// the destination may only be the account brokering the offer
if (*dest != ctx.tx[sfAccount])
return tecNO_PERMISSION;
}
else
{
// the destination must be the seller or the broker.
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 the seller specified a destination
if (auto const dest = so->at(~sfDestination))
{
if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
return tecNFTOKEN_BUY_SELL_MISMATCH;
// fixUnburnableNFToken
if (ctx.view.rules().enabled(fixUnburnableNFToken))
{
// the destination may only be the account brokering the offer
if (*dest != ctx.tx[sfAccount])
return tecNO_PERMISSION;
}
else
{
// the destination must be the buyer or the broker.
if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
return tecNFTOKEN_BUY_SELL_MISMATCH;
}
}

// The broker can specify an amount that represents their cut; if they
Expand Down
113 changes: 73 additions & 40 deletions src/test/app/NFToken_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2872,15 +2872,20 @@ class NFToken_test : public beast::unit_test::suite
BEAST_EXPECT(ownerCount(env, minter) == 2);
BEAST_EXPECT(ownerCount(env, buyer) == 1);

// issuer cannot broker the offers, because they are not the
// Destination.
env(token::brokerOffers(
issuer, offerBuyerToMinter, 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);
{
// issuer cannot broker the offers, because they are not the
// Destination.
TER const expectTer = features[fixUnburnableNFToken]
? tecNO_PERMISSION
: tecNFTOKEN_BUY_SELL_MISMATCH;
env(token::brokerOffers(
issuer, offerBuyerToMinter, offerMinterToBroker),
ter(expectTer));
env.close();
BEAST_EXPECT(ownerCount(env, issuer) == 0);
BEAST_EXPECT(ownerCount(env, minter) == 2);
BEAST_EXPECT(ownerCount(env, buyer) == 1);
}

// Since broker is the sell offer's destination, they can broker
// the two offers.
Expand Down Expand Up @@ -2917,29 +2922,52 @@ class NFToken_test : public beast::unit_test::suite
BEAST_EXPECT(ownerCount(env, minter) == 1);
BEAST_EXPECT(ownerCount(env, buyer) == 2);

// Cannot broker offers when the sell destination is not the buyer.
env(token::brokerOffers(
broker, offerIssuerToBuyer, offerBuyerToMinter),
ter(tecNFTOKEN_BUY_SELL_MISMATCH));
env.close();
BEAST_EXPECT(ownerCount(env, issuer) == 1);
BEAST_EXPECT(ownerCount(env, minter) == 1);
BEAST_EXPECT(ownerCount(env, buyer) == 2);
{
// Cannot broker offers when the sell destination is not the
// buyer.
TER const expectTer = features[fixUnburnableNFToken]
? tecNO_PERMISSION
: tecNFTOKEN_BUY_SELL_MISMATCH;
env(token::brokerOffers(
broker, offerIssuerToBuyer, offerBuyerToMinter),
ter(expectTer));
env.close();

// Broker is successful when destination is buyer.
env(token::brokerOffers(
broker, offerMinterToBuyer, offerBuyerToMinter));
env.close();
BEAST_EXPECT(ownerCount(env, issuer) == 1);
BEAST_EXPECT(ownerCount(env, minter) == 1);
BEAST_EXPECT(ownerCount(env, buyer) == 0);
BEAST_EXPECT(ownerCount(env, issuer) == 1);
BEAST_EXPECT(ownerCount(env, minter) == 1);
BEAST_EXPECT(ownerCount(env, buyer) == 2);

// 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);
// amendment switch: When enabled the broker fails, when
// disabled the broker succeeds if the destination is the buyer.
TER const eexpectTer = features[fixUnburnableNFToken]
? tecNO_PERMISSION
: TER(tesSUCCESS);
env(token::brokerOffers(
broker, offerMinterToBuyer, offerBuyerToMinter),
ter(eexpectTer));
env.close();

if (features[fixUnburnableNFToken])
// Buyer is successful with acceptOffer.
env(token::acceptBuyOffer(buyer, offerMinterToBuyer));
env.close();

// Clean out the unconsumed offer.
env(token::cancelOffer(buyer, {offerBuyerToMinter}));
env.close();

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);
return;
}
}

// Show that if a buy and a sell offer both have the same destination,
Expand All @@ -2957,15 +2985,20 @@ class NFToken_test : public beast::unit_test::suite
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);
{
// Cannot broker offers when the sell destination is not the
// buyer or the broker.
TER const expectTer = features[fixUnburnableNFToken]
? tecNO_PERMISSION
: tecNFTOKEN_BUY_SELL_MISMATCH;
env(token::brokerOffers(
issuer, offerBuyerToBroker, offerMinterToBroker),
ter(expectTer));
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(
Expand Down Expand Up @@ -6047,4 +6080,4 @@ class NFToken_test : public beast::unit_test::suite

BEAST_DEFINE_TESTSUITE_PRIO(NFToken, tx, ripple, 2);

} // namespace ripple
} // namespace ripple

0 comments on commit 4ea7d56

Please sign in to comment.