Incorrect access control on CvxStakingPositionManager.mint() allows unauthorized address to mint position NFTs

[hats-bug-reporter[bot]]

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

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. Any user can own a staking position by depositing ETH, CVX or cvgCVX tokens depending on both contracts.

Staking positions are represented by a unique NFT. When the user deposits these listed tokens, the contracts issues a unique position NFTs to depositor i.e msg.sender

For minting the staking position NFT, both contracts calls CvxStakingPositionManager.mint() function in their deposit functions.

This can be checked here in CvgCvxStakingPositionService.sol

226        else {
227            _tokenId = _cvxStakingPositionManager.mint(msg.sender);
228        }

and in CvxAssetStakingService.sol, it can be checked here

209        else {
210            _tokenId = stakingPositionManager.mint(msg.sender);
211        }

Now, Let's see the issue:

The issue is in stakingPositionManager.mint() which is implemented as :

    /** @dev This function is only callable by a Convex StakingContract.
     *       Mints an NFT to the user and link the tokenId with the corresponding StakingPositionService.
     *  @param account Receiver of the minted NFT
     */
    function mint(address account) external returns (uint256) {
        /// @dev Verify that caller is a StakingContract
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        /// @dev Increments the nextId
        uint256 tokenId = nextId++;

        /// @dev Link the minted token with the StakingService
        stakingPerTokenId[tokenId] = msg.sender;
        /// @dev Mints the token
        _mint(account, tokenId);

        return tokenId;
    }

mint() function check whether the function caller is staking contract as whitelisted by cvgControlTower contract.

However, This is incorrect mint() function access control as the function Natspec states:

This function is only callable by a Convex StakingContract

It means that only CvxAssetStakingService.sol and CvgCvxStakingPositionService.sol should be allowed to call the stakingPositionManager.mint() function.

The current implementation deviates from Natspec design and allows unauthorized address to mint the position NFTs.

After digging bit more to understand where exactly cvgControlTower.isStakingContract() is used in convergence contracts. cvgControlTowerV2 specifically states:

 /**
     * @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

It means that use of isStakingContract is relevant to where CVG is being minted, However, in current implementation of above both contracts isStakingContract is being used to check the position NFT minting.

This is actually a wrong access check which is confirmed by both function Natspec and isStakingContract specific use in Convergence contracts.

Instead of isStakingContract, the CvxStakingPositionManager.sol has used isCvxStaking to determine if the function caller is CVX staking contract or not. This can be checked here

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

Therefore, use isCvxStaking instead of isStakingContract to check the function caller to mint position NFTs.

Impact Unauthorized address is being allowed to mint position NFTs. This violates the mint() function Natspec design as mentioned for access control. This would break the potential protocol design functionality as explained above. The mint() has used incorrect isStakingContract instead of isCvxStaking, thereby voids the whole validation of cvx staking contract caller verifications.

Recommendation to fix As mentioned in stakingPositionManager.mint() function Natspec, mint() should only be callable by Convex StakingContract.

Consider below changes:

    function mint(address account) external returns (uint256) {
-        /// @dev Verify that caller is a StakingContract
-        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");

+        /// @dev Verify that caller is a CVX StakingContract
+        require(isCvxStaking(msg.sender), "NOT_CVX STAKING");

        /// @dev Increments the nextId
        uint256 tokenId = nextId++;

        /// @dev Link the minted token with the StakingService
        stakingPerTokenId[tokenId] = msg.sender;
        /// @dev Mints the token
        _mint(account, tokenId);

        return tokenId;
    }

[0xRizwan]

Please ignore this issue.

[PlamenTSV]

34