[Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination #4373

nixer89 · 2023-01-02T14:53:35Z

Description

When trading NFTs, one can set a "Destination" for his NFTokenOffer. This has the purpose that only the Destination account can accept this NFTokenOffer. It allows direkt NFT transfers or brokered mode for NFT trades.

However, there seems to be a bug with the brokered mode for NFTokenOffers when:

Sell-Offer has a Destination account set and there is a Buy-Offer without Destination from the account mentioned in the Destination field of the Sell-Offer.
Buy-Offer has a Destination account set and there is a Sell-Offer without Destination from the account mentioned in the Destination field of the Buy-Offer.

Here is an example:
Transaction:
E67A0EA8861E597CF2248C449002FC62E55AAF6A375F2B3E616ADD20A32AFAF7
is a brokered NFTokenAcceptOffer.

It brokers the Sell offer created in the transaction:
948C4F6BDC8099200252BB24EDE62B8638617635A2331991DC9640812A8D9C26

which has the account r4CizHZZYpDgrRbWavMiNecuyRdbTLTPh set as destination. And 0 XRP as amount:

"DeletedNode": {
                    "FinalFields": {
                        "Amount": "0",
                        "Destination": "r4CizHZZYpDgrRbWavMiNecuyRdbTLTPh",
                        "Flags": 1,
                        "NFTokenID": "0008138873C516A5EF9A8E860889E355EC9B93EEAECFEC7B46C9F20F0000026A",
                        "NFTokenOfferNode": "0",
                        "Owner": "rBZ3FrRzTgy46H3C9Uede8CAuH7rTGA5iE",
                        "OwnerNode": "18",
                        "PreviousTxnID": "948C4F6BDC8099200252BB24EDE62B8638617635A2331991DC9640812A8D9C26",
                        "PreviousTxnLgrSeq": 76810377
                    },
                    "LedgerEntryType": "NFTokenOffer",
                    "LedgerIndex": "314B0A42F7B13E9791DBB8C68982334A1E11DD42B59A3A36C28F76F6FDC46998"
                }

and the buy offer from account r4CizHZZYpDgrRbWavMiNecuyRdbTLTPh created in transaction:
88ED6197673596F6FF8E7A1BD415B53AD945232B8D1CA87D274A3BF6A4E60429

for the same NFT with an amount of 60 XRP:

"DeletedNode": {
                    "FinalFields": {
                        "Amount": "60000000",
                        "Flags": 0,
                        "NFTokenID": "0008138873C516A5EF9A8E860889E355EC9B93EEAECFEC7B46C9F20F0000026A",
                        "NFTokenOfferNode": "0",
                        "Owner": "r4CizHZZYpDgrRbWavMiNecuyRdbTLTPh",
                        "OwnerNode": "0",
                        "PreviousTxnID": "88ED6197673596F6FF8E7A1BD415B53AD945232B8D1CA87D274A3BF6A4E60429",
                        "PreviousTxnLgrSeq": 76623180
                    },
                    "LedgerEntryType": "NFTokenOffer",
                    "LedgerIndex": "64ECA592F1204FF6C6E7ACA8A95ACA8EC6354B8CAB974A59DCD1B35E238EA696"
                }

These two offers got brokered by Account rBkFerpC65D7uuWAhFkuQFdLF6FVYaoBot which is NOT the Destination account of the Sell offer.

In my opinion this brokered transactions should not be possible since the Destination field defines which account can accept the offer.

The Destination field for the Sell offer was set to r4CizHZZYpDgrRbWavMiNecuyRdbTLTPh but still rBkFerpC65D7uuWAhFkuQFdLF6FVYaoBot was able to broker/accept it. The Documentation clearly states that, if a Destination field is set, only the Destination account can accept this offer.

The text was updated successfully, but these errors were encountered:

WietseWind · 2023-01-02T15:14:15Z

This hurt multiple users in the last week. Thanks for investigating and the write up.

dangell7 · 2023-01-02T17:57:57Z

rippled/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp

Line 115 in fda9e9a

// If the buyer specified a destination, that destination must be

rippled/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp

Line 123 in fda9e9a

// If the seller specified a destination, that destination must be

intelliot · 2023-01-19T00:49:06Z

@ledhed2222 @shawnxie999 - could you look at this?

ledhed2222 · 2023-01-19T20:59:01Z

@nixer89 @WietseWind - could you please explain the workflow that caused users to be hurt by this? I'm understanding the scenario and seeing where this happens, but would like to understand the full context please. Sounds like it involves some degree of social engineering

nixer89 · 2023-01-19T21:39:45Z

I am not sure how the offers were set up the way they were. I don't own or know any of the involved accounts.

But a simple setup could be:

issuer mints NFT
Buyer sets order to buy for 60 XRP
Issuer decides for whatever reason to give the NFT out for free and creates a 0 XRP sell offer with the buyers account as Destination.

Now anyone can broker this trade/transfer.

I don't think it involves social engineering.

Maybe just inattentive users / issuers or market places not showing all offers (eg some only show offers where the MP broker account is set as destination)

ledhed2222 · 2023-01-19T22:11:21Z

yeah ok thanks @nixer89. the thing is that the broker in this case isn't able to get the NFT to go to the incorrect destination. You'll note that the comment and condition noted above in this thread explicitly allows this behavior: that is, a broker can broker offers that have a destination if that destination is either:

themselves (IE the destination is the broker), or
the other party

In this case, the one you have above, the worst thing that can happen is the yes, the broker is going to take 60 XRP from the buyer. but the buyer's offer should have been cancelled prior to them getting an offer to receive the NFT for free. the seller's wish of getting the NFT to the buyer for 0 XRP is respected, and the buyer's wish of purchasing the NFT for 60 XRP is also respected.

note, my personal recommendation for the use of brokered mode has always been for platforms to ensure that both buyside and sellside offers indicate the broker as the destination, where the broker is the platform itself. but if offers are happening off platform, with unwise parameters and/or not checking the existing order book and cleaning up offers you no longer wish to make, this kind of thing is going to happen.

nixer89 · 2023-01-22T18:40:38Z

I think , if it's working as intended, it's a bad design choice of the XRPL.

The only means of transferring an NFT is to use a sell offer with a destination and amount = 0 XRP.

So a user could have two accounts. Both with buy offers for an NFT with different amounts.

Then one of the offers gets accepted. Now he tries to transfer from one account to the other (which still has the buy offer the user didn't think about).

Now his transfer gets brokered and he looses the XRP he offered for that NFT with his other account.

Imho brokers should only be able to accept offers where:

Both offers have NO destination set
Both offers have the broker account as Destination set.

This increases user protection and doesn't harm "real" brokers. Only the ones using this "flaw".

I mean, you said yourself, that brokered transactions should both have the Destination field set.

Cadey · 2023-02-03T06:48:13Z

I have to agree this is working as designed. The broker mode is actually doing its job perfectly.

A brokers role is to connect buyers and sellers together and they make money by joining the widest trade gaps.

The actual problem people are trying to fix here is the inability to directly request an nft be transfered. A new method on the ledger which isnt a sell offer but still has to be accepted (to stop spaming a wallet full of junk nfts) would be ideal. It also wouldn't impact trading averages if your calculating a sold average, today you have to ignore the zeros.

On the issue of the offers having a destination set, I still think the broker is working correctly. Its an easy odd scenario I grant you but it still valid. Alice want to give it to Bob, but Bob offers to buy it from Alice with out realising he could get it for free. Along comes Mr broker, see the gap and makes the connection and took the difference.

There are better solutions to this problem than stopping a broker sale in my opinion. For example matching offers when placed. If Bob was willing to buy the Nft first for 60 and Alice placed a sell for anything less than 60 it could automatically be sold for 60. Likewise is Alice put a sell in for zero and Bob put a buy for anything more than zero he gets it automatically for zero (destination rules still apply on matches) . There are many more options I'm sure :)

Side note on the issue....
Some people might argue only the "best" two offers should be linked during a brokered sale but that's not practical. For example what if there are two open offers with different tokens, one is xrp, one is xxcoin. Both are set at 100. Which is better... Is xxcoin worth more or less than an xrp. You can see how this is simply not going to work.

xVet · 2023-02-04T20:41:34Z

I disagree, a new transactor is not necessary imo.

The best solution is to see where this is usually a problem and that's for NFT transfers.

Good solution for this is what nixer proposed elsewhere :

A new flag tfTransfer and if this flag is set, the Destination becomes a required field, and the Amount could be forced to 0.

Offers with tfTransfer could lock out brokers to broker this.

Transfers of NFTs would be much easier to handle and safer at the same time. Without limiting the specifications of the broker mode.

intelliot · 2023-02-07T18:34:45Z

~~Correct me if I'm wrong, but I believe this issue will be fixed by #4403~~

shawnxie999 · 2023-02-07T18:53:23Z

Correct me if I'm wrong, but I believe this issue will be fixed by #4403

@intelliot Isn't Scott's PR just to check if the seller and buyer are the same person? Meanwhile this ticket intends to restrict the Destination field of an offer to only the broker's account

intelliot · 2023-02-07T19:03:43Z

@shawnxie999 Is a new PR needed for that then?

shawnxie999 · 2023-02-07T19:12:36Z

@shawnxie999 Is a new PR needed for that then?

#4399
@intelliot I believe this PR is the one that fixes this ticket

xVet · 2023-02-07T21:06:25Z

Correct #4399 is a fix, but this fix is excluding brokers to broker transactions if they are not set in the destination .

Brokers should continue to be able to broker open offers (where they are not set as destination)

Hence my comment to introduce rather a new flag to handle NFT transfers more elegantly and safer and let the broker check if flag exists to allow or deny brokering.

pkmelee337 · 2023-02-07T21:19:01Z

I disagree, a new transactor is not necessary imo.

The best solution is to see where this is usually a problem and that's for NFT transfers.

Good solution for this is what nixer proposed elsewhere :

A new flag tfTransfer and if this flag is set, the Destination becomes a required field, and the Amount could be forced to 0.

Offers with tfTransfer could lock out brokers to broker this.

Transfers of NFTs would be much easier to handle and safer at the same time. Without limiting the specifications of the broker mode.

Agreed with @xVet here. I think it was one of the initial ideas of the brokered mode to be able to broker trades without a destination/owner set as the destination. Thereby, it's also possible to filter on that transfer flag.

ajkagy · 2023-02-07T21:25:40Z

Imho brokers should only be able to accept offers where:

Both offers have NO destination set

Both offers have the broker account as Destination set.

I disagree with this approach since it defeats the purpose of being able to broker open offers. Broker mode is essentially working as intended since the brokered offer had an outstanding buy order from the destination wallet. If anything, the user needs to be aware of open offers they have out there, but I also like the new flag approach for transfers which locks out brokers essentially.

In addition, it would essentially drastically change the way broker mode works to the point that all of the open source tools we've built and plan to build around letting users freely mint/script/interact around XLS20 offers with no destination to essentially be non-functioning with our platform.

nixer89 · 2023-02-07T21:51:00Z

I think you misunderstand the goal of the fix. The xls20 specification clearly states in regards of the Destination field:

Only allowed if the lsfSellToken flag is set. Indicates the AccountID that this sell offer is intended for (either a buyer or a broker). If present, only that account can accept the sell offer.

So it is specified that if a destination field is available in the sell offer, then ONLY the destination account should be able to accept this offer. The current implementation has clearly a bug here since the offer was accepted by an account which was not set as destination.

This has to be fixed!

If people want their offers to be brokered, then they have to leave the destination field empty or set a specific broker account as Destination. That is the intended way of the broker mode to work. Set the broker as destination and let him find the matching offers.

The broker mode was not intended to be able to accept offers where the broker account is not equal to the destination account (if a destination is set).

And I was just re reading my above comment and want to add option:

3: one of the offers has the broker account as Destination and the other offer has not destination account set

This should of course also be possible and I believe this fix doesn't disallow such a transaction.

ajkagy · 2023-02-07T22:17:17Z

I think you misunderstand the goal of the fix. The xls20 specification clearly states in regards of the Destination field:
Only allowed if the lsfSellToken flag is set. Indicates the AccountID that this sell offer is intended for (either a buyer or a broker). If present, only that account can accept the sell offer.
So it is specified that if a destination field is available in the sell offer, then ONLY the destination account should be able to accept this offer. The current implementation has clearly a bug here since the offer was accepted by an account which was not set as destination.

This has to be fixed!

If people want their offers to be brokered, then they have to leave the destination field empty or set a specific broker account as Destination. That is the intended way of the broker mode to work. Set the broker as destination and let him find the matching offers.

The broker mode was not intended to be able to accept offers where the broker account is not equal to the destination account (if a destination is set).

And I was just re reading my above comment and want to add option:

3: one of the offers has the broker account as Destination and the other offer has not destination account set

This should of course also be possible and I believe this fix doesn't disallow such a transaction.

From your wording it made it sound like option 3 wasn't a possibly given the fix proposed, which fundamentally changes broker mode and the paradigm behind an offer with an undefined destination. The whole point to an offer with an undefined destination should be that any party (Broker or P2P) should be able to accept that offer.

nixer89 · 2023-02-07T22:35:19Z

Sorry I think my words were not clear:

by the current implementation, the broker mode is possible if:

Both offers have the broker account set as destination
None of the offers have set a destination
One offer has the broker account as Destination and the other offer as NO destination set

These are the three ways broker mode works right now. And none of the amendments or PRs does change this.

Also not this fix we are talking about. It fixes a different issue where a broker account was able to accept an offer where a Destination was specified but the broker was NOT the destination. That's clearly against the specification.

xVet · 2023-02-07T22:47:38Z

I think you misunderstand the goal of the fix. The xls20 specification clearly states in regards of the Destination field:
Only allowed if the lsfSellToken flag is set. Indicates the AccountID that this sell offer is intended for (either a buyer or a broker). If present, only that account can accept the sell offer.
So it is specified that if a destination field is available in the sell offer, then ONLY the destination account should be able to accept this offer. The current implementation has clearly a bug here since the offer was accepted by an account which was not set as destination.

This has to be fixed!

If people want their offers to be brokered, then they have to leave the destination field empty or set a specific broker account as Destination. That is the intended way of the broker mode to work. Set the broker as destination and let him find the matching offers.

The broker mode was not intended to be able to accept offers where the broker account is not equal to the destination account (if a destination is set).

And I was just re reading my above comment and want to add option:

3: one of the offers has the broker account as Destination and the other offer has not destination account set

This should of course also be possible and I believe this fix doesn't disallow such a transaction.

Sorry, thanks to Nixer and Denis explaining. This fix is ideal and doesn't restrict anything.

I take my comment back, #4399 is the perfect candidate to be included in the amendment.

Thank you 🙏❤️

ajkagy · 2023-02-07T22:50:24Z

Sorry I think my words were not clear:

by the current implementation, the broker mode is possible if:

Both offers have the broker account set as destination

None of the offers have set a destination

One offer has the broker account as Destination and the other offer as NO destination set

These are the three ways broker mode works right now. And none of the amendments or PRs does change this.

Also not this fix we are talking about. It fixes a different issue where a broker account was able to accept an offer where a Destination was specified but the broker was NOT the destination. That's clearly against the specification.

got it, so this doesn't change any kind of core functionality and is just essentially fixing the bug around the dest/transfer and a broker that wasn't a dest being able to broker that edge case which shouldn't happen in the first place. Makes sense. Thanks!

…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)

intelliot · 2023-02-14T07:38:53Z

Will be fixed by #4399 and the fixNonFungibleTokensV1_2 amendment, available in 1.10.0-rc2, when activated.

intelliot assigned nbougalis, ledhed2222 and shawnxie999 Jan 19, 2023

intelliot assigned JoelKatz and aanchal4 Jan 23, 2023

This was referenced Jan 25, 2023

fixNFTokenBrokerAccept Transia-RnD/rippled#7

Closed

fixNFTokenBrokerAccept Transia-RnD/rippled#8

Closed

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Merged

nixer89 mentioned this issue Jan 26, 2023

[Bug] Brokering 2 offers from the same account when he owns the NFT #4374

Closed

intelliot added Bug High Priority Will Need Documentation labels Feb 7, 2023

intelliot added this to Core Ledger Feb 7, 2023

github-project-automation bot moved this to 🆕 New in Core Ledger Feb 7, 2023

intelliot moved this from 🆕 New to ⭐️ Highest Priority in Core Ledger Feb 7, 2023

intelliot linked a pull request Feb 7, 2023 that will close this issue

[Amendment] If you set a destination on an NFT offer, only that destination can settle through brokerage (fix #4373) #4399

Merged

7 tasks

intelliot removed the High Priority label Feb 7, 2023

intelliot added this to the 1.10 milestone Feb 9, 2023

intelliot closed this as completed Feb 14, 2023

github-project-automation bot moved this from ⭐️ Highest Priority to ✅ Merged in Core Ledger Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination #4373

[Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination #4373

nixer89 commented Jan 2, 2023 •

edited

Loading

WietseWind commented Jan 2, 2023

dangell7 commented Jan 2, 2023 •

edited

Loading

intelliot commented Jan 19, 2023

ledhed2222 commented Jan 19, 2023

nixer89 commented Jan 19, 2023

ledhed2222 commented Jan 19, 2023 •

edited

Loading

nixer89 commented Jan 22, 2023

Cadey commented Feb 3, 2023 •

edited

Loading

xVet commented Feb 4, 2023

intelliot commented Feb 7, 2023 •

edited

Loading

shawnxie999 commented Feb 7, 2023 •

edited

Loading

intelliot commented Feb 7, 2023

shawnxie999 commented Feb 7, 2023 •

edited

Loading

xVet commented Feb 7, 2023

pkmelee337 commented Feb 7, 2023

ajkagy commented Feb 7, 2023 •

edited

Loading

nixer89 commented Feb 7, 2023 •

edited

Loading

ajkagy commented Feb 7, 2023 •

edited

Loading

nixer89 commented Feb 7, 2023

xVet commented Feb 7, 2023

ajkagy commented Feb 7, 2023

intelliot commented Feb 14, 2023

[Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination #4373

[Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination #4373

Comments

nixer89 commented Jan 2, 2023 • edited Loading

Description

WietseWind commented Jan 2, 2023

dangell7 commented Jan 2, 2023 • edited Loading

intelliot commented Jan 19, 2023

ledhed2222 commented Jan 19, 2023

nixer89 commented Jan 19, 2023

ledhed2222 commented Jan 19, 2023 • edited Loading

nixer89 commented Jan 22, 2023

Cadey commented Feb 3, 2023 • edited Loading

xVet commented Feb 4, 2023

intelliot commented Feb 7, 2023 • edited Loading

shawnxie999 commented Feb 7, 2023 • edited Loading

intelliot commented Feb 7, 2023

shawnxie999 commented Feb 7, 2023 • edited Loading

xVet commented Feb 7, 2023

pkmelee337 commented Feb 7, 2023

ajkagy commented Feb 7, 2023 • edited Loading

nixer89 commented Feb 7, 2023 • edited Loading

ajkagy commented Feb 7, 2023 • edited Loading

nixer89 commented Feb 7, 2023

xVet commented Feb 7, 2023

ajkagy commented Feb 7, 2023

intelliot commented Feb 14, 2023

nixer89 commented Jan 2, 2023 •

edited

Loading

dangell7 commented Jan 2, 2023 •

edited

Loading

ledhed2222 commented Jan 19, 2023 •

edited

Loading

Cadey commented Feb 3, 2023 •

edited

Loading

intelliot commented Feb 7, 2023 •

edited

Loading

shawnxie999 commented Feb 7, 2023 •

edited

Loading

shawnxie999 commented Feb 7, 2023 •

edited

Loading

ajkagy commented Feb 7, 2023 •

edited

Loading

nixer89 commented Feb 7, 2023 •

edited

Loading

ajkagy commented Feb 7, 2023 •

edited

Loading