Lines of code

Vulnerability details

Impact

The ConvexStakingWrapper.sol implementation makes several modifications to the original design. One of the key changes is the way rewards are distributed to stakers. A new ConcurRewardPool.sol contract is used to store rewards, allowing users to claim their delegated amounts.

There seems to be a key component of the _calcRewardIntegral function that is missing from the modified implementation, namely, the way bal is updated upon transferring tokens from the ConvexStakingWrapper.sol contract to the ConcurRewardPool.sol contract. As a result, when reward.remaining is set to bal, following calls to distribute rewards will revert due to an underflow in d_reward = bal - reward.remaining. This will lead certain token loss as deposits and withdrawals will not succeed until sufficient rewards have been distributed to the ConvexStakingWrapper.sol contract, thus covering the shortfall.

Proof of Concept

Let’s consider the following scenario:

• There is a single convex pool distributing only cvx tokens.
• Alice deposits LP tokens into this pool and is the sole staker.
• This pool earns 100 cvx tokens which will be distributed to Alice the next time she checkpoints her stake.
• Alice then deposits the 0 amount to execute the _calcRewardIntegral function on her pool. This function will calculate d_reward as bal - reward.remaining which is effectively the change in contract balance. Therefore, d_reward will be 100 - 0 and this amount is transferred to the ConcurRewardPool.sol contract.
• At the end of the _calcRewardIntegral function, because bal != reward.remaining, reward.remaining is set to 100 (i.e. the bal).
• The pool earns 50 cvx tokens, and Alice once again deposits the 0 amount to mimic the checkpointing behaviour again.
• However, this time the _calcRewardIntegral will revert as d_reward = bal - reward.remaining will be equal to 50 - 100. Therefore, the function will continue to revert until 100 cvx tokens have been earned, and even after this is reached, those reward tokens are forever lost in the ConvexStakingWrapper.sol contract.

<https://github.com/code-423n4/2022-02-concur/blob/shelter-client/contracts/ConvexStakingWrapper.sol#L216-L259&gt;

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

Tools Used

Manual code review.
Confirmation with Taek.

Recommended Mitigation Steps

Consider ensuring that bal is decremented each time rewards are sent out of the ConvexStakingWrapper.sol contract.

The original implementation is shown here and outlines how bal should be updated.

The text was updated successfully, but these errors were encountered:

All reactions