crytic / crytic/slither

update unprotected-upgradeable to consider whether selfdestruct functions can be called directly

Open
#1,646 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

Uninitialized UUPS contracts that use OZ >=4.3.2 will have FPs because the functions which delegatecall (`upgradeTo` and `upgradeToAndCall`) must be [called within the context of deletatecall](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/91e8d0ba3c3beb8a1db31310d8599664e48639ef/contracts/proxy/utils/UUPSUpgradeable.sol#L68-L86) (they have an `onlyProxy` modifer). Thus, the UUPS implementation cannot be destroyed although there may be other undesirable consequeunces.

The detector should consider whether the functions found in `_can_be_destroyed` have this restriction:
https://github.com/crytic/slither/blob/4c976d5af56219eeef079e03a35009af3e03644d/slither/detectors/statements/unprotected_upgradeable.py#L9

Ref https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-5vp3-v4hc-gx76

Contributor guide

Open the contributing guide

Research direction

Start in slither/detectors/statements/unprotected_upgradeable.py, especially _can_be_destroyed, and review the linked OpenZeppelin UUPSUpgradeable implementation and its onlyProxy restriction. The detector should distinguish directly callable selfdestruct paths from upgrade functions that require delegatecall context, with the false positive described in the issue no longer reported.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, solidity
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.