Closed tina1998612 closed 2 years ago
Fix double refund issue by adding hasRefunded mapping.
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
Hi I added a little more changes:
I also added the tests, let me know if you have any feedback! :)
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.
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).
Agree, just calculate and store the buyer amount in time, like amountPaid
mapping.
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.
But I still wonder how much we care about the same NFT being refunded twice. Maybe the isOwnerMint
mapping is enough?
@tina1998612 what do you think about this approach? https://github.com/exo-digital-labs/ERC721R/tree/fix/owner-refund
https://github.com/exo-digital-labs/ERC721R/commit/933c897355f1168df7af7e5f90aa892f6dda53c9
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 what do you think about this approach?
https://github.com/exo-digital-labs/ERC721R/tree/fix/owner-refund
https://github.com/exo-digital-labs/ERC721R/commit/933c897355f1168df7af7e5f90aa892f6dda53c9
Hmm as you said it increases gas cost for minting.
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?
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.
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.
@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?
@elie222 OK, I check it out if still need to add some test
Arr, I din't see this pull request. This is better approach. I remove my pull request.
Arr, I din't see this pull request. This is better approach. I remove my pull request.
Thanks for the contribution in any case!
Added: