code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L226

Vulnerability details

Impact

Calculating reward balance within convexConfig.baseRewardPool is not an easy task, mostly due to the design of BaseRewardPool contracts. In short, the convexConfig.baseRewardPool holds some reward itself, then it maintains a list of extraRewards, each of which also potentially holds rewards.

The implementation of balanceOfJPEG ignores rewards held in baseRewardPool directly, and directly proceeds to traverse baseRewardPool.extraRewards. It also does not check all extraRewards entries exhaustively, but prematurely stops upon reaching the first extraReward with JPEG as rewardToken. The lack of baseRewardPool is definitely a problem. As for extraRewards, since there is no guarantee that no two extraReward entries can't both map to the same rewardToken, it is also better to traverse all entries instead of stopping early.

The incomplete fetch of rewards leads to an imprecise, underestimated JPEG balance, and as the value propagates up to YVaultLPFarming, it undermines the calculate of JPEG yield. This causes users to receive less JPEG reward than they should.

Proof of Concept

In StrategyPUSDConvex, the calculation of JPEG balance only considers JPEG within strategy itself and part of the many reward of baseRewardPool

    function balanceOfJPEG() external view returns (uint256) {
        uint256 availableBalance = jpeg.balanceOf(address(this));

        IBaseRewardPool baseRewardPool = convexConfig.baseRewardPool;
        uint256 length = baseRewardPool.extraRewardsLength();
        for (uint256 i = 0; i < length; i++) {
            IBaseRewardPool extraReward = IBaseRewardPool(baseRewardPool.extraRewards(i));
            if (address(jpeg) == extraReward.rewardToken()) {
                availableBalance += extraReward.earned();
                //we found jpeg, no need to continue the loop
                break;
            }
        }

        return availableBalance;
    }

Compared with code used to retrieve rewards in Convex BaseRewardPool, it is easy to see Convex implementation is exhaustive, while strategy implementation is not.

    function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
        uint256 reward = earned(_account);
        if (reward > 0) {
            rewards[_account] = 0;
            rewardToken.safeTransfer(_account, reward);
            IDeposit(operator).rewardClaimed(pid, _account, reward);
            emit RewardPaid(_account, reward);
        }

        //also get rewards from linked rewards
        if(_claimExtras){
            for(uint i=0; i < extraRewards.length; i++){
                IRewards(extraRewards[i]).getReward(_account);
            }
        }
        return true;
    }

This imprecision in retrieving JPEG balance is reflected in YVaultLPFarming, where JPEG balance is used to update JPEG rewards distributed to vaultToken depositors.

    function _computeUpdate() internal view returns (uint256 newAccRewardsPerShare, uint256 currentBalance) {
        currentBalance = vault.balanceOfJPEG() + jpeg.balanceOf(address(this));
        uint256 newRewards = currentBalance - previousBalance;

        newAccRewardsPerShare = accRewardPerShare + newRewards * 1e36 / totalStaked;
    }

Overall, the bug results in vaultToken stakers to receive lesser JPEG token than they should have through rewards.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Exhaustively traverse extraReward array, and also remember to add reward from baseRewardPool itself to availableBalance.

Notice how we passed address(this) as the argument of earned. This is reported in a separate bug report, but for completeness, the entire fix is shown in both report entries.

    function balanceOfJPEG() external view returns (uint256) {
        uint256 availableBalance = jpeg.balanceOf(address(this));

        IBaseRewardPool baseRewardPool = convexConfig.baseRewardPool;
        availableBalance += baseRewardPool.earned(address(this));
        uint256 length = baseRewardPool.extraRewardsLength();
        for (uint256 i = 0; i < length; i++) {
            IBaseRewardPool extraReward = IBaseRewardPool(baseRewardPool.extraRewards(i));
            if (address(jpeg) == extraReward.rewardToken()) {
                availableBalance += extraReward.earned(address(this));
            }
        }

        return availableBalance;
    }

spaghettieth commented 2 years ago

baseRewardPool.earned returns the amount of CVX tokens earned, which is not what we are looking for in balanceOfJPEG. Your reasoning would be correct if balanceOfJPEG cared about rewards in other tokens, but it only cares about JPEG rewards which are claimed from a single extraReward pool.

dmvt commented 2 years ago

Invalid per sponsor's explanation.

code-423n4 / 2022-04-jpegd-findings