rippled: [Bug] Brokering 2 offers from the same account when he owns the NFT

I recently found out that one can broker 2 offers from the same account. This happens in the following scenario:

Account A owns NFT B
Account A has a Sell Offer for NFT B of 100 XRP
Account A has a Buy Offer for the same NFT B of 120 XRP
The two Offers can be brokered by a 3rd party with a max broker fee of 20 XRP.
NO NFT is transferred but the offers are brokered and the BrokerFee is sent to the broker account.

This already happend in the transaction: 9197F983793170453C99C0C03854343ABA3483E4207AC7CEBAF33B152B45D369

The account rG6K73xHYxY36QxB6Yo1KB9ncNfJ2sXJKr owns/owned the NFT 000903E8A93FF1BCFBACF203EEF99F9F98D3BA041506618D2DCBAB9D00000002.

The mentioned account has a sell offer for this NFT: 6BE7C90C3F406360F49B2775D3F39028BE9CC5A1C1228715EE0C0A0BDDD20E95 looking like this:

"DeletedNode": {
                    "FinalFields": {
                        "Amount": "1000000",
                        "Flags": 1,
                        "NFTokenID": "000903E8A93FF1BCFBACF203EEF99F9F98D3BA041506618D2DCBAB9D00000002",
                        "NFTokenOfferNode": "0",
                        "Owner": "rG6K73xHYxY36QxB6Yo1KB9ncNfJ2sXJKr",
                        "OwnerNode": "0",
                        "PreviousTxnID": "25A7D444EB3C1F5F1080DAF3020499D45E19EFD54495641B89E1E9EB0E9F55B2",
                        "PreviousTxnLgrSeq": 75783967
                    },
                    "LedgerEntryType": "NFTokenOffer",
                    "LedgerIndex": "6BE7C90C3F406360F49B2775D3F39028BE9CC5A1C1228715EE0C0A0BDDD20E95"
                }

and a buy offer for the same NFT: F7D75FB1BDD1420EC3CE89104AD058155A7E84B11DF185155577E4AA0384775C looking like this:

"DeletedNode": {
                    "FinalFields": {
                        "Amount": "1020000",
                        "Flags": 0,
                        "NFTokenID": "000903E8A93FF1BCFBACF203EEF99F9F98D3BA041506618D2DCBAB9D00000002",
                        "NFTokenOfferNode": "0",
                        "Owner": "rG6K73xHYxY36QxB6Yo1KB9ncNfJ2sXJKr",
                        "OwnerNode": "0",
                        "PreviousTxnID": "9E05724965376EDF2D56DF53CFD556595A90C7CEB78734641DF551B82382E536",
                        "PreviousTxnLgrSeq": 75768325
                    },
                    "LedgerEntryType": "NFTokenOffer",
                    "LedgerIndex": "F7D75FB1BDD1420EC3CE89104AD058155A7E84B11DF185155577E4AA0384775C"
                }

One account (rBkFerpC65D7uuWAhFkuQFdLF6FVYaoBot) was able to broker the sell and the buy offer which are from the same Account for the same NFT he owns. Looking at the meta data, no NFT was transferred:

Brokered NFTokenAcceptOffer:

9197F983793170453C99C0C03854343ABA3483E4207AC7CEBAF33B152B45D369

This transaction should not be possible in my opinion.

About this issue

Original URL
State: closed
Created a year ago
Comments: 27 (3 by maintainers)

Commits related to this issue

Prevent brokered sale of NFToken to owner: Fixes #4374 — committed to scottschurr/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: Fixes #4374 — committed to scottschurr/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: Fixes #4374 — committed to scottschurr/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: Fixes #4374 — committed to scottschurr/rippled by scottschurr a year ago
[Amendment] Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering e... — committed to XRPLF/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money ... — committed to XRPLF/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money ... — committed to XRPLF/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money ... — committed to XRPLF/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money ... — committed to XRPLF/rippled by scottschurr a year ago
Prevent brokered sale of NFToken to owner: (#4403) Fixes #4374 It was possible for a broker to combine a sell and a buy offer from an account that already owns an NFT. Such brokering extracts money ... — committed to Transia-RnD/rippled by scottschurr a year ago

Most upvoted comments

You bring up good points. We’re trying to think through the best way to solve this. IMO the way forward is preventing users from getting into this state in the first place, as I mentioned above, not simply band-aiding it with stopping the brokered mode execution. That’s the symptom, not the problem.

@sschurr and I are thinking through some options

On Fri, Jan 27, 2023 at 13:36 Ed Hennis @.***> wrote:

The average user is non technical. They are not geeks like we are. They have no idea what can happen when they accidentally forget to remove their buy offer.

The XRPL itself is highly technical. Non technical users interact with it through clients, which are expected to do most of that checking and handholding. For example, XUMM will make the user double-check the DestinationTag before sending a transaction. There are plenty of ways a user directly interacting with the ledger can shoot themselves in the foot: blackholeing their active wallet, sending a payment to the wrong account, reversing TakerPays and TakerGets in an OfferCreate, etc.

However, that said:

The transaction fails with a tec-class code if: The buyer already owns the NFToken.

Also here: the Buyer is already the Owner. So this transaction should never have succeeded in the first place.

The docs are sometimes wrong, so I went back to the spec:

The NFTokenOffer against which NFTokenOfferAccept transaction is placed is an offer to buy the NFToken and the account executing the NFTokenOfferAccept is not, at the time of execution, the current owner of the corresponding NFToken.

Sure enough, even though this condition is under direct mode, I think the spec implies that the conditions of direct mode also apply to brokered mode, because several other of the failure conditions in direct mode don’t make sense if they’re not checked in brokered mode. (To catch this in direct mode, it looks like the code checks that the account can not accept its own offer, which effectively checks that the account doesn’t own the NFT, but means the check is missed in brokered mode.)

So if the implementation disagrees with the spec, then I now agree that this is a bug.

— Reply to this email directly, view it on GitHub https://github.com/XRPLF/rippled/issues/4374#issuecomment-1406927860, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXUFDL7OAQWKXVYH7WQTVDWUQIQ7ANCNFSM6AAAAAATO5CWWQ . You are receiving this because you were mentioned.Message ID: @.***>

ledhed2222 on Jan 27, 2023

@xVet I don’t understand the question. I believe the check is implemented in #4403. It’s not particularly troublesome, but there are other NFT-related fixes pending (namely https://github.com/XRPLF/rippled/pull/4380) and the current plan is to bundle them all into a single amendment for simplicity; so, this check is blocked until those other fixes are reviewed and approved.

intelliot on Feb 6, 2023

@Cadey, yes, https://github.com/XRPLF/rippled/pull/4403 prevents a broker from selling an NFToken from the token’s owner back to the same owner. The solution was originally suggested by @nixer89.

scottschurr on Feb 4, 2023

@shawnxie999 regarding…

When deleting sell offers, don’t we still have to iterate through every entry in the sell offer directory in order to delete them from the owner directory? Isn’t that still going to be expensive?

Yes, that’s right. We’re still adding to the overhead whenever an NFToken changes hands. That’s not great.

What I’m saying, however, is there’s a clear benefit associated with the cost. For the added cost we get to remove every sell offer associated with the NFToken that changed hands. All of these sell offers are removed from the ledger as well as the entire sell offer directory. That’s a clear reduction of load on the ledger. So we’re getting a clear benefit for the compute price that we pay.

I’m not trying to say that we should obviously remove all sell offers whenever an NFToken changes hands. It’s not obvious it’s a good choice. But it’s a considerably better choice than selectively canceling buy offers, at least in terms of compute costs.

scottschurr on Feb 3, 2023

@Cadey, I will not disagree with your correctness argument. But it’s important to balance the correctness argument with compute efficiency. Take a look at this report:

https://dev.to/ripplexdev/ensuring-stable-performant-nfts-on-the-xrp-ledger-mkm

You’ll see that NFT minting and trading already have a significantly higher compute cost than XRP payments. We need to be cautious about increasing that compute load associated with NFTokens yet further.

Clearing the sell offers looks like it would have a pretty good cost to benefit ratio, because all of the sell offers for that NFT could be unconditionally removed. So the compute cost leads to a clear benefit on the ledger.

Clearing buy offers from only the new owner has a much worse cost to benefit ratio. There will typically be more buy offers than sell offers. The entire buy directory for that NFToken must be walked and each offer examined to see if the buy offer belongs to the new NFToken owner. Only if the buy offer is owned by the new NFToken owner should the offer be automatically removed. More compute time and less ledger benefit.

scottschurr on Feb 3, 2023

So for this issue, I think it’s perfectly correct for the ledger to cancel all buy offers for an nft that an account ends up owning.

When the Nft is updated to its new account, the ledger should cancel all open buy offers for that nft in then new account. It helps to clean up the ledger. The same should go for sell offers from the prevoiuse owner, any remaining sells should be cleared in the same way.

Cadey on Feb 3, 2023

The average user is non technical. They are not geeks like we are. They have no idea what can happen when they accidentally forget to remove their buy offer.

The XRPL itself is highly technical. Non technical users interact with it through clients, which are expected to do most of that checking and handholding. For example, XUMM will make the user double-check the DestinationTag before sending a transaction. There are plenty of ways a user directly interacting with the ledger can shoot themselves in the foot: blackholeing their active wallet, sending a payment to the wrong account, reversing TakerPays and TakerGets in an OfferCreate, etc.

However, that said:

The transaction fails with a [tec-class code](https://xrpl.org/tec-codes.html) if:
The buyer already owns the NFToken.
Also here: the Buyer is already the Owner. So this transaction should never have succeeded in the first place.

The docs are sometimes wrong, so I went back to the spec:

The NFTokenOffer against which NFTokenOfferAccept transaction is placed is an offer to buy the NFToken and the account executing the NFTokenOfferAccept is not, at the time of execution, the current owner of the corresponding NFToken.

Sure enough, even though this condition is under direct mode, I think the spec implies that the conditions of direct mode also apply to brokered mode, because several other of the failure conditions in direct mode don’t make sense if they’re not checked in brokered mode. (To catch this in direct mode, it looks like the code checks that the account can not accept its own offer, which effectively checks that the account doesn’t own the NFT, but means the check is missed in brokered mode.)

So if the implementation disagrees with the spec, then I now agree that this is a bug.

ximinez on Jan 27, 2023

We have just checked how many times this happened until now: 8 times!

9197F983793170453C99C0C03854343ABA3483E4207AC7CEBAF33B152B45D369
027135E9EE998981D6170836FF0DCE534B003F8B6C9BDD3909E3B7A5E43B983C
FF08F31C725B16D86DEA7080CFCADCC0624F678F5A9A11AE27480EFD5D41C877
2B4B3849A01CE121300642B0A2A16468E4297A938B738B493437A07B07CDF032
48C0B888B6F19E958709F7A1BA68D46225A971560E6201DE04EE91A8387CC906
EA0A0894439D4C7EDB039495F1EB0CC7C3E331C06AE2463A5C78BBC4B8C5AA05
E70A15701E4EC8F322F7BA90B9407A009E3F3D13B8A39CCB4D1D40F1DDD4A046
2B891A1F7BA6B69999F504B0088E23F5E011C3CADBC9967BBFA38AD2950558FB

The bad thing: Even Marketplaces broker those offers!

This is a very bad user experience.

Some marketplaces have decided to hide offers where the MP broker account is NOT set as the Destination.

That is because they want the users to create offers through their MP to set the MP’s Broker Account as Destination and receive the Broker Fee.

So the User doesn’t even have a chance to see his old Buy offer(s) which still exists on the XRPL when he tries to sell the NFT on a Marketplace.

Yes, we could now blame the Marketplaces and tell them that they should show warnings etc. But this is just a Layer 2 solution. And not really a good one.

So solving this issue on Layer1 would be much more beneficial.

nixer89 on Jan 27, 2023

The new owner might have a stale buy offer for their NFToken sitting out in the Ledger. Then, before canceling that buy offer, they could create a new sell offer. We’ve returned to having both legs of the loop and the exploit becomes possible again.

IMHO, the user has to take some responsibility here. If they can’t be bothered to check their own outstanding offers, we don’t necessarily need to stop them from shooting themselves in the foot.

Let’s decide who the XRPL is for. For the people who develop it or for end users. If it’s for the first, then I suggest we just stop right here. And look for other things to do.

alloynetworks on Jan 27, 2023

I don’t think such cases are common, as 1) victim should have created a few Buy offers 2) the previously created Buy offer should be for a larger amount than the amount of a new Sell offer. In most cases users try to sell for more than what they try to buy them for.

If the seller and buyer are the same account, means no NFT will be transferred - would be nice to get an Error, so such transaction wouldn’t get through.

ihomp on Jan 26, 2023

IMHO, the user has to take some responsibility here. If they can’t be bothered to check their own outstanding offers, we don’t necessarily need to stop them from shooting themselves in the foot.

If we think like this, then blockchain mass adoption will never happen. They (the users) will use the XRPL one time, get their funds removed by a broker when accidentally having a sell and buy offer for the very same NFT they own, and then leave the XRPL as fast as they can.

The average user is non technical. They are not geeks like we are. They have no idea what can happen when they accidentally forget to remove their buy offer.

We should put user protection above any broker which could make a quick buck.

Brokering 2 offers where the NFT owner is also the Sell Offer Owner AND the Buy Offer Owner makes absolutely no sense.

Ok. So: sorry for that rant but it had to be said 😃

So going on with more technical details now:

The documentation states for an NFTokenAcceptOffer: https://xrpl.org/nftokenacceptoffer.html#execution-details

If the transaction succeeds:
The NFTtoken changes ownership, meaning that the token is removed from the NFTokenPage of the existing owner and added to the NFTokenPage of the new owner.

This is clearly not the case. The NFToken has not changed ownership. Even the meta data does not show any NFTokenPage changes.

The transaction fails with a [tec-class code](https://xrpl.org/tec-codes.html) if:
The buyer already owns the NFToken.

Also here: the Buyer is already the Owner. So this transaction should never have succeeded in the first place.

I still stand by my point: this is a bug which has to be fixed!

nixer89 on Jan 26, 2023

The new owner might have a stale buy offer for their NFToken sitting out in the Ledger. Then, before canceling that buy offer, they could create a new sell offer. We’ve returned to having both legs of the loop and the exploit becomes possible again.

IMHO, the user has to take some responsibility here. If they can’t be bothered to check their own outstanding offers, we don’t necessarily need to stop them from shooting themselves in the foot.

ximinez on Jan 26, 2023

I had previously discussed this with @nbougalis and others. the issue here is more that these “orphaned” offers can stick around. You’ll note that to actually create the scenario you describe, there are a few more steps:

Alice mints NFT
Bob creates a buy offer for it for 5 XRP
Alice decides to gift the NFT to Bob for 0, creating a sell offer (hopefully using a destination too)
Bob accepts the sell offer, because it is better than paying 5 XRP
!!! At this point, Bob has the NFT and still has their buy offer from when they did not have the NFT !!! This is because the order book is not cleared when an NFT changes hands.
Now that Bob owns the NFT, he cannot create new buy offers. However he still has one left over from when he did not own it. He can create new sell offers. If he does, resulting in both a buy and sell offer for the same NFT, yes those two can be brokered.

I don’t think that preventing the brokering of those offers is the solution. I’m not even sure that it’s a bad thing that they can be brokered, given that the state can happen, because the broker is providing a service to the ledger of cleaning up junk offers.

If anything, we would want to try and avoid the scenario in the first place, which would involve auto-cancelling any buy-side offers owned by an account for a given NFT whenever that account takes ownership of said NFT.

@scottschurr what do you think about that idea?

ledhed2222 on Jan 25, 2023

There can’t be fake buy or sell walls. It’s not an automatic matching order book.

And fake offers are already possible today by creating unfunded buy or sell offers with 3rd party accounts. Which is much harder to detect for the average user than buy and sell fake offers from the account owner itself.

nixer89 on Jan 2, 2023

the sell offer as well as the buy offer are from the same account that ALSO owns the NFT. there is nothing to settle, the NFT stays in the same account. this should not be possible.

it is as if you offer your car for 10k and want the same car for 12k. then someone comes, buys your car for 10k and sells it to you for 12k and makes a 2k profit. doesn’t make sense? yes it doesn’t. so doesn`t make the above mention transaction sense and should be prohibited for users safety of funds.

once can easily forget existing old offers.

nixer89 on Jan 2, 2023