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

Remove isContract due to potential misuse #3417

Closed
devtooligan opened this issue May 17, 2022 · 13 comments
Closed

Remove isContract due to potential misuse #3417

devtooligan opened this issue May 17, 2022 · 13 comments
Labels
breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@devtooligan
Copy link

In spite of the warnings provided in the comments of the function itself, the name isContract is a misnomer and creates a potential security risk for anyone who doesn't bother to read the notes or someone who is reviewing a 3rd party contract that uses this fn.

There is a misconception that calling this function will return false if the address is an eoa. This can lead to the inadvertant introduction of an exploit and other risks already clearly identified in the comments. But comments inside the function are not enough in this case where the name of the fn is so blatantly misleading.

Propose changing the name of the function to hasCode which is much more descriptive of what the function does. This should be a breaking change and may end up being a wake up call to anyone who has been misusing the fn to date.

@kryptoklob
Copy link

I don't think isContract is a misnomer anymore than hasCode would be. It would be misleading if the function was called isNotEOA, but it's not. hasCode has the same caveats that isContract does, because they're literally synonymous.

@devtooligan
Copy link
Author

Disagree. It's not clear from the name isContract what method is being used to determine if the address is a contract or not. You know it's synonymous, but someone who hasn't looked at the fn has no idea. They just see the fn name and think, oh it's a contrct.

hasCode indicates that the address has code at this moment in time and doesn't come with the implications that isContract does

@nventuro
Copy link
Contributor

nventuro commented May 17, 2022

There is a misconception that calling this function will return false if the address is an eoa

isContract will always return false for an account that is an EOA. The problem is that that it will also return false for things that are not contracts (i.e. don't have code), but have, will have or have had some of the properties of smart contracts (such as the ability to commit to future action), which in some scenarios defeats the purpose of the check.


That said, I don't quite follow the distinction you're making between 'having code', and 'being a contract'. It seems like everyone uses 'smart contract' to mean 'account with code', so the name change doesn't seem to add much.

@devtooligan
Copy link
Author

I guess I should have said, "there is a misconception that if isContract() returns false the address is an eoa."

I guess because there are two types of addresses, those with code and those for eoas. Because you are a seasoned developer, you know that the isContract function is checking if there is code at that address. In fact, you probably wouldn't even use the isContract function in the first place, neither would I. The users of isContract are generally newer users who may not realize that the check for isContract is to check whether or not it has code.

There have been many cases of developers using !isContract thinking it means the address is an eoa and inadvertantly introducing exploits into their code. Changing the name to hasCode removes that implication, and at least forces them to think about it a little more, and maybe even open up the function and read all those helpful comments.

One last question, tbh I feel that removing this function altogether from OZ is the best solution. The reason I submitted this rename issue is because it felt like an acceptable middle ground. Would you be open to a pr that removes all references to isContract altogether?

@alcuadrado
Copy link

alcuadrado commented May 17, 2022

I believe an assertIsContract(addr) that reverts if addr has no code is the only safe utility that you can provide. I proposed this to @frangio some time ago, but unfortunately, I've forgotten what the arguments against it were.

PS: I don't understand why hasCode would be different from isContract. That's actually what brought me to this issue. IMO the problem with both is that they return a boolean when only true and unknown are valid answers.

@frangio
Copy link
Contributor

frangio commented May 17, 2022

assertIsContract would be safe, but I'm not sure what it would be useful for. isContract is necessary to implement standards like ERC721 and ERC1155.

Nowadays Solidity has addr.code.lengh that we could use directly. When Address.isContract was introduced this feature didn't exist and isContract was used to wrap the necessary assembly.

Note that removal of isContract would be a breaking change, so the best we can do now is to deprecate it and mark it for removal in 5.0. I would be in favor of this.

@devtooligan
Copy link
Author

devtooligan commented May 17, 2022

@frangio I would be a huge supporter of deprecating and removing isContract at next major release as you suggest (and I know many others would as well). Is there anything I can do to help with that?

@Amxx
Copy link
Collaborator

Amxx commented May 18, 2022

The thing is, we need that function in some parts of the repo (ERC721 & ERC1155). We can mark isContract as deprecate and remove it in 5.0, how will be handle the place where it is used ?

  • Will we manually inline the logic? (not a big fan of that)
  • Will we continue providing this function but with a different name? If yes, should we start publishing it with the new name before isContract is removed?

@devtooligan
Copy link
Author

@Amxx
I would propose manually inlining the logic, but instead of assembly we use address(..).code.size

@Amxx Amxx added the breaking change Changes that break backwards compatibility of the public API. label May 18, 2022
@Amxx Amxx added this to the 5.0 milestone May 18, 2022
@frangio frangio changed the title Rename isContract to hasCode Remove isContract due to potential misuse May 20, 2022
@ashwinYardi
Copy link
Contributor

@Amxx I would propose manually inlining the logic, but instead of assembly we use address(..).code.size

If this is agreed upon, would love to draft a PR for this. :)

@frangio
Copy link
Contributor

frangio commented Aug 11, 2022

@ashwinYardi Please open a PR against the next branch (future 5.0).

@ashwinYardi
Copy link
Contributor

ashwinYardi commented Sep 6, 2022

Here we go: #3682

cc @frangio @Amxx

@frangio
Copy link
Contributor

frangio commented May 31, 2023

Fixed in #3945.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

No branches or pull requests

7 participants