Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NFTokenAcceptOffer doesn't check the reserves of new NFT holder (Version: [1.11.0, 1.12.0-b3]) #4679

Closed
tequdev opened this issue Aug 29, 2023 · 0 comments · Fixed by #4767
Closed
Assignees

Comments

@tequdev
Copy link
Contributor

tequdev commented Aug 29, 2023

Issue Description

When the NFTokenAcceptOffer is executed, the transaction succeeds even though the NFToken recipient doesn't have sufficient reserves for the new NFTokenPage.

Steps to Reproduce

import { Wallet, Client, xrpToDrops } from 'xrpl'

const issuerWallet = Wallet.generate()
const userWallet = Wallet.generate()

const main = async () => {
  const client = new Client("wss://s.devnet.rippletest.net:51233/")
  await client.connect()
  console.log(await client.getServerInfo())
  console.log(client.buildVersion)

  console.log('issuer', issuerWallet.address)
  console.log('user', userWallet.address)

  // faucet
  await client.fundWallet(issuerWallet)
  await client.fundWallet(userWallet, { amount: '11' })
  console.log('fund done')

  // nft mint 
  const response1 = await client.submitAndWait({
    TransactionType: "NFTokenMint",
    Account: issuerWallet.address,
    NFTokenTaxon: 1,
    Flags: 8
  }, { wallet: issuerWallet })
  const nftoken_id = (response1.result.meta as any).nftoken_id
  console.log(nftoken_id)

  // nft offer
  const response2 = await client.submitAndWait({
    TransactionType: "NFTokenCreateOffer",
    Account: issuerWallet.address,
    Amount: xrpToDrops(0),
    NFTokenID: nftoken_id,
    Flags: 1
  }, { wallet: issuerWallet })
  const offer_id = (response2.result.meta as any).offer_id
  console.log(offer_id)

  // nft accept
  await client.submitAndWait({
    TransactionType: "NFTokenAcceptOffer",
    Account: userWallet.address,
    NFTokenSellOffer: offer_id,
  }, { wallet: userWallet })
  console.log('accept done')
}

main()

Expected Result

Should fail with tecINSUFFICIENT_RESERVE

Actual Result

Transaction successes.

Environment

1.12.0-b3 (wss://s.devnet.rippletest.net:51233/)
1.11.0 (wss://testnet.xrpl-labs.com)

Supporting Files

Reserves are checked in NFTokenMint.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Core Ledger Sep 8, 2023
@intelliot intelliot assigned shawnxie999 and unassigned ckniffen and kennyzlei Oct 16, 2023
@intelliot intelliot moved this from 📋 Backlog to 🏗 In progress in Core Ledger Oct 16, 2023
intelliot pushed a commit that referenced this issue Feb 2, 2024
Without this amendment, an NFTokenAcceptOffer transaction can succeed
even when the NFToken recipient does not have sufficient reserves for
the new NFTokenPage. This allowed accounts to accept NFT sell offers
without having a sufficient reserve. (However, there was no issue in
brokered mode or when a buy offer is involved.)

Instead, the transaction should fail with `tecINSUFFICIENT_RESERVE` as
appropriate. The `fixNFTokenReserve` amendment adds checks in the
NFTokenAcceptOffer transactor to check if the OwnerCount changed. If it
did, then it checks the new reserve requirement.

Fix #4679
sophiax851 pushed a commit to sophiax851/rippled that referenced this issue Jun 12, 2024
…LF#4767)

Without this amendment, an NFTokenAcceptOffer transaction can succeed
even when the NFToken recipient does not have sufficient reserves for
the new NFTokenPage. This allowed accounts to accept NFT sell offers
without having a sufficient reserve. (However, there was no issue in
brokered mode or when a buy offer is involved.)

Instead, the transaction should fail with `tecINSUFFICIENT_RESERVE` as
appropriate. The `fixNFTokenReserve` amendment adds checks in the
NFTokenAcceptOffer transactor to check if the OwnerCount changed. If it
did, then it checks the new reserve requirement.

Fix XRPLF#4679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

4 participants