Convex staking contracts can not be stored in `isCvxStaking` mapping due to lack of setter functionality

hats-bug-reporter[bot] commented 7 months ago

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x91977298fe67be0f7296c88032c100de4723442a8de844c78b5123ffb18ddab7 Severity: medium

Description: Description\ CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol are the contracts which are in charge of registering deposits and withdraws of staking position. Both of these contracts inherits from the StakingServiceBase which implements all the staking logic.

The issue here is that, Both CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol are CVX staking contracts which can not be added in isCvxStaking mapping.

isCvxStaking is actually determines the given address is an cvx staking contract and therefore can trigger withdrawals from the CvxBlackHole.

    mapping(address => bool) public isCvxStaking; /// contractAddress => bool

To insert the CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol contracts in isCvxStaking, there is no setter function available in contract and due to this lack of functionality the contracts can not be verified as CVX staking contracts which are further used in validation for contracts like CvxBlackHole.

For example:

In Convergence control Tower, for sdt integration the following functions are added via isSdtStaking.

Convergence control Tower address- 0xB0Afc8363b8F36E0ccE5D54251e20720FfaeaeE7

    ISdtStakingPositionService[] public sdAndLpAssetStaking;

    /// @dev Determines if an address is a sdt staking contract and therefore can trigger withdrawals from the SdtBlackHole.
    mapping(address => bool) public isSdtStaking; /// contractAddress => bool

    /**
     * @dev Determines if an address is a staking contract and therefore is able to mint CVG with `mintStaking` function.
     *      Only these addresses can be set up as gauge.
     */
    mapping(address => bool) public isStakingContract; /// contractAddress => bool

    /**
     * @notice Toggle a staking contract, can only be called by the owner.
     * @param contractAddress address of the staking contract to toggle
     */
    function toggleStakingContract(address contractAddress) external onlyOwner {
        isStakingContract[contractAddress] = !isStakingContract[contractAddress];
    }

    /**
     * @notice Insert a new sd/LP-asset staking contract, can only be called by the clone factory.
     * @param _sdtStakingClone address of the new staking contract
     */
    function insertNewSdtStaking(ISdtStakingPositionService _sdtStakingClone) external {
        require(msg.sender == cloneFactory, "CLONE_FACTORY");
        sdAndLpAssetStaking.push(_sdtStakingClone);
        isSdtStaking[address(_sdtStakingClone)] = true;
        isStakingContract[address(_sdtStakingClone)] = true;
    }

It means that for any sdt staking contract, the clone factory owner can call the insertNewSdtStaking() function to add the sdt staking address as a part of isSdtStaking and isStakingContract mappings so that these addresses can be easily verified as sdt staking addressess across Convergence contracts.

A similar functionality is required for CVX staking contracts.

Impact

CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol can not be added as CVX staking contract so these can contract can not be validated as CVX staking contract as used across Convergence contracts. This would break the intended protocol design and void the basic validation of CVX staking contracts and would left the contract without use of isCvxStaking mapping across convergence contracts. Contracts like CvxBlackHole expects the caller validation as CVX staking contract so it would revert due to no setter function can add both CVX staking contracts and store in isCvxStaking.

The access control where isCvxStaking as msg.sender is expected will always revert and the functions using it would be permanently unusable by users.

Recommendation to fix\ Please refer the cvgControlTower implementation sdt integration for add/insert of sdt staking contracts and related toggle functionalities.

Similarly, add the functionalities in contract for CVX staking contracts so it can be part of isCvxStaking and convergence contract function expecting isCvxStaking as msg.sender validation should not revert or unresponsive for both CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol

PlamenTSV commented 6 months ago

Unused variable, control tower is queried directly

0xRizwan commented 6 months ago

@0xR3vert Can you please check this issue?

There are 2 major issues if this issue is not fixed.

**ISSUE 01**: This issue which is already explained in detailed with below recommendation to fix it.

For sdt staking, insertNewSdtStaking() function was present but for convex staking, there is no such functionality present in contract for convex staking.

Since isCvxStaking is a part of CvxStakingPositionManager.sol contract.

    /// @dev Determines if an address is a cvx staking contract and therefore can trigger withdrawals from the CvxBlackHole.
    mapping(address => bool) public isCvxStaking; /// contractAddress => bool

I recommend moving isCvxStaking to CvgControlTowerV2.sol contract and consider below recommendation to fix this issue:

// SPDX-License-Identifier: MIT
/**
 _____
/  __ \
| /  \/ ___  _ ____   _____ _ __ __ _  ___ _ __   ___ ___
| |    / _ \| '_ \ \ / / _ \ '__/ _` |/ _ \ '_ \ / __/ _ \
| \__/\ (_) | | | \ V /  __/ | | (_| |  __/ | | | (_|  __/
\____/\___/|_| |_|\_/ \___|_|  \__, |\___|_| |_|\___\___|
                                 __/ |
                                |___/
 */
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
import "./interfaces/ICvgControlTower.sol";
import "./interfaces/Convex/ICvxStakingPositionService.sol";
import "./interfaces/Convex/ICvxRewardDistributor.sol";
import "./interfaces/Convex/ICvxConvergenceLocker.sol";
import "./interfaces/Convex/ICvxStakingPositionManager.sol";

/// @title Cvg-Finance - CvgControlTowerV2
/// @notice Heart of Convergence V2
/// @dev Acts as a dictionary of addresses for other contracts
contract CvgControlTowerV2 is Ownable2StepUpgradeable {
    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                  GLOBAL PARAMETERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    uint128 public cvgCycle;
    ICvgOracle public cvgOracle;
    ICvg public cvgToken;
    ICvgRewards public cvgRewards;
    address public cloneFactory;

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                  BONDS GLOBAL PARAMETERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    IBondCalculator public bondCalculator;
    IBondPositionManager public bondPositionManager;
    IBondDepository public bondDepository;

    /// @dev Determines if an address is a bond contract and therefore is able to mint CVG with `mintBond` function.
    mapping(address => bool) public isBond; /// contractAddress => bool

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                  STAKING GLOBAL PARAMETERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */
    ISdtStakingPositionManager public sdtStakingPositionManager;
    ISdtStakingPositionService public cvgSdtStaking;
    ISdtBlackHole public sdtBlackHole;
    ISdtBuffer public cvgSdtBuffer;
    IERC20Mintable public cvgSDT;
    IERC20 public sdt;
    address public poolCvgSdt;
    address public sdtFeeCollector;
    address public sdtStakingViewer;
    ISdtStakingPositionService[] public sdAndLpAssetStaking;
    address public sdtRewardDistributor;

    /// @dev Determines if an address is a sdt staking contract and therefore can trigger withdrawals from the SdtBlackHole.
    mapping(address => bool) public isSdtStaking; /// contractAddress => bool

    /**
     * @dev Determines if an address is a staking contract and therefore is able to mint CVG with `mintStaking` function.
     *      Only these addresses can be set up as gauge.
     */
    mapping(address => bool) public isStakingContract; /// contractAddress => bool
    address public sdtUtilities;

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                 LOCKING PARAMETERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */
    ILockingPositionManager public lockingPositionManager;
    ILockingPositionService public lockingPositionService;
    ILockingPositionDelegate public lockingPositionDelegate;
    IVotingPowerEscrow public votingPowerEscrow;
    IGaugeController public gaugeController;
    IYsDistributor public ysDistributor;

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                 WALLETS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */
    address public treasuryPod;
    address public treasuryPdd;
    address public treasuryDao;
    address public treasuryAirdrop;
    address public treasuryTeam;
    address public veSdtMultisig;
    address public treasuryPartners;

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                 VESTING
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    IVestingCvg public vestingCvg;
    address public ibo;

    event NewCycle(uint256 cvgCycleId);

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                        CONVEX PART  
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    address public cvxConvergenceMultisig;
    ICvxStakingPositionManager public cvxStakingPositionManager;
    ICvxRewardDistributor public cvxRewardDistributor;
    IERC20 public cvx;
    ICvxConvergenceLocker public cvxConvergenceLocker;

    ICvxStakingPositionService[] public cvxAssetStaking;
    ICvxStakingPositionService public cvgCvxStaking;

+    /// @dev Determines if an address is a cvx staking contract and therefore can trigger withdrawals from the CvxBlackHole.
+    mapping(address => bool) public isCvxStaking; /// contractAddress => bool

    function toggleBond(address _bondDepository) external onlyOwner {
        isBond[_bondDepository] = !isBond[_bondDepository];
    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                      CLONE FACTORY FUNCTIONS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    /**
     * @notice Toggle a staking contract, can only be called by the owner.
     * @param contractAddress address of the staking contract to toggle
     */
    function toggleStakingContract(address contractAddress) external onlyOwner {
        isStakingContract[contractAddress] = !isStakingContract[contractAddress];
    }

    /**
     * @notice Insert a new sd/LP-asset staking contract, can only be called by the clone factory.
     * @param _sdtStakingClone address of the new staking contract
     */
    function insertNewSdtStaking(ISdtStakingPositionService _sdtStakingClone) external {
        require(msg.sender == cloneFactory, "CLONE_FACTORY");
        sdAndLpAssetStaking.push(_sdtStakingClone);
        isSdtStaking[address(_sdtStakingClone)] = true;
        isStakingContract[address(_sdtStakingClone)] = true;
    }

+    /**
+     * @notice Insert a new CV/LP-asset staking contract, can only be called by the clone factory.
+     * @param _cvxStakingClone address of the new convex staking contract
+     */

+    function insertNewCVXStaking(ICvxStakingPositionService _cvxStakingClone) external {
+        require(msg.sender == cloneFactory, "CLONE_FACTORY");
+        cvxAssetStaking.push(_cvxStakingClone);
+        isCvxStaking[address(_cvxStakingClone)] = true;
+        isStakingContract[address(_cvxStakingClone)] = true;
+    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                      STAKING GETTERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    struct Staking {
        address stakingContract;
        string stakingName;
    }

    /**
     * @notice Get sdt staking contracts with pagination.
     * @param _cursorStart cursor position (starting position)
     * @param _lengthDesired desired length of array
     * @return array of sdt staking contracts with related global data
     */
    function getSdtStakings(uint256 _cursorStart, uint256 _lengthDesired) external view returns (Staking[] memory) {
        uint256 _totalArrayLength = sdAndLpAssetStaking.length;

        if (_cursorStart + _lengthDesired > _totalArrayLength) {
            _lengthDesired = _totalArrayLength - _cursorStart;
        }
        /// @dev Prevent to reach an index that doesn't exist in the array
        Staking[] memory array = new Staking[](_lengthDesired);
        for (uint256 i = _cursorStart; i < _cursorStart + _lengthDesired; ) {
            ISdtStakingPositionService sdtStakingService = sdAndLpAssetStaking[i];
            array[i - _cursorStart] = Staking({
                stakingContract: address(sdtStakingService),
                stakingName: sdtStakingService.stakingAsset().name()
            });
            unchecked {
                ++i;
            }
        }

        return array;
    }

    /**
     * @notice Get cvx staking contracts with pagination.
     * @param _cursorStart cursor position (starting position)
     * @param _lengthDesired desired length of array
     * @return array of cvx staking contracts with related global data
     */
    function getCvxStakings(uint256 _cursorStart, uint256 _lengthDesired) external view returns (Staking[] memory) {
        uint256 _totalArrayLength = cvxAssetStaking.length;

        if (_cursorStart + _lengthDesired > _totalArrayLength) {
            _lengthDesired = _totalArrayLength - _cursorStart;
        }

        /// @dev Prevent to reach an index that doesn't exist in the array
        Staking[] memory array = new Staking[](_lengthDesired);
        for (uint256 i = _cursorStart; i < _cursorStart + _lengthDesired; ) {
            ICvxStakingPositionService cvxStakingService = cvxAssetStaking[i];
            array[i - _cursorStart] = Staking({
                stakingContract: address(cvxStakingService),
                stakingName: cvxStakingService.stakingAsset().name()
            });

            unchecked {
                ++i;
            }
        }

        return array;
    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                          SETTERS
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */
    function setOracle(ICvgOracle newCvgOracle) external onlyOwner {
        cvgOracle = newCvgOracle;
    }

    function setTreasuryPod(address newTreasuryPodMultisig) external onlyOwner {
        treasuryPod = newTreasuryPodMultisig;
    }

    function setTreasuryPdd(address newTreasuryPddMultisig) external onlyOwner {
        treasuryPdd = newTreasuryPddMultisig;
    }

    function setTreasuryDao(address newTreasuryDao) external onlyOwner {
        treasuryDao = newTreasuryDao;
    }

    function setTreasuryAirdrop(address newTreasuryAirdrop) external onlyOwner {
        treasuryAirdrop = newTreasuryAirdrop;
    }

    function setTreasuryTeam(address newTreasuryTeam) external onlyOwner {
        treasuryTeam = newTreasuryTeam;
    }

    function setTreasuryPartners(address newtreasuryPartners) external onlyOwner {
        treasuryPartners = newtreasuryPartners;
    }

    function setBondDepository(IBondDepository newBondDepository) external onlyOwner {
        bondDepository = newBondDepository;
    }

    function setBondCalculator(IBondCalculator newBondCalculator) external onlyOwner {
        bondCalculator = newBondCalculator;
    }

    function setCvgRewards(ICvgRewards newCvgRewards) external onlyOwner {
        cvgRewards = newCvgRewards;
    }

    function setVeCVG(IVotingPowerEscrow newVotingPowerEscrow) external onlyOwner {
        votingPowerEscrow = newVotingPowerEscrow;
    }

    function setGaugeController(IGaugeController newGaugeController) external onlyOwner {
        gaugeController = newGaugeController;
    }

    function setCloneFactory(address newCloneFactory) external onlyOwner {
        cloneFactory = newCloneFactory;
    }

    function setLockingPositionManager(ILockingPositionManager newLockingPositionManager) external onlyOwner {
        lockingPositionManager = newLockingPositionManager;
    }

    function setLockingPositionService(ILockingPositionService newLockingPositionService) external onlyOwner {
        lockingPositionService = newLockingPositionService;
    }

    function setLockingPositionDelegate(ILockingPositionDelegate newLockingPositionDelegate) external onlyOwner {
        lockingPositionDelegate = newLockingPositionDelegate;
    }

    function setYsDistributor(IYsDistributor _ysDistributor) external onlyOwner {
        ysDistributor = _ysDistributor;
    }

    function setCvg(ICvg _cvgToken) external onlyOwner {
        cvgToken = _cvgToken;
    }

    function setBondPositionManager(IBondPositionManager _bondPositionManager) external onlyOwner {
        bondPositionManager = _bondPositionManager;
    }

    function setVestingCvg(IVestingCvg _vestingCvg) external onlyOwner {
        vestingCvg = _vestingCvg;
    }

    function setIbo(address _ibo) external onlyOwner {
        ibo = _ibo;
    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                        STAKE DAO 
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    function setSdtStakingPositionManager(ISdtStakingPositionManager _sdtStakingPositionManager) external onlyOwner {
        sdtStakingPositionManager = _sdtStakingPositionManager;
    }

    function setSdtStakingViewer(address _sdtStakingViewer) external onlyOwner {
        sdtStakingViewer = _sdtStakingViewer;
    }

    function setSdt(IERC20 _sdt) external onlyOwner {
        sdt = _sdt;
    }

    function setCvgSdt(IERC20Mintable _cvgSDT) external onlyOwner {
        cvgSDT = _cvgSDT;
    }

    function setCvgSdtStaking(ISdtStakingPositionService _cvgSdtStaking) external onlyOwner {
        cvgSdtStaking = _cvgSdtStaking;
    }

    function setVeSdtMultisig(address _veSdtMultisig) external onlyOwner {
        veSdtMultisig = _veSdtMultisig;
    }

    function setCvgSdtBuffer(ISdtBuffer _cvgSdtBuffer) external onlyOwner {
        cvgSdtBuffer = _cvgSdtBuffer;
    }

    function setPoolCvgSdt(address _poolCvgSDT) external onlyOwner {
        poolCvgSdt = _poolCvgSDT;
    }

    function setSdtBlackHole(ISdtBlackHole _sdtBlackHole) external onlyOwner {
        sdtBlackHole = _sdtBlackHole;
    }

    function setSdtFeeCollector(address _sdtFeeCollector) external onlyOwner {
        sdtFeeCollector = _sdtFeeCollector;
    }

    function setSdtUtilities(address _sdtUtilities) external onlyOwner {
        sdtUtilities = _sdtUtilities;
    }

    function setSdtRewardDistributor(address _sdtRewardDistributor) external onlyOwner {
        sdtRewardDistributor = _sdtRewardDistributor;
    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                        CONVEX
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */
    function setCvxConvergenceMultisig(address _cvxConvergenceMultisig) external onlyOwner {
        cvxConvergenceMultisig = _cvxConvergenceMultisig;
    }

    function setCvxStakingPositionManager(ICvxStakingPositionManager _cvxStakingPositionManager) external onlyOwner {
        cvxStakingPositionManager = _cvxStakingPositionManager;
    }

    function setCvx(IERC20 _cvx) external onlyOwner {
        cvx = _cvx;
    }

    function setCvxConvergenceLocker(ICvxConvergenceLocker _cvxConvergenceLocker) external onlyOwner {
        cvxConvergenceLocker = _cvxConvergenceLocker;
    }

    function setCvgCvxStaking(ICvxStakingPositionService _cvgCvxStaking) external onlyOwner {
        cvgCvxStaking = _cvgCvxStaking;
    }

    function setCvxRewardDistributor(ICvxRewardDistributor _cvxRewardDistributor) external onlyOwner {
        cvxRewardDistributor = _cvxRewardDistributor;
    }

    /* =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-=
                      CYCLE MANAGEMENT
    =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--=-=-=-= */

    /**
     * @notice Update Cvg cycle.
     * @dev Updates ysCvg total supply too, can only be called by CvgRewards.
     */
    function updateCvgCycle() external {
        require(msg.sender == address(cvgRewards), "NOT_CVG_REWARDS");
        emit NewCycle(++cvgCycle);
    }
}

0xRizwan commented 6 months ago

@0xR3vert

**ISSUE 02**:

If ISSUE 01 is not fixed then the below issue will occur.

In CvgControlTowerV2.sol, To get the CVX staking, the following function has been implemented:

    function getCvxStakings(uint256 _cursorStart, uint256 _lengthDesired) external view returns (Staking[] memory) {
@>      uint256 _totalArrayLength = cvxAssetStaking.length;

        if (_cursorStart + _lengthDesired > _totalArrayLength) {
            _lengthDesired = _totalArrayLength - _cursorStart;
        }

        /// @dev Prevent to reach an index that doesn't exist in the array
        Staking[] memory array = new Staking[](_lengthDesired);
        for (uint256 i = _cursorStart; i < _cursorStart + _lengthDesired; ) {
            ICvxStakingPositionService cvxStakingService = cvxAssetStaking[i];
            array[i - _cursorStart] = Staking({
                stakingContract: address(cvxStakingService),
                stakingName: cvxStakingService.stakingAsset().name()
            });

            unchecked {
                ++i;
            }
        }

        return array;
    }

getCvxStakings() function calculates the array length as :

      uint256 _totalArrayLength = cvxAssetStaking.length;

This considers the cvxAssetStaking length which is an array as given below:

     ICvxStakingPositionService[] public cvxAssetStaking;

The issue is that, since there is no insertNewCVXStaking() function implemented in contract so cvx staking contracts can not be pushed to cvxAssetStaking array.

In cvxAssetStaking() function, _totalArrayLength will always be 0 since the CVX staking is not a part of cvxAssetStaking.

Therefore, getCvxStakings() will not be able to return the CVX stakings due to the issue described above.

To mitigate both ISSUE 01 and ISSUE 02, the above given recommendation must be implemented.

hats-finance / Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb

Convex staking contracts can not be stored in `isCvxStaking` mapping due to lack of setter functionality #59