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 Address.isContract #3945

Merged
merged 20 commits into from
Jan 24, 2023

Conversation

JulissaDantes
Copy link
Contributor

@JulissaDantes JulissaDantes commented Jan 11, 2023

Replaces #3682
Fixes #3417

  • This PR removes isContract utility function from Address.sol library.
  • Wherever isContract was used in the other contracts, is replaced by inlining address(...).code.length > 0

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@JulissaDantes JulissaDantes mentioned this pull request Jan 11, 2023
3 tasks
@JulissaDantes JulissaDantes marked this pull request as ready for review January 11, 2023 13:35
@JulissaDantes JulissaDantes requested review from frangio and Amxx January 11, 2023 13:35
contracts/proxy/utils/Initializable.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/extensions/ERC721Consecutive.sol Outdated Show resolved Hide resolved
contracts/token/ERC777/ERC777.sol Outdated Show resolved Hide resolved
Comment on lines 13 to 38
* [IMPORTANT]
* ====
* It is unsafe to assume that an address for which this function returns
* false is an externally-owned account (EOA) and not a contract.
*
* Among others, `isContract` will return false for the following
* types of addresses:
*
* - an externally-owned account
* - a contract in construction
* - an address where a contract will be created
* - an address where a contract lived, but was destroyed
* ====
*
* [IMPORTANT]
* ====
* You shouldn't rely on `isContract` to protect against flash loan attacks!
*
* Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets
* like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract
* constructor.
* ====
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a new file at docs/modules/ROOT/pages/knowledge.adoc, add it in docs/modules/ROOT/nav.adoc, and include this content there so we don't lose it. I think it should be a section called "Can I restrict a function to EOAs only?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the new doc page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of "FAQ" as the title for the section? Or you can check how I did it and if it can remain like that, no problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes FAQ works.

@frangio frangio changed the title Remove isContract Remove Address.isContract Jan 11, 2023
JulissaDantes and others added 5 commits January 12, 2023 11:34
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
@@ -0,0 +1,14 @@
Can I restrict a function to EOAs only?
Copy link
Contributor

@frangio frangio Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer to this question is closer to "no", but the content below seems to be saying "yes".

The key part that is missing is that although isContract (equivalently, addr.code.length > 0) may seem to differentiate contracts from EOAs, it can only say that an address is currently a contract, and its negation (that an address is not currently a contract) does not imply that the address is an EOA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, please check the new version to see if its clear enough after the update.

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


When calling external addresses from your contract it is unsafe to assume that an address is an externally-owned account (EOA) and not a contract. Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract constructor.

Some criteria to discard an address as a contract address can be:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an address has code size = 0, this does not mean that it is not a contract. Some counterexamples are:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the updated version

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

🦋 Changeset detected

Latest commit: 91802e5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

This was referenced Sep 10, 2024
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

Successfully merging this pull request may close these issues.

3 participants