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

[nixer89]

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

1. Account A owns NFT B
2. Account A has a Sell Offer for NFT B of 100 XRP
3. Account A has a Buy Offer for the same NFT B of 120 XRP
4. The two Offers can be brokered by a 3rd party with a max broker fee of 20 XRP.
5. 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.

[dangell7]

So we want to reject the broker transaction if the buyOffer and sellOffer are for an NFT owned by the same account and the broker is not that account?

Who could then broker that trade? If no destination was set the trade would never settle right? Or it would only be acceptable by the account that generated the buy/sell?

[nixer89]

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.

[dangell7]

Will this will create fake buy/sell walls?

[nixer89]

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.

[intelliot]

@ledhed2222 @shawnxie999 what are your thoughts on this?

[ledhed2222]

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?

[scottschurr]

@ledhed2222, first of all, your description of how the problem occurred looks spot on to me. Nicely done.

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.

This suggestion has real usability benefits. However, when I envision the implementation it seems a little expensive. I think the implementation would probably look something like this:

1. Every time any NFT changes hands on the Ledger, then
2. The NFTokenAcceptOffer transactor must walk the entire buy-side offer directory for the NFToken that is moving.
3. For each NFTokeOffer in the buy-side directory it must fetch the offer itself from the ledger. If the offer belongs to the new owner, then we cancel the offer.
4. If the number of canceled NFTokenOffers exceeds some reasonable number, then the transaction fails with a tec (to limit execution time and to manage the size of the metadata).

That seems like a lot of work to do every time any NFToken changes hands. I think it would be very seldom that we would actually encounter any TokenOffers that would be canceled in this way. It seems unusual that any reasonable user would intentionally have more than one buy offer for a given NFToken. But, yeah, it would both reduce ledger trash and prevent this kind of problem from happening. So I wouldn't dismiss the suggestion out of hand.

I have an alternative suggestion that would also prevent this scenario. Suppose that every time an NFToken changed hands we delete all the sell offers for that NFToken.

How does that help? In order for this particular exploit to occur, the NFT owner must have both a buy and a sell offer in the ledger. If we remove any sell offers they had, then one leg of the loop has been removed the exploit cannot be used.

This approach has the advantage that we can really remove ledger trash. Here's why we would be removing all the offers unconditionally:

• Any sell offers that are held by someone other than the new owner can no longer be fulfilled. Yeah, they might be useable in the future if the NFToken changes hands again, but that is not a strong motivation for keeping them around.

• Any sell offer that is held by the new owner will be deleted to prevent this particular exploit.

It takes a really unusual circumstance for the new owner to have a pre-existing sell offer for the newly acquired token. The owner must have previously owned the NFToken, created the offer, and then not consumed the offer when the NFToken was transferred away. So this is weird.

It's not all wine and roses, however. The biggest problem I see is that people are sloppy.

• 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.

The following suggestion:

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.

Has a certain appeal to me. But it also seems like it could be a pretty expensive lesson the the NFToken holder, since they gave money to the broker and got nothing in return.

As far as I can see, the cheapest (in compute power) solution to this problem is to not allow an NFTokenAcceptOffer to complete if the source and the destination of the NFToken are the same account. That's really easy to see by examination in the NFTokenAcceptOffer transaction itself. No additional information needs to be fetched from the ledger since we already need both NFTokenOffers in hand. So the costs, in terms of ledger compute time, are quite low.

It seems like that's exactly @nixer89's suggestion. Of all the suggestions I've seen, that seems like the one to go with.

Sorry for the long ramble, Further thoughts?

[Silkjaer]

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.

That's an expensive lesson. I thought the reserves was incentive to clean up, but if that doesn't work, the reserves may be to small.

[nixer89]

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.

Sorry @ledhed2222, but I think we have to take off our developer glasses here. This is about user protection and not protecting random broker accounts. And certainly the owner reserve is there to prevent "ledger spam".

And it's a non argument that "brokers cleaning up junk offers". There are no junk offers as long as the user pays the reserve for it.

And thank you @scottschurr for the lengthy comment :-)
But I disagree in parts. Removing only sell offers is not enough. The user, due to the concept of Brokerage and how Marketplaces use it, can easily setup multiple buy offers for a single NFT. Even if unintentionally.

There are lots of cases where one user has multiple buy offers for the same NFT. Removing only sell offers won't help anyone. Once the user owns the NFT and the buy offers still exist, we will face the same problem if he decides to sell the NFT for a lower amount than his buy offer.

This happens more often than we think (that there are existing buy offers, where the NFT Owner is equal to the Buy Offer Owner). This is also partly the issuer in #4373 and how the NFToken Transfer works.

Anyway, I think the cheapest (performance wise) and easiest solution would be to disallow Broker-Mode when the Owners of the NFT, the sell and the buy offer are the same. Just as @scottschurr mentioned.

I can see that it could be expensive to remove buy and/or sell offers on the fly. And removing offers would only help if the buy offers are getting removed.

Usually users have 1 or more buy offers for a NFT and don't have any sell offers for an NFT previously to owning the NFT.

[ximinez]

• 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.

[Silkjaer]

I completely agree that users should take responsibility and clean up their NFTokenOffers. However the incentive to do so is freeing their locked reserves.

The brokerage model is way to abstract for a regular user to figure out the pitfalls of having multiple offers for the same NFToken. Other objects don't have rewards for "cleaners", so why should NFTokenOffers have that?

I agree that this is unexpected behavior and it should not be possible to broker a sales to self.

[nixer89]

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

1:

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.

2:

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!

[ihomp]

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.

[alloynetworks]

• 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.

[nixer89]

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.

[ximinez]

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.

[ledhed2222]

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](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. — Reply to this email directly, view it on GitHub <#4374 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXUFDL7OAQWKXVYH7WQTVDWUQIQ7ANCNFSM6AAAAAATO5CWWQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Cadey]

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.

[scottschurr]

@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.

[shawnxie999]

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.

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? @scottschurr

[scottschurr]

@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.

[Cadey]

@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.

So it's true there is a exec cost to cancellation but I'm trying to also balance minor chages to full voted changes. If we want to offer the same protection with out an exec cost when an nft is transfered may be the offer object itself can have a composite key of the owner and offer. If the Nft is transferred the old offers will simply be rejected based on a new comp key?

Buy this type of change would require a full amendment vote since its a larger logical change?

[scottschurr]

@Cadey, any code change that affects transaction outcomes, no matter how small, must be guarded by an amendment. That's because the hash of the outcome of every transaction accepted into the ledger must be agreed upon by more than 80% of validators on the UNL.

In that sense there are no small changes that affect transaction outcomes. They must all be guarded by amendments and voted onto the ledger.

The change being proposed to correct this problem is a small one, but it is still guarded by an amendment: #4403

[Cadey]

@Cadey, any code change that affects transaction outcomes, no matter how small, must be guarded by an amendment. That's because the hash of the outcome of every transaction accepted into the ledger must be agreed upon by more than 80% of validators on the UNL.

In that sense there are no small changes that affect transaction outcomes. They must all be guarded by amendments and voted onto the ledger.

The change being proposed to correct this problem is a small one, but it is still guarded by an amendment: #4403

Thats the change to stop a broker connecting a buy and a sell to the same address's? Different issue?,

[scottschurr]

@Cadey, yes, #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.

[xVet]

@Cadey, yes, #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.

is the implementation of this check troublesome? i feel this is pretty straightforward and little effort to gain a lot of usability out of this. Not only for users but also developers.

Without restricting really the broker mode and without intensive I/O on rippled side.

[intelliot]

@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 #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.