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

0 stars 1 forks source link

Missing checks in `_rewardTokensConfigs.processorFees` & `_rewardTokensConfigs.podFees` in `CvxAssetStakerBuffer::setRewardTokensConfig` will cause `CvxAssetStakerBuffer::pullRewards`uncallable. #71

Open hats-bug-reporter[bot] opened 6 months ago

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

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x44cf3fdf0d7f9f6b7dbbc4cb49886fbfcd7bb352275eb0d966c0b4e00b814a1c Severity: low

Description: Description\

The function CvxAssetStakerBuffer::setRewardTokensConfig is missing value checks on _rewardTokensConfigs.processorFees & _rewardTokensConfigs.podFees parameters.

If the owner set these two value such that _rewardTokensConfigs.podFees + _rewardTokensConfigs.processorFees > DENOMINATOR, the following line will revert due to underflow:

CvxAssetStakerBuffer::pullRewards Line 167:

            uint256 amountToStakers = balance - podFees - processorFees;

Attack Scenario\

The function CvxAssetStakerBuffer::pullRewards will process rewards from Convex to stakers for the previous cycle. Owner can either through accidental or intentional setting wrong values for _rewardTokensConfigs.processorFees & _rewardTokensConfigs.podFees values to cause the pullRewards function uncallable. As a result, users' rewards will be frozen temporary or permanently.

Attachments

NA

  1. Proof of Concept (PoC) File

In CvxAssetStakerBuffer::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;

Noticed that when_rewardTokensConfig.podFees + _rewardTokensConfig.processorFees > DENOMINATOR, it will result in podFees + processorFees > balance, therefore resulting in revert due to underflow in line 167.

Setting this issue as Low Severity as CvxAssetStakerBuffer::setRewardTokensConfig function is protected however the impact can be High as it cause denial of service in CvxAssetStakerBuffer::pullRewardsfunction.

  1. Revised Code File (Optional)

Consider making the following changes in CvxAssetStakerBuffer::setRewardTokensConfig:

function setRewardTokensConfig(
        ICvxAssetStakerBuffer.CvxRewardConfig[] calldata _rewardTokensConfigs
    ) external onlyOwner {
        delete rewardTokensConfigs;
        for (uint256 i; i < _rewardTokensConfigs.length; ) {
++.       require(_rewardTokensConfigs[i].processorFees + _rewardTokensConfigs[i].podFees <= DENOMINATOR, "Wrong Value!");
            rewardTokensConfigs.push(_rewardTokensConfigs[i]);
            unchecked {
                i++;
            }
        }
    }
PlamenTSV commented 6 months ago

Centralization risk, no value loss also since the owner can change it back immediately.

0xEricTee commented 6 months ago

Owner might be unaware of this in the first place so would like to hear insights from the sponsor.