rippled: [Bug] Buy/Sell NFTokenOffers with Destination can be brokered by accounts not set as Destination
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
Destinationfrom the account mentioned in theDestinationfield of the Sell-Offer. - Buy-Offer has a Destination account set and there is a Sell-Offer without
Destinationfrom the account mentioned in theDestinationfield 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.
About this issue
- Original URL
- State: closed
- Created a year ago
- Reactions: 1
- Comments: 22 (3 by maintainers)
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!
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 🙏❤️
Sorry I think my words were not clear:
by the current implementation, the broker mode is possible if:
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.
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.
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
tfTransferand if this flag is set, theDestinationbecomes a required field, and theAmountcould be forced to0.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.
https://github.com/XRPLF/rippled/blob/fda9e9a7eedf1ab8e9bd2a9aa1ad97d6c69eafd1/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp#L115
https://github.com/XRPLF/rippled/blob/fda9e9a7eedf1ab8e9bd2a9aa1ad97d6c69eafd1/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp#L123
This hurt multiple users in the last week. Thanks for investigating and the write up.
I think you misunderstand the goal of the fix. The xls20 specification clearly states in regards of the
Destinationfield: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.
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.
https://github.com/XRPLF/rippled/pull/4399 @intelliot I believe this PR is the one that fixes this ticket
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:
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.
@ledhed2222 @shawnxie999 - could you look at this?