CvxAssetStakerBuffer.sol#pullRewards() - If the `cvsAssetWrapper` is shutdown, `pullRewards` will revert every time and the rewards cannot be distributed to the `rewardReceiver`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: @dethSCA Submission hash (on-chain): 0x4e6fed0213efb6d2401ee1282c12fdcdb1a01b7e50c439a2d0717e0bcc9435ef Severity: medium

Description: Description\ The CvsAssetStakerBuffer uses a cvsAssetBuffer in order to stake and get rewards.

From the deploy scripts we can see some of the wrappers that will be used by the protocol.

For this example, I'll use the CVX_CRV_WRAPPER, but the issue persists in every other wrapper as well.

All wrappers have a variable called isShutdown and it can be set to true in the rare ocassion that a wrapper needs to be shutdown.

Once a wrapper is shutdown, it permanently disables stake.

//stake cvxcrv
    function stake(uint256 _amount, address _to) public {
        require(!isShutdown, "shutdown");
        ...

getReward can still be called, as to not block users getting their rewards.

This all looks good, but there is a problem with how Convergence uses pullRewards.

 function pullRewards(address processor) external returns (ICommonStruct.TokenAmount[] memory) {
        require(cvxAssetStakingService == msg.sender, "NOT_CVX_ASSET_STAKING_SERVICE");

        address treasuryPod = cvgControlTower.treasuryPod();

        address rewardReceiver = address(cvxRewardDistributor);
        uint256 rewardLength = rewardTokensConfigs.length;

        /// @dev Stakes all cvxAsset pending on the contract
        stakeAllCvxAsset();
        /// @dev Claim all rewards
        cvxAssetWrapper.getReward(address(this));

        ICommonStruct.TokenAmount[] memory rewardAssets = new ICommonStruct.TokenAmount[](rewardLength);
        uint256 counterDelete;
        for (uint256 i; i < rewardLength; ) {
            ICvxAssetStakerBuffer.CvxRewardConfig memory rewardConfig = rewardTokensConfigs[i];
            IERC20 token = rewardConfig.token;
            uint256 balance = token.balanceOf(address(this));

            uint256 processorFees = (balance * rewardConfig.processorFees) / DENOMINATOR;
            uint256 podFees = (balance * rewardConfig.podFees) / DENOMINATOR;
            uint256 amountToStakers = balance - podFees - processorFees;

            if (amountToStakers != 0) {
                token.safeTransfer(rewardReceiver, amountToStakers);
                rewardAssets[i - counterDelete] = ICommonStruct.TokenAmount({token: token, amount: amountToStakers});
            }

            if (processorFees != 0) {
                token.safeTransfer(processor, processorFees);
            }

            if (podFees != 0) {
                token.safeTransfer(treasuryPod, podFees);
            }

            if (balance == 0) {
                unchecked {
                    ++counterDelete;
                }
            }
            unchecked {
                ++i;
            }
        }

        // solhint-disable-next-line no-inline-assembly
        assembly {
            mstore(rewardAssets, sub(mload(rewardAssets), counterDelete))
        }
        return rewardAssets;
    }

You can see that in order to distribute the rewards received from the cvxAssetWrapper first we call stakeAllCvsAsset which will attempt to stake the entire cvxAsset balance of the contract.

function stakeAllCvxAsset() public {
        uint256 balanceCvxAsset = cvxAsset.balanceOf(address(this));
        if (balanceCvxAsset != 0) {
            if (stakingType == 0) {
                cvxAssetWrapper.stake(balanceCvxAsset, address(this));
            } else {
                cvxAssetWrapper.stake(balanceCvxAsset);
            }
        }
    }

And this is the problem, if the wrapper is shutdown, stake will always revert and because stakeAllCvsAsset is called inisde pullRewards, the rewards that the contract has accumulated can never be distributed to the rewardReceiver.

This check can very easily be forced to pass, as anyone can simply send 1 wei of cvsAsset to the contract and force it to call stake every time.

function stakeAllCvxAsset() public {
        uint256 balanceCvxAsset = cvxAsset.balanceOf(address(this));
        if (balanceCvxAsset != 0) {
        ...

This effectively bricks the entire rewarding logic of CvsAssetStakerBuffer and makes it so all users lose all their rewards, as they will never get distributed to the correct contract.

Considering this a Medium severity issue, as the likelihood of the wrapper getting shutdown is low, but the impact is high, as the rewards become completely unclaimable, because they aren't distributed.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

The best way to handle a wrapper shutting down is, to add a new parameter to pullRewards called bool stake, which if true will call stakeAllCvsAsset or if false will skip it.

Another way to fix this would be to add an only owner function which mimics pullRewards, but won't call stakeAllCvsAsset.

I also recommend adding a function that will transfer cvxAsset to a receiver in case the wrapper is shutdown, as currently if the wrapper is shutdown any cvxAsset in the contract will be lost forever.

[0xdeth]

I forgot to add, that getReward can be manually called for CvsAssetStakerBuffer and the rewards will be received in the contract, but they cannot be distributed and cannot be manually withdrawn, they will be lost forever in the contract.

    //claim
    function getReward(address _account) external {
        //claim directly in checkpoint logic to save a bit of gas
        _checkpointAndClaim([_account, _account]);
    }

[PlamenTSV]

Once the shutdown is over, another call to pullRewards would restake all rewards accordingly. Marking as medium for sponsor to say, since I could be wrong.

[0xdeth]

The shutdown is permanent, you can take a look at the contract.

[0xR3vert]

In the case of a shutdown wrapper, this simply means that the staking contract on our side has expired. If this happens, and a new wrapper replaces it, we'll do everything we can to upgrade/migrate the contracts in question. We're getting away from classic usage, and even if PullRewards is blocked - which, by the way, is not an obligatory transaction, as it can be triggered at any time - we're in a position to unblock the situation if need be. We therefore consider this issue to be invalid