Slither claims an upgradeTo function is exposed when it (likely?) is not

[FlacoJones]

Describe the issue:

When running Slither on this UUPS upgradeable contract](https://github.com/OpenQDev/OpenQ-Contracts/blob/development/contracts/OpenQ/Implementations/OpenQV0.sol), it triggers the unprotected-upgradeable-contract critical warning as shown below:

However, the latest OpenZeppelin UUPSUpgradeable.sol does indeed protect its upgradeTo method with both onlyProxy, as well as an optional _authorizeUpgrade method which I have onlyOwnered as seen here.

So I believe that only an owner calling via the proxy will be able to call upgradeTo....unless I'm wrong and missing something and Slither is doing its wonderful job 😅.

Code example to reproduce the issue:

git clone https://github.com/OpenQDev/OpenQ-Contracts.git cd OpenQ-Contracts solc-select use 0.8.12 slither . --exclude-dependencies

Version:

slither 0.8.2

Relevant log output:

OpenQV0 (contracts/OpenQ/Implementations/OpenQV0.sol#18-254) is an upgradeable contract that does not protect its initiliaze functions: OpenQV0.initialize(address) (contracts/OpenQ/Implementations/OpenQV0.sol#30-35). Anyone can delete the contract with: UUPSUpgradeable.upgradeTo(address) (node_modules/@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol#72-75)UUPSUpgradeable.upgradeToAndCall(address,bytes) (node_modules/@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol#85-88)Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unprotected-upgradeable-contract

[0xalpharush]

I'll have to think about this more before modifying the detector as we'd prefer false positives over false negatives. For now, you can add an onlyProxy modifier on the initializer function to "mute" this result. Note, this depends on https://github.com/crytic/slither/pull/1122 which is not released yet, so you'd need to clone master. I think this would also obviate the need for onlyProxy on other functions (example) unless you have another motivation for doing so.

[FlacoJones]

Sounds good! Airing on the side of false positives is what I'd prefer to in terms of security. Awesome job with the package.

[FlacoJones]

Feel free to close if this is in the works anyways.

[fabianorodrigo]

@0xalpharush , I found the onlyProxy modifier only in the UUPSUpgradeable contract. What would you suggest for contracts following the Transparent pattern? Doing my own implementation of onlyProxy and use it would work?

What about minimal proxies (ERC-1167)? In this specific case, I'm facing the issue with unprotected-upgradeable-contract detector.

Thanks

[0xalpharush]

Without looking at the code, it's difficult for me to assess whether this a false negative. This detector looks for an initialize function and a selfdestruct that is reachable. I would make sure that someone cannot call the function that contains selfdestruct directly on the implementation contract.

[fabianorodrigo]

Thank you for your attention, @0xalpharush,

This case is about the contract Game in this repository https://github.com/fabianorodrigo/dAppSoccerbet .

Game is a implementation of minimal proxy pattern (ERC1167). When I deploy GameFactory (upgradeable transparent), I also create a new Game that will serve just as a template. When calling the function GameFactory.newGame, then that first instance of Game is "cloned" (actually is created a proxy that delegates the calls to that first instance).

I created a modifier onlyDelegateCall (similar to onlyProxy) and applyed it to the initialize function. However, slither unprotected-upgradeable-contract detector still points that the initialize function is unprotected.

Regards,

[0xalpharush]

Thanks for sharing the code. The vulnerability slither is warning of would be an attacker calling Game.initialize and setting themself as the owner. Then, they could call Game.destroyContract on the implementation and cause calls via the proxy to either successfully delegatecall to an address with no code or revert if the proxy checks for contract existence. (See https://blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/ for further detail on this issue.)

By adding this modifier, the owner can only be set with initialize via the proxy, preventing someone from calling destroyContract directly since their address cannot be equal to the uininitialized value of owner, 0x0.

I hope that provides background on the detector and its recommendation. To "mute" this result, you can either change the name of onlyDelegateCall to onlyProxy or you can add an ignore comment, slither-disable-next-line above Game.initialize

I will make a PR that adds onlyDelegateCall to the whitelist for the unprotected-upgradeable-contract detector, but that won't be available until the next release.

[fabianorodrigo]

Thanks a lot for this explanation, @0xalpharush ! I will follow your suggestion to renaming modifier onlyDelegateCall.

[0xalpharush]

onlyDelegateCall just needs to be added here https://github.com/crytic/slither/blob/4c976d5af56219eeef079e03a35009af3e03644d/slither/detectors/statements/unprotected_upgradeable.py#L45