0

I've been using the Openzeppelin contracts upgradeable in my project and today I decided to run the slither tool to check how is it going. Well ... apparently it isn't very good. But I can't understand where is the real problem (or if it's a false positive), so I'm in need of your help.

The detector says:

CalculatorUpgradeable (contracts/Calculator.sol#13-59) is an upgradeable contract that does not protect its initiliaze functions: CalculatorUpgradeable.initialize() (contracts/Calculator.sol#31-33). 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)

What does it mean by "does not protect its initiliaze functions"? The initialize function has the onlyOwner modifier.

On the documentation, they recommend to add a constructor to ensure "initialize" cannot be called on the logic contract. Sorry but I didn't get the idea :(

Here is the code of the contract caught by this detector:

// SPDX-License-Identifier: MIT
pragma solidity >=0.4.22 <0.9.0;

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

contract CalculatorUpgradeable is Initializable,  OwnableUpgradeable,  UUPSUpgradeable {

    function initialize() external initializer onlyOwner {
        __Ownable_init();
    }

    function calcPercentage(uint256 amount, uint256 percentage)
        external
        pure
        returns (uint256)
    {
        return (amount * percentage * 100) / 10000;
    }

    function _authorizeUpgrade(address newImplementation)
        internal
        override
        onlyOwner
    {}
}

Thanks a lot for any clarification,

2 Answers 2

0

Based on this it seems to be a false positive https://github.com/crytic/slither/pull/1046

It should be fixed soon.

By the way, there is an issue with your smart contract

function initialize() external initializer onlyOwner {
    __Ownable_init();
}

It should be called by onlyOwner but at this moment owner is address(0) and it's not possible to call function initialize() since you're not the owner.

1
  • Adam, Thank you for clarification about the FP and the observation about misusage of onlyOwner
    – Fabiano
    Mar 26, 2022 at 18:35
0

Accordingly to the answer of contributor 0xalpharush in this issue https://github.com/crytic/slither/issues/1136, one solution to mute this FP is adding the modifier onlyProxy to the initialize function.

The modifier onlyProxy is defined in the abstract contract UUPSUpgradeable and it checks that the execution of the sinalized function is being performed through a delegatecall call and that the execution context is a proxy contract with an implementation (as defined in ERC1967) pointing to implementantion contract.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.