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):
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:
- Call upgradeTo on the proxy using a non-owner address. Reverts with "caller is not the owner"
- Call upgradeTo on the implAddress using a non-owner address. Reverts with "Function must be called through delegatecall"
- Call initialize directly on the implAddress. Completes successfully but the owner of the proxy contract does not change.
- 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.
Environment
Hardhat
Ubuntu 20.04
Solidity ^0.8.4