Users will not be able to claim their their CVX rewards under certain conditions

[hats-bug-reporter[bot]]

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

Description: Description\

Two conditions are required for this to occur,

1. User must still have claimable cvg;
2. MAX_STAKING must have been reached;

Attack Scenario\

When owners of a staking position, want to claim the cvg and convex rewards, the call the claimCvgCvxRewards function in the StakingService contract. The function calculates the cvg and cvx rewards entitled to the staking position. The function then calls the claimCvgCvxSimple function in cvxRewardsDistributor.sol

    function claimCvgCvxRewards(
        uint256 _tokenId,
        uint256 _minCvgCvxAmountOut,
        bool _isConvert
    ) external checkCompliance(_tokenId) {
        (uint256 cvgClaimable, ICommonStruct.TokenAmount[] memory tokenAmounts) = _claimCvgCvxRewards(_tokenId);

        cvxRewardDistributor.claimCvgCvxSimple(msg.sender, cvgClaimable, tokenAmounts, _minCvgCvxAmountOut, _isConvert);

        emit ClaimCvgCvxMultiple(_tokenId, msg.sender);
    }

Here, the internal _withdrawRewards function is called.

    function claimCvgCvxSimple(
        address receiver,
        uint256 totalCvgClaimable,
        ICommonStruct.TokenAmount[] memory totalCvxRewardsClaimable,
        uint256 minCvgCvxAmountOut,
        bool isConvert
    ) external {
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        _withdrawRewards(receiver, totalCvgClaimable, totalCvxRewardsClaimable, minCvgCvxAmountOut, isConvert);
    }

The _withdrawRewards function is meant to mint the accumulated cvg rewards and claim convex rewards, and if there's no cvg rewards to claim, just claims convex rewards for the user. Now, if a user still has totalCvgClaimable, the mintStaking function is called in the CVG contract.

    function _withdrawRewards(
        address receiver,
        uint256 totalCvgClaimable,
        ICommonStruct.TokenAmount[] memory totalCvxRewardsClaimable,
        uint256 minCvgCvxAmountOut,
        bool isConvert
    ) internal {
        /// @dev Mints accumulated CVG and claim Convex rewards
        if (totalCvgClaimable > 0) {
            CVG.mintStaking(receiver, totalCvgClaimable);
        }

        for (uint256 i; i < totalCvxRewardsClaimable.length; ) {
            uint256 rewardAmount = totalCvxRewardsClaimable[i].amount;
        ...
        }
    }

The mintStaking function check the amount minted is not more than MAX_STAKING parameter, upon which it reverts if it is, causing the entire function chain to revert, and users to not be able to claim cvg, nor claim convex rewards.

    function mintStaking(address account, uint256 amount) external {
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        uint256 _mintedStaking = mintedStaking;
        require(_mintedStaking < MAX_STAKING, "MAX_SUPPLY_STAKING");

        /// @dev ensure every tokens will be minted from staking
        uint256 newMintedStaking = _mintedStaking + amount;
        if (newMintedStaking > MAX_STAKING) {
            newMintedStaking = MAX_STAKING;
            amount = MAX_STAKING - _mintedStaking;
        }

        mintedStaking = newMintedStaking;
        _mint(account, amount);
    }

This can also be weaponized, as it introduces a race condition, when MAX_STAKING has nearly been reached, increasing the amount of frontrunning attacks and DOS as users would be incentivized to mint as much cvg tokens as they can before the limit is reached.

Attachments

Revised Code File (Optional)

Best way to fix it is to remove the check for if MAX_STAKING has been reached. The function logic will handle normally if _mintedStaking == MAX_STAKING. The newMintedStaking will be more than MAX_STAKING, which the if condition will be forced to cap, and then set the amount to 0. That way, 0 amount will be CVG will minted to claimers, protecting the codebase from bypassing MAX_STAKING and also making sure users can claim the other convex rewards.

    function mintStaking(address account, uint256 amount) external {
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        uint256 _mintedStaking = mintedStaking;
      //  require(_mintedStaking < MAX_STAKING, "MAX_SUPPLY_STAKING");

        /// @dev ensure every tokens will be minted from staking
        uint256 newMintedStaking = _mintedStaking + amount;
        if (newMintedStaking > MAX_STAKING) {
            newMintedStaking = MAX_STAKING;
            amount = MAX_STAKING - _mintedStaking;
        }

        mintedStaking = newMintedStaking;
        _mint(account, amount);
    }

[PlamenTSV]

The total supply of CVG will be 150 million. The whitepaper specifies 40% of that will be allocated to staking. The contract defines: uint256 public constant MAX_STAKING = 60_000_000 * 10 ** 18 4/10 * 150 mill = 60 mill with 1e18 decimals for the CVG token. The code is correct. Check out point 5.2, page 6 of the whitepaper: https://ipfs.io/ipfs/QmR3VBUjh1VKAEF3LVyvZcLnznEuqUc2kRu3hrzqoESHmo/2022-11-15-wp.pdf

[bG9zZXIvZmFpbHVyZQ]

Hi @PlamenTSV

What the issue is referencing is not that the amount allocated to staking is incorrect, what is being referenced is that users will not be able to claim any of their convex rewards if they still have cvgClaimable > 0 while MAX_STAKING limit has been met.

This check in the mintStaking function will continually revert the claimCvgCvxRewards function chain, as the user still has unclaimed CVG, but CVG staking supply is now over the MAX_STAKING limit.

        require(_mintedStaking < MAX_STAKING, "MAX_SUPPLY_STAKING");

The issue is not that users should be able to claim more than the required 60_000_000 tokens, on the contrary, its that once this limit has been met, users who still have unclaimed CVG will not be able to claim their convex rewards because of the above check.

Notice that, if a user had unclaimed CVG which is less than newMintedStaking - MAX_STAKING, the difference is minted to them (regardless of their initial unclaimed CVG). That is the idea the recommendation is going for. Rather than reverting when MAX_STAKING has been reached, the function will simply mint 0 tokens to the user. That way the claimCvgCvxRewards function chain will not revert.

[PlamenTSV]

8 years is the period it would take to emit all of the tokens for staking for the limit to even be reachable. You are right it seems that the logic is off, I wrongly gave it the invalid label, but that wide timeframe really hits the likelihood. Will give it both labels and wait for the sponsor to see.

[bG9zZXIvZmFpbHVyZQ]

Yeah, you're right about the time frame, although the potential of a permanent DOS at that time is a high risk scenario. Given though the needed condition that users has to still have claimable CVG, and the time period, the likelihood is pretty medium/low, I'd conclude that it fits a medium severity issue.

[0xR3vert]

This issue is Out Of Scope as it concern the Cvg.sol contract. In conclusion this issue is Invalid.