Add the fixEnforceNFTokenTrustline amendment: #4946

scottschurr · 2024-03-09T00:45:22Z

High Level Overview of Change

Introduces the fixEnforceNFTokenTrustline amendment which fixes bugs in interactions between NFTokenOffers and trust lines.

Resolves #4925.
Resolves #4941.

Context of Change

#4925

This issue describes a wide timing window that causes a rare bug. To understand the process you need some background in how NFTokens work.

When an NFToken is minted, it may have a transfer fee associated with it. If that transfer fee is non-zero, then a portion of the funds paid for the NFToken's transfer are syphoned off for the NFT's issuer. That transfer fee is always paid in the same funds as were used for the transfer.

The NFTokenCreateOffer transaction verifies that,

If the NFToken has a transfer fee and
The funds for the transfer are an IOU (not XRP) then
The NFToken issuer must have a trust line for the funds.

If the issuer does not have the necessary trust line then the NFTokenCreateOffer fails.

But here's the timing window. When the NFTokenCreateOffer happens no funds are transferred. The funds are transferred when the corresponding NFTokenAcceptOffer transaction completes. But NFTokenAcceptOffer does not verify the presence of the trust line.

With that background, here are the steps for the bug:

Alice creates a trust line for USD.
Alice mints an NFToken with a non-zero transfer fee.
Becky acquires the NFToken from Alice.
Becky creates an offer to sell the NFToken for USD(100).
Alice removes her trust line for USD.
Carol accepts Becky's offer for the NFToken and pays in USD.
Alice's trust line is recreated and receives the transfer fee in USD.

The bug appears in step 7, where the trust line that Alice deleted in step 5 is recreated.

The fix is simply to put a check in the NFTokenAcceptOffer transaction that is similar to the check in NFTokenCreateOffer. But the check is transaction changing, so it must be guarded by an amendment.

#4941

This issue describes an even rarer, but related bug.

The checks described in the previous issue involve looking for a trust line connected to the NFToken issuer. That almost always works. But there's one situation where it doesn't. An IOU issuer can always accept any IOU that they issue. And there is no trust line to represent that capability. In fact, having a trust line from an account to itself is forbidden.

Here are the steps for the bug:

Alice issues USD
Alice mints an NFToken with a non-zero transfer fee.
Becky acquires the NFToken from Alice.
Becky attempts to creates an offer to sell the NFToken for Alice's USD(100). This attempt fails with tecNO_LINE

The bug appears in step 4, where Becky cannot create the offer.

The fix is simply to check for the presence of the trust line or if the IOU's issuer is the same as the NFToken issuer. But the check is transaction changing, so it must be guarded by an amendment.

Type of Change

Bug fix (non-breaking change which fixes an issue)
Tests (unit tests for these changes were added)

Consideration

This NFT-related amendment arrives at about the same time as a different NFT-related amendment: #4945

Should these two pull requests be merged into a single amendment?

Proposed Commit Message

Add the fixEnforceNFTokenTrustline amendment:

Fix bugs in interactions between NFTokenOffers and trust lines.

Since the NFTokenAcceptOffer does not check the trust line that
the issuer receives as a transfer fee in the NFTokenAcceptOffer,
if the issuer deletes the trust line after NFTokenCreateOffer,
the trust line is created for the issuer by the
NFTokenAcceptOffer.  That's fixed.

Also if Alice issues an IOU and also mints NFTokens with a
transfer fee, Alice's IOU cannot be used to pay for transfers
of those NFTokens.  That's fixed.

Resolves #4925.
Resolves #4941.

Fix bugs in interactions between NFTokenOffers and trust lines. Since the NFTokenAcceptOffer does not check the trust line that the issuer receives as a transfer fee in the NFTokenAcceptOffer, if the issuer deletes the trust line after NFTokenCreateOffer, the trust line is created for the issuer by the NFTokenAcceptOffer. That's fixed. Also if Alice issues an IOU and also mints NFTokens with a transfer fee, Alice's IOU cannot be used to pay for transfers of those NFTokens. That's fixed. Resolves XRPLF#4925. Resolves XRPLF#4941.

codecov-commenter · 2024-03-09T01:52:56Z

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 61.66%. Comparing base (c28e005) to head (b40d3fc).
Report is 8 commits behind head on develop.

❗ Current head b40d3fc differs from pull request most recent head 132aa97. Consider uploading reports for the commit 132aa97 to get more accurate results

Files	Patch %	Lines
src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp	78.57%	1 Missing and 2 partials ⚠️
src/ripple/app/tx/impl/NFTokenCreateOffer.cpp	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4946      +/-   ##
===========================================
+ Coverage    61.64%   61.66%   +0.02%     
===========================================
  Files          806      806              
  Lines        70967    70986      +19     
  Branches     36690    36704      +14     
===========================================
+ Hits         43745    43771      +26     
+ Misses       19865    19855      -10     
- Partials      7357     7360       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

dangell7 · 2024-03-18T14:54:52Z

I thought the recreation of the trustline was a feature and that was the reason for disallowIncomingTrustline?

scottschurr · 2024-03-18T16:51:54Z

Hi @dangell7, in this particular case the recreation of the trust line is a bug. The NFToken in the bug report does not have the nft::flagCreateTrustLines flag set. So the code should not be spontaneously adding trust lines to the issuer of the NFToken.

The test code that I added may be confusing. Two NFTokens are created:

One with tfTrustLine and
One without.

The NFToken created with tfTrustLine is there to verify that my changes don't break the workflow when that trust line is supposed to be created. The one without tfTrustLine tests the reported bug.

NFTokens have a lot of (too many?) options. So it's easy to get tangled up.

Does that make sense? Feel free to ask further questions.

shawnxie999

Thanks for helping out with this! Looks good!

dangell7 · 2024-03-19T08:13:35Z

Hi @dangell7, in this particular case the recreation of the trust line is a bug. The NFToken in the bug report does not have the nft::flagCreateTrustLines flag set. So the code should not be spontaneously adding trust lines to the issuer of the NFToken.

The test code that I added may be confusing. Two NFTokens are created:

One with tfTrustLine and

One without.

The NFToken created with tfTrustLine is there to verify that my changes don't break the workflow when that trust line is supposed to be created. The one without tfTrustLine tests the reported bug.

NFTokens have a lot of (too many?) options. So it's easy to get tangled up.

Does that make sense? Feel free to ask further questions.

Yepp I totally forgot about the flag on the NFToken.

seelabs

👍 Two nits, fix them (or not) at your discretion.

seelabs · 2024-03-20T19:12:16Z

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

+ // - If the Amount is not XRP, and
+ // - If the NFToken issuer is not the Amount issuer, and
+ // - If the NFToken issuer does not have a trust line for the Amount
+ // - Then reject the token accept.


Nit: The above comment doesn't clarify the code, it just repeats it. I prefer the code with out comment.

seelabs · 2024-03-20T19:18:33Z

src/ripple/protocol/Feature.h

@@ -355,6 +355,7 @@ extern uint256 const fixFillOrKill;
 extern uint256 const fixNFTokenReserve;
 extern uint256 const fixInnerObjTemplate;
 extern uint256 const featurePriceOracle;
+extern uint256 const fixNFTokenTrustlineSurprise;


That's a cute name, but it seems a little to jocular. Maybe fixEnforceNFTokenTrustline?

scottschurr mentioned this pull request Mar 9, 2024

Address rare corruption of NFTokenPage linked list #4945

Open

5 tasks

scottschurr requested a review from shawnxie999 March 9, 2024 00:46

scottschurr assigned shawnxie999 Mar 9, 2024

scottschurr added Amendment Perf impact not expected Change is not expected to improve nor harm performance. labels Mar 9, 2024

scottschurr requested a review from seelabs March 11, 2024 16:32

scottschurr assigned seelabs Mar 11, 2024

shawnxie999 reviewed Mar 18, 2024

View reviewed changes

src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp Show resolved Hide resolved

shawnxie999 approved these changes Mar 18, 2024

View reviewed changes

seelabs approved these changes Mar 20, 2024

View reviewed changes

[FOLD] Address comments from @seelabs

132aa97

scottschurr requested a review from gregtatcam March 21, 2024 19:33

scottschurr assigned gregtatcam Mar 21, 2024

scottschurr changed the title ~~Add the fixNFTokenTrustlineSurprise amendment:~~ Add the fixEnforceNFTokenTrustline amendment: Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the fixEnforceNFTokenTrustline amendment: #4946

Add the fixEnforceNFTokenTrustline amendment: #4946

scottschurr commented Mar 9, 2024 •

edited

codecov-commenter commented Mar 9, 2024 •

edited

dangell7 commented Mar 18, 2024

scottschurr commented Mar 18, 2024

shawnxie999 left a comment

dangell7 commented Mar 19, 2024

seelabs left a comment

seelabs Mar 20, 2024

seelabs Mar 20, 2024

Add the fixEnforceNFTokenTrustline amendment: #4946

Are you sure you want to change the base?

Add the fixEnforceNFTokenTrustline amendment: #4946

Conversation

scottschurr commented Mar 9, 2024 • edited

High Level Overview of Change

Context of Change

#4925

#4941

Type of Change

Consideration

Proposed Commit Message

codecov-commenter commented Mar 9, 2024 • edited

Codecov Report

dangell7 commented Mar 18, 2024

scottschurr commented Mar 18, 2024

shawnxie999 left a comment

Choose a reason for hiding this comment

dangell7 commented Mar 19, 2024

seelabs left a comment

Choose a reason for hiding this comment

seelabs Mar 20, 2024

Choose a reason for hiding this comment

seelabs Mar 20, 2024

Choose a reason for hiding this comment

scottschurr commented Mar 9, 2024 •

edited

codecov-commenter commented Mar 9, 2024 •

edited