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

Add code comment for proxy & upgradeability #1301

Open
montyly opened this issue Jul 26, 2022 · 3 comments
Open

Add code comment for proxy & upgradeability #1301

montyly opened this issue Jul 26, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@montyly
Copy link
Member

montyly commented Jul 26, 2022

Describe the desired feature

Since 0.8.3 we support custom code comment (ex: @custom:security non-reentrant), we can add the support for things like:

  • @custom:security isProxy
  • @custom:security isUpgradeable
  • @custom:version name:<version number>
    for the contract definition.

This will allow to leverage some of the detectors from slither-check-upgradeability automatically (without having to specify manually what contracts are upgradeable, the version etc...)

@montyly montyly added the enhancement New feature or request label Jul 26, 2022
@webthethird
Copy link
Contributor

@montyly I think this would be a good one for me to tackle, since I'm also planning on making some PRs to improve upgradeable proxy detection.

One tangentially related improvement I plan on submitting is to separate out the delegatecall detection into a Contract.is_proxy property, and redesign Contract.is_upgradeable_proxy to confirm that it is in fact upgradeable by locating the delegate target variable and its setter/getter functions. I've already done this in my own fork as part of a research project I finished recently, but I need to clean it up and break it down into incremental PR submissions with tests.

So, my question here is, would you recommend including a check for @custom:security isUpgradeableProxy in addition to the ones listed above? Or would it make more sense for devs to include both @custom:security isProxy and @custom:security isUpgradeable comments? i.e., to indicate that a) it is a proxy, and b) in fact it is an upgradeable proxy.

To start with I'll just use the two separately, since there is no Contract.is_proxy property yet, though I'd like to know for future reference.

@webthethird
Copy link
Contributor

Another question: how do you imagine @custom:version name:<version number> working? My assumption is that <version_number> is the only field that would need to be extracted here, but I want to confirm that name is not also a field.

@SheldonHolmgren
Copy link
Contributor

FTR, this seems tangentially related to crytic/crytic-compile#132

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

No branches or pull requests

3 participants