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

ERC20Burnable: Description needs correction #5138

Open
SKYBITDev3 opened this issue Aug 2, 2024 · 7 comments
Open

ERC20Burnable: Description needs correction #5138

SKYBITDev3 opened this issue Aug 2, 2024 · 7 comments

Comments

@SKYBITDev3
Copy link

The description states that the ERC20Burnable extension makes it possible for token holders to destroy others' tokens, which is incorrect:

Extension of ERC20 that allows token holders to destroy both their own tokens and those that they have an allowance for, in a way that can be recognized off-chain (via event analysis).

Here's a corrected description:
Extension of ERC20 that allows token holders to destroy their own tokens or allow other particular accounts or contracts to destroy tokens that the holder had granted an allowance for, in a way that can be recognized off-chain (via event analysis).

@frangio frangio transferred this issue from OpenZeppelin/docs.openzeppelin.com Aug 2, 2024
@Amxx
Copy link
Collaborator

Amxx commented Aug 2, 2024

Hello @SKYBITDev3

I don't see what is wrong with the current description. I look correct to me.
IMO the proposed "corrected description" is less clear.

@ernestognw ?

@ernestognw
Copy link
Member

I think it's currently clear as it states that it "allows token holders to destroy both: a) their own tokens and b) those they have allowance for". The new one is perhaps more explicit but isn't changing the meaning imo.

@SKYBITDev3
Copy link
Author

SKYBITDev3 commented Aug 2, 2024

Having burnFrom in a token that you hold makes it possible for others to burn your tokens if you give an allowance to them.

It makes much more sense to state that the token holder can allow others to burn his tokens, instead of stating that the token holder can burn others' tokens.

@ernestognw
Copy link
Member

instead of stating that the token holder can burn others' tokens.

This is not what the docs state. It states that anyone can burn tokens they have an allowance for. Your proposal feels something like "allow other's to have allowance".

@SKYBITDev3
Copy link
Author

instead of stating that the token holder can burn others' tokens.

This is not what the docs state

Yes it does. The statement "allows token holders to destroy both their own tokens and those that they have an allowance for" is the same as the combination of these two statements:
"allows token holders to destroy their own tokens"
"allows token holders to destroy tokens that they have an allowance for", which means other holders' tokens, as own tokens are covered by the first statement.

It's the frames of reference that is the issue here, i.e. who's the holder and who's the burner.
With burn, the holder is the burner;
With burnFrom, any account or contract is the burner, if the token holder had given allowances to spend his tokens beforehand to those accounts or contracts, regardless of whether those accounts or contracts are holders of the tokens. The published description doesn't make this clear, as it just says token holders are the burners for both burn and burnFrom.

So you should make it clear thatburnFrom makes it possible for any account or contract to burn a holder's tokens if that holder gives an allowance beforehand, even if those accounts or contracts don't hold any of the tokens.

@Amxx
Copy link
Collaborator

Amxx commented Aug 19, 2024

So you should make it clear that burnFrom makes it possible for any account or contract to burn a holder's tokens if that holder gives an allowance beforehand, even if those accounts or contracts don't hold any of the tokens.

This is the case even without burnForm, because any account that has allowance can use transferFrom to get full ownership, and then burn that.

Again, I think the documentation is very clear that this it is possible to burn tokens you have allowance for.

Extension of {ERC20} that allows token holders to destroy both their own tokens and those that they have an allowance for.

@SKYBITDev3
Copy link
Author

SKYBITDev3 commented Aug 19, 2024

This is the case even without burnForm, because any account that has allowance can use transferFrom to get full ownership, and then burn that.

Yes, I know that, burnForm is just a specialized version of transferFrom. That's not the issue.

The currently published statement "allows token holders to destroy both their own tokens and those that they have an allowance for" is the same as saying the following 2 statements:

  • allows token holders to destroy their own tokens
  • allows token holders to destroy others' tokens that they have an allowance for

Even though the statements aren't wrong, they fail to include this very important statement:
allows non- token holders to destroy others' tokens that they have an allowance for

My point is that burnForm should not only be about a token holder doing any burning, but that's what the published description only covers. i.e. the frame of reference is fixated only on the token holder doing burning.

Having burnForm makes it possible for anyone else (or any contract), even if they don't hold any of the tokens, to burn an amount of tokens that the token holder had given an allowance for (via approve or a signature for use with permit) to the burner.

Similarly and more generally, having transferForm makes it possible for anyone else (or any contract), even if they don't hold any of the tokens, to transfer an amount of tokens that the token holder had given an allowance for (via approve or a signature for use with permit) to the transferer.

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

3 participants