code423n4 commented 2 years ago

Handle

kirk-baird

Vulnerability details

Impact

Incorrect accounting in _calcRewardIntegral() causes the rewards to be locked and will create an overflow that reverts for future calls.

The impact is a significant loss in each rewards token in terms of value and _calcRewardIntegral() cannot be called again until we have transferred sufficient balance of each rewards token to ConvexStakingWrapper to account for the rewards already claimed.

Proof of Concept

_calcRewardIntegral()

    function _calcRewardIntegral(
        uint256 _pid,
        uint256 _index,
        address _account,
        uint256 _balance,
        uint256 _supply
    ) internal {
        RewardType memory reward = rewards[_pid][_index];

        //get difference in balance and remaining rewards
        //getReward is unguarded so we use remaining to keep track of how much was actually claimed
        uint256 bal = IERC20(reward.token).balanceOf(address(this));
        uint256 d_reward = bal - reward.remaining;
        // send 20 % of cvx / crv reward to treasury
        if (reward.token == cvx || reward.token == crv) {
            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = (d_reward * 4) / 5;
        }
        IERC20(reward.token).transfer(address(claimContract), d_reward);

        if (_supply > 0 && d_reward > 0) {
            reward.integral =
                reward.integral +
                uint128((d_reward * 1e20) / _supply);
        }

        //update user integrals
        uint256 userI = userReward[_pid][_index][_account].integral;
        if (userI < reward.integral) {
            userReward[_pid][_index][_account].integral = reward.integral;
            claimContract.pushReward(
                _account,
                reward.token,
                (_balance * (reward.integral - userI)) / 1e20
            );
        }

        //update remaining reward here since balance could have changed if claiming
        if (bal != reward.remaining) {
            reward.remaining = uint128(bal);
        }

        rewards[_pid][_index] = reward;
    }

The core of this bug stems from setting the remaining rewards to the initial contract balance i.e.reward.remaining = uint128(bal); even after we have transferred the rewards to the claimContract.

Example

Say we have reward.token = tokenA, tokenA.balanceOf(ConvexStakingWrapper) = 100 and rewards[pid][tokena]remaining = 0 then we do an event that calls _calcRewardIntegral():

bal = 100
d_reward = 100 - 0 = 100
IERC20(reward.token).transfer(address(claimContract), d_reward); occurs making the tokenA.balanceOf(ConvexStakingWrapper) = 0
reward.remaining = bal = 100

Now say the contract accrues 50 of tokenA as rewards so tokenA.balanceOf(ConvexStakingWrapper) = 50 and an event happens so do _calcRewardIntegral():

bal = 50
uint256 d_reward = bal - reward.remaining; is 50 - 100 = -50 this is an overflow and reverts

Thus, we are unable to make future calls to this contract with this pid until the balance of tokenA is greater than 100.

Recommended Mitigation Steps

Consider, removing reward.remaining entirely. This will prevent future calls from overflowing when there are not sufficient rewards. However, this will no longer track the individual reward amounts for each (pid, index) pair.

GalloDaSballo commented 2 years ago

The warden has identified a way for reward tokens to consistently be stuck in the contract. Anytime the balance of tokens in the contract is less than the balance of the previous _calcRewardIntegral the operation uint256 d_reward = bal - reward.remaining; will underflow causing a revert.

This consistently will prevent users from claiming rewards until the contracts balance is greater than reward.remaining

With the information that I have I believe this will consistently happen after the first claim as the new balance of the contract will always be lower than the reward.remaining (the previous balance of the contract)

Because the issue is consistent, and effectively breaks the contract, I believe High Severity to be appropriate