Fix double refund issue by adding hasRefunded mapping

[tina1998612]

Added:

1. The same NFT can only be refunded once, because the mint price is paid only once.
2. If the NFT was freely minted by the owner, it cannot be refunded.

[tina1998612]

Fix double refund issue by adding hasRefunded mapping.

[elie222]

Thanks for the PR. Could you explain the reasoning for this addition a little more, please.

Do we mind if a user is returning an NFT twice? As long as they paid the mint price, they can receive it back. Unless I'm missing something

[tina1998612]

Hi I added a little more changes:

1. The same NFT can only be refunded once, because the mint price is paid only once.
2. If the NFT was freely minted by the owner, it cannot be refunded.

I also added the tests, let me know if you have any feedback! :)

[madeinfree]

I help to added the reproduce（for who looking at this PR but doesn't know what happened） testcase repo by foundry

https://github.com/madeinfree/ERC721R-refund-drain-testcase

the last test testIWantToRug result is

The refundAddress balance is 5 ether, spend 0.1 ether to buy one token then get the tokenId 14.

uint256[] memory ids = new uint256[](15);
  for(uint256 i = 0; i < 15; i++) {
  ids[i] = 14; // only one token
}

cheats.prank(king);
wagmi.refund(ids);
assertEq(king.balance, 6.4 ether); drain all balance.

[elie222]

Thanks for the updates. I agree we need to get a fix in for this. I don't believe it affects the CryptoFighters example because we store exactly how much everyone paid for their NFT.
We may even want to do:

mapping(uint256 => uint256) public amountPaid; // mapping from tokenId to amount paid for token

If a refund occurs then we drop the amountPaid to 0. This would also cover the double refund on the same NFT issue. The advantage is that we use one mapping instead of 2. (Although I'm not actually sure that we care if the same NFT is refunded twice. As long as someone paid for the mint of an NFT. But regardless this approach would fix both I believe).

[madeinfree]

Agree, just calculate and store the buyer amount in time, like amountPaid mapping.

[elie222]

Looking at this further, what I like about @tina1998612's approach is that it doesn't increase the gas costs for minting. It only increases gas fees for owner mint and for refunds. Which seems preferable.

[elie222]

But I still wonder how much we care about the same NFT being refunded twice. Maybe the isOwnerMint mapping is enough?

[elie222]

@tina1998612 what do you think about this approach?
https://github.com/exo-digital-labs/ERC721R/tree/fix/owner-refund

933c897

[tina1998612]

But I still wonder how much we care about the same NFT being refunded twice. Maybe the isOwnerMint mapping is enough?

If a refunded NFT is resold on Opensea (maybe by refund address), the same token ID cannot be refunded twice.

[tina1998612]

@tina1998612 what do you think about this approach?

https://github.com/exo-digital-labs/ERC721R/tree/fix/owner-refund

933c897

Hmm as you said it increases gas cost for minting.

[elie222]

But I still wonder how much we care about the same NFT being refunded twice. Maybe the isOwnerMint mapping is enough?

If a refunded NFT is resold on Opensea (maybe by refund address), the same token ID cannot be refunded twice.

Right. But wondering if this matters?

If account A mints the NFT 55 pays 0.1 and refunds and gets 0.1 back.
Now refund address sells it on OpenSea for 0.2eth and someone else tries to refund token 55 and get 0.1eth back for it.
Do we mind?

[tina1998612]

But I still wonder how much we care about the same NFT being refunded twice. Maybe the isOwnerMint mapping is enough?

If a refunded NFT is resold on Opensea (maybe by refund address), the same token ID cannot be refunded twice.

Right. But wondering if this matters?

If account A mints the NFT 55 pays 0.1 and refunds and gets 0.1 back.

Now refund address sells it on OpenSea for 0.2eth and someone else tries to refund token 55 and get 0.1eth back for it.

Do we mind?

Yes because the NFT smart contract wouldn't have enough fund to refund if the earnings from opensea is not sent to the contract directly. It would be more complex and costly to send the funds back.

[elie222]

Yes because the NFT smart contract wouldn't have enough fund to refund if the earnings from opensea is not sent to the contract directly. It would be more complex and costly to send the funds back.

Got it. I merged in your version. amountPaid would work to stop contract running out of funds but would increase mint price so we'll put it onto the owner and refunders instead.

[elie222]

@madeinfree do you want to make a PR with any additional test cases from your repo that would be helpful? Or they're all covered now?

[madeinfree]

@elie222 OK, I check it out if still need to add some test

[lawweiliang]

Arr, I din't see this pull request. This is better approach. I remove my pull request.

[elie222]

Arr, I din't see this pull request. This is better approach. I remove my pull request.

Thanks for the contribution in any case!

[elie222]

@tina1998612 #36