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

One user can burn another user's tokens? #117

Closed
TravelingTechGuy opened this issue Feb 18, 2022 · 4 comments
Closed

One user can burn another user's tokens? #117

TravelingTechGuy opened this issue Feb 18, 2022 · 4 comments

Comments

@TravelingTechGuy
Copy link

Hope I got it right - could always be my implementation.
I tried adding a burn functionality to my token. I added the following:

function burn(uint256 tokenId) public {
    _burn(tokenId);
    emit Burned(msg.sender, tokenId);
}

All it does is just pass the token id to _burn and emit an event.

I then added this test, which was supposed to fail:

it('should not allow user to burn an NFT they don\'t own', async () => {
      await myToken.connect(user1).mint(1, {value: ethers.utils.parseEther(regularPrice)});  //creates a single token with ID 0
      await myToken.connect(user2).burn(0);  //supposed to fail: user 1 owns token 0
});

But it does not. I looked into the _burn code, and it looks like it does not check ownership before proceeding to transfer the token to 0x0.

Should this check be enforced by my contract, or is there something missing in the _burn function?

@Austinhs
Copy link
Contributor

Austinhs commented Feb 18, 2022

Depending on your logic for burn, While it probably is rare I could see the burn ability existing in some other function. For example... After X amount of time, the contract will burn your previous token and mint you a new 1 or 2. If the contract/deployer manages that then it's not an "owner".

I think the idea with burn was to simply refer to the action of burning but checking for ownership is something that depends on business logic.

But lets see what @Vectorized says -- I think he added this

@Vectorized
Copy link
Collaborator

Please refer to and use the ERC721ABurnable class. It’s burn function will only allow authorised users to burn.

The internal _burn function does not have the authorised check.

This is to be consistent with OpenZeppelin’s implementation.

@ahbanavi
Copy link
Contributor

Also, it was discussed earlier in #61 (comment)

@TravelingTechGuy
Copy link
Author

Thanks @Vectorized! I've updated my contract accordingly.

I'd add this to the README so people won't just call the internal _burn by mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants