`CVX1.sol` does not check `treasuryDao` address is zero address as done in other contracts

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x7c14ad1f8fa442488ddff348435fdbda4465c93cd7641d75b583cf0aa29e106b Severity: medium

Description: Description\

The inscope convergence contracts transfer their ownership to treasury dao address during initialization of contract via initialize() function.

Most of the contracts make sure that treasuryDAO address is not zero address before transferring the contract ownership. This is to prevent zero address ownership which could result in overall redeployment of contract.

These below contracts have checked treasuryDAO address must not be address(0) before transferring the contract ownership.

1) CvgCvxStakingPositionService.sol 2) CvxAssetStakerBuffer.sol 3) CvxAssetStakingService.sol 4) CvxRewardDistributor.sol

This is done by checking as:

        address _treasuryDao = cvgControlTower.treasuryDao();
        require(_treasuryDao != address(0), "TREASURY_DAO_ZERO");
        _transferOwnership(_treasuryDao);

This enures that treasuryDAO address is not zero address.

However, below contracts does not ensure the above check in their initialize() function,

1) CVX1.sol 2) CvxConvergenceLocker.sol 3) CvxStakingPositionManager.sol

There should be conformity in design while transferring the ownership of contracts which is maintained by 4 contracts, however above 3 contracts does not ensures it.

Impact Contract ownership transfer to treasury dao address is at risk if the treasury dao address is not set so by default it would be address(0). Therefore, checking against address(0) is very much required while transferring the ownership to it. This is ensured in few contracts and rest contract left without it. At worst, it will result in redeployment of contracts.

Recommendation to fix\

Please consider this check in initialize() of CVX1.sol, CvxConvergenceLocker.sol, CvxStakingPositionManager.sol

        address _treasuryDao = cvgControlTower.treasuryDao();
        require(_treasuryDao != address(0), "TREASURY_DAO_ZERO");
        _transferOwnership(_treasuryDao);

[0xRizwan]

The issue is identified as Medium severity as it will impact the whole contracts functionality. It will break the contract functionality which is accessed by onlyOwner modifier. The treasuryDao address is automatically fetched without checking while intializing the affected contracts so possibility of happening is issue is high as well as impact is high as the initialize() function wont revert due to missing validation check as done in other contracts.

In CVX1.sol

    function initialize(
        string memory _name,
        string memory _symbol,
        ICvxRewardPool _cvxRewardPool
    ) external initializer {
        __ERC20_init(_name, _symbol);

        cvxRewardPool = _cvxRewardPool;
        CVX.approve(address(_cvxRewardPool), type(uint256).max);

-        _transferOwnership(cvgControlTower.treasuryDao());
+        address _treasuryDao = cvgControlTower.treasuryDao();
+        require(_treasuryDao != address(0), "TREASURY_DAO_ZERO");
+        _transferOwnership(_treasuryDao);
    }

In CvxConvergenceLocker.sol,

    function initialize(
        string memory _name,
        string memory _symbol,
        IDelegateRegistry _cvxDelegateRegistry,
        ICvx1 _cvx1,
        RewardConfiguration[] calldata _rewardTokensConfiguration
    ) external initializer {
        __ERC20_init(_name, _symbol);

        mintFees = 100; // 1%

        cvxDelegateRegistry = _cvxDelegateRegistry;
        cvx1 = _cvx1;

        for (uint256 i; i < _rewardTokensConfiguration.length; ) {
            rewardTokensConfiguration.push(_rewardTokensConfiguration[i]);
            unchecked {
                ++i;
            }
        }

        CVX.approve(address(CVX_LOCKER), type(uint256).max);

-        _transferOwnership(cvgControlTower.treasuryDao());
+        address _treasuryDao = cvgControlTower.treasuryDao();
+        require(_treasuryDao != address(0), "TREASURY_DAO_ZERO");
+        _transferOwnership(_treasuryDao);
    }

and in CvxStakingPositionManager.sol,

    function initialize() external initializer {
        /// @dev Initialize the NFT contract
        __ERC721_init("Staking Positions Convergence", "STK-CVG");

        /// @dev Initialize first token ID as 1
        nextId = 1;

        /// @dev Timelocking maximum is initialized at 10 days.
        maxLockingTime = 10 days;

-        _transferOwnership(cvgControlTower.treasuryDao());
+        address _treasuryDao = cvgControlTower.treasuryDao();
+        require(_treasuryDao != address(0), "TREASURY_DAO_ZERO");
+        _transferOwnership(_treasuryDao);
    }

[Jonsey]

0xB0Afc8363b8F36E0ccE5D54251e20720FfaeaeE7 is a deployed contract with cvgControlTower.treasuryDao() === 0x0af815364BD9e9E60f3d2D3bAc1320B77d3E35F7

[PlamenTSV]

This would be a centralization risk, thus OOS. The only thing apart from that is that it is inconsistency around the different code files.

[0xRizwan]

@PlamenTSV How it is OOS? I believe it fits in low severity defination per rules.

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

[PlamenTSV]

@0xRizwan The 2 points I currently see are:

1. The issue is out of scope since the initialization and proper arguments are up to the owners and they are trusted, per the rules in the guidelines
2. It differs from checks in different files which is borderline low/info I will give it both labels and let the sponsor decide if it is OOS. Thanks for the comment!

[0xR3vert]

As mentioned above, the treasuryDao is already configured in the controlTower, so the require present in the initializer is an unecessary check. Even if we deploy without this check in the initializer, we systematically run post-deployment checks to make sure that everything has been configure correctly. It's simply an inconstistency in this contract that has no real impact once we're in production. In conclusion, it's invalid.