Slither -- upgradeable contract does not protect its initialize function

Support Upgrades
1

Hello! I am building with Open Zeppelin upgradeable contracts/proxy contracts using UUPSUpgradeable and OwnableUpgradeable. When I run my contracts through slither I get a High impact warning telling me that:

ExampleV1 (contracts/examples/ExampleImpl.sol#11-19) is an upgradeable contract that does not protect its initiliaze functions: ExampleV1.initialize(address,uint256)
 
Anyone can delete the contract with: UUPSUpgradeable.upgradeTo(address) UUPSUpgradeable.upgradeToAndCall(address,bytes) 

ExampleV2 (contracts/examples/ExampleImpl.sol#21-35) is an upgradeable contract that does not protect its initiliaze functions 

Anyone can delete the contract with: UUPSUpgradeable.upgradeTo(address) UUPSUpgradeable.upgradeToAndCall(address,bytes)

https://github.com/crytic/slither/wiki/Detector-Documentation#unprotected-upgradeable-contract

My code is a version of the below (if this is bad practice please advise):

:1234: Code to reproduce

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

contract BaseImpl is UUPSUpgradeable, OwnableUpgradeable {
    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
}

contract ExampleV1 is BaseImpl {
    uint256 public randomData;

    function initialize(address _owner, uint256 _data) external initializer {
        __Ownable_init();
        transferOwnership(_owner);
        randomData = _data;
    }
}

contract ExampleV2 is BaseImpl {
    uint256 public randomData;
    uint256 public randomData2;

    function initialize(
        address _owner,
        uint256 _data1,
        uint256 _data2
    ) external initializer {
        __Ownable_init();
        transferOwnership(_owner);
        randomData = _data1;
        randomData2 = _data2;
    }
}

I took a look at the UUPSUpgradeable contract and it seems to protect against this by using onlyProxy() as well as the _authorizeUpgrade(). I CAN call the initialize function directly on the implementation address, but I don't see why this matters since the state is stored on the proxy address and the state of the implementation address is irrelevant?

In summary I tried the following:

  1. Call upgradeTo on the proxy using a non-owner address. Reverts with "caller is not the owner"
  2. Call upgradeTo on the implAddress using a non-owner address. Reverts with "Function must be called through delegatecall"
  3. Call initialize directly on the implAddress. Completes successfully but the owner of the proxy contract does not change.
  4. Call initialize with a non-owner address on the proxy. Reverts with "contract is already initialized"

If I create a second proxy with this as the implementation, upgrading through that delegate call shouldn't matter either since I'm altering the state / implementation on that new proxy.

Can someone explain what I'm missing or if this is a false positive? I really don't want to just ignore a high impact warning from slither, especially when I'm not 100% confident.

:computer: Environment

Hardhat
Ubuntu 20.04
Solidity ^0.8.4

2

Hi @Derked, this is a Slither issue, see https://github.com/crytic/slither/issues/1136.

Your example looks fine, but you should also add the following in your implementation:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

This prevents the implementation itself from being initialized. See the caution note in https://docs.openzeppelin.com/contracts/4.x/api/proxy#Initializable

You may also consider changing the initializer to public if you want the flexibility of calling that initializer from a child contract, for example.

3

Thanks so much for the response! Very helpful.

One last question I am looking for confirmation on is function signature clashing. I know this was possibly a problem with the Transparent Upgradeable Proxys, but reading the UUPS contracts it appears all the function calls are private or internal, which would make function clashing a non issue. Am I reading this correctly?

4

Both transparent and UUPS proxy patterns avoid function clashes through different designs.

With transparent proxies, function selector clashes are avoided by restricting upgrades to an admin (and if the caller is not the admin, it would always delegate to the implementation). See https://blog.openzeppelin.com/the-state-of-smart-contract-upgrades/#transparent-proxies

With UUPS, the upgrade logic is in the implementation contract, so the Solidity compiler would give an error if there was a function clash. See https://blog.openzeppelin.com/the-state-of-smart-contract-upgrades/#universal-upgradeable-proxies