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.

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

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.3%. Comparing base (8258640) to head (fa2b678).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #4946     +/-   ##
=========================================
- Coverage     71.3%   71.3%   -0.0%     
=========================================
  Files          796     796             
  Lines        66968   66981     +13     
  Branches     10866   10889     +23     
=========================================
- Hits         47771   47751     -20     
- Misses       19197   19230     +33

Files	Coverage Δ
src/ripple/protocol/Feature.h	`100.0% <ø> (ø)`
src/ripple/protocol/impl/Feature.cpp	`94.6% <ø> (ø)`
src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp	`89.7% <92.3%> (+0.2%)`	⬆️

... and 7 files with indirect coverage changes

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?

Fix 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. Resolves XRPLF#4925.

scottschurr · 2024-06-17T19:20:42Z

The XLS-25d: NFTokenMintOffer commit ( 9f7c619) interacted a great deal with this pull request.

That commit absorbed the fix for issue If same account issues an IOU asset and an NFToken, that account's IOU cannot be used to pay the fee for that account's NFTs #4941 that was previously in this pull request.
Similarly, that commit absorbed the unit tests for issue If same account issues an IOU asset and an NFToken, that account's IOU cannot be used to pay the fee for that account's NFTs #4941 that were previously in this commit.

So I rebased this commit to the tip of develop and resolved the differences. I also squashed the commits and changed the commit message to ...

Reflect the changed amendment name and
That this commit only addresses issue NFT issuer is inadvertently creating a trustline due to third-party transactions(Version: 2.1.0) #4925 now.

I'm not marking this as passed yet, since I want to allow my reviewers an opportunity to check my work with the squash and rebase. But as far as I'm concerned I think this branch is ready.

seelabs

Still 👍 after latest changes

shawnxie999

looks good 👍

* upstream/develop: (32 commits) fixInnerObjTemplate2 amendment (XRPLF#5047) Set version to 2.3.0-b1 Ignore restructuring commits (XRPLF#4997) Recompute loops (XRPLF#4997) Rewrite includes (XRPLF#4997) Rearrange sources (XRPLF#4997) Move CMake directory (XRPLF#4997) Add bin/physical.sh (XRPLF#4997) Prepare to rearrange sources: (XRPLF#4997) Change order of checks in amm_info: (XRPLF#4924) Add the fixEnforceNFTokenTrustline amendment: (XRPLF#4946) Replaces the usage of boost::string_view with std::string_view (XRPLF#4509) docs: explain how to find a clang-format patch generated by CI (XRPLF#4521) XLS-52d: NFTokenMintOffer (XRPLF#4845) chore: remove repeat words (XRPLF#5041) Expose all amendments known by libxrpl (XRPLF#5026) fixReducedOffersV2: prevent offers from blocking order books: (XRPLF#5032) Additional unit tests for testing deletion of trust lines (XRPLF#4886) Fix conan typo: (XRPLF#5044) Add new command line option to make replaying transactions easier: (XRPLF#5027) ...

Fix 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. Resolves XRPLF#4925.

scottschurr mentioned this pull request Mar 9, 2024

Address rare corruption of NFTokenPage linked list #4945

Merged

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

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

scottschurr force-pushed the nft-trustline branch from 132aa97 to d0fef14 Compare June 17, 2024 19:05

seelabs approved these changes Jun 17, 2024

View reviewed changes

shawnxie999 approved these changes Jun 17, 2024

View reviewed changes

scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 17, 2024

Merge branch 'develop' into nft-trustline

fa2b678

seelabs merged commit 223e6c7 into XRPLF:develop Jun 18, 2024
19 checks passed

seelabs mentioned this pull request Jun 20, 2024

Proposed 2.3.0-b1 #5050

Merged

1 task

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

Loading

codecov-commenter commented Mar 9, 2024 •

edited by codecov bot

Loading

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

scottschurr commented Jun 17, 2024

seelabs left a comment

shawnxie999 left a comment

Add the fixEnforceNFTokenTrustline amendment: #4946

Add the fixEnforceNFTokenTrustline amendment: #4946

Conversation

scottschurr commented Mar 9, 2024 • edited Loading

High Level Overview of Change

Context of Change

#4925

#4941

Type of Change

Consideration

Proposed Commit Message

codecov-commenter commented Mar 9, 2024 • edited by codecov bot Loading

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 Jun 17, 2024

seelabs left a comment

Choose a reason for hiding this comment

shawnxie999 left a comment

Choose a reason for hiding this comment

scottschurr commented Mar 9, 2024 •

edited

Loading

codecov-commenter commented Mar 9, 2024 •

edited by codecov bot

Loading