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): 0xe2c3e76cf60858a7fc64bfcb3c0ec2dfeaadf878e041d6504013d4178e6e389c 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. When the

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.

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.

Recommendation to fix\ Per the stakingPositionManager.mint() function Natspec, only allow convex staking contract to access mint function thereby minting position NFTs.

The use of isStakingContract in stakingPositionManager.mint() is not correct as explained above.

For example understanding(Actual implementation could differ):

+   address public CvxAssetStakingService;
+  address public CvgCvxStakingPositionService;

    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 convex StakingContract only 
+      require( msg.sender == CvxAssetStakingService | | msg.sender == CvgCvxStakingPositionService);

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

Correction:

For example understanding(Actual implementation could differ):

+   address public CvxAssetStakingService;
+  address public CvgCvxStakingPositionService;

    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 convex StakingContract only 
+      require( msg.sender == CvxAssetStakingService || msg.sender == CvgCvxStakingPositionService);

        /// @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;
    }

[PlamenTSV]

There could be multiple services and these Staking Services are exactly staking contracts that the tower will keep track of. I do not really see the issue here, unauthorized addresses by the tower would be unable to mint NFT, since minting is only done on deposits to non-existing positions in order to create them.