openzeppelin-contracts
openzeppelin-contracts copied to clipboard
OpenZeppelin
Remove isContract due to potential misuse
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.
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.
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
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.
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?
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.
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.
@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?
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
isContractis removed?
@Amxx
I would propose manually inlining the logic, but instead of assembly we use address(..).code.size
@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. :)
Here we go: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3682
cc @frangio @Amxx