Royco

Boring Vault Chef

6.1 Calling claimRewards with an empty array allows reclaiming rewards

The claimRewards function accepts an array of rewardIds to claim. It calls the internal function _getEpochRangeForRewards to determine the epoch range for each reward and fetch the corresponding Reward structs. Additionally, this function updates the userToRewardBucketToClaimedRewards mapping to mark rewards as claimed by modifying the appropriate bit in the reward bucket.

function _getEpochRangeForRewards(uint256[] calldata rewardIds)
    internal
    returns (uint48 minEpoch, uint48 maxEpoch, Reward[] memory rewardsToClaim)
{
    // ...
    // Variables to cache reward claim data as a gas optimization.
    uint256 cachedRewardBucket;
    uint256 cachedClaimedRewards;

    for (uint256 i = 0; i < rewardsLength; ++i) {
        // ...
    }
    
    // @audit If rewardIds.length is 0, then it would unset all bits for 0th reward bucket.
    // Write back the final cache to persistent storage
    userToRewardBucketToClaimedRewards[msg.sender][cachedRewardBucket] = cachedClaimedRewards;
}

However, when the function is called with an empty rewardIds array, the loop is skipped, and the final line is executed. This results in all bits in the userToRewardBucketToClaimedRewards mapping for the 0th reward bucket being unset, effectively marking them as unclaimed. Consequently, users can reclaim rewards in the 0th reward bucket multiple times.

Recommendation

Consider updating the _getEpochRangeForRewards function to revert when the rewardIds array is empty.

6.2 Missing cache initialization leads to double claiming of rewards

The claimRewards function takes a rewardIds array as an argument and calls _getEpochRangeForRewards to get the epoch range for all rewards to claim and the corresponding Reward structs. A nested mapping userToRewardBucketToClaimedRewards is used to efficiently keep track of claimed rewards per user. Each reward bucket contains batches of 256 contiguous rewardIds. Each bit corresponds to whether the reward is claimed or not. To optimize gas usage, the _getEpochRangeForRewards function caches reward bucket data, minimizing the number of SLOAD and SSTORE operations when claiming rewards

function _getEpochRangeForRewards(uint256[] calldata rewardIds)
    internal
    returns (uint48 minEpoch, uint48 maxEpoch, Reward[] memory rewardsToClaim)
{
    // ...
    
    // Variables to cache reward claim data as a gas optimization.
    uint256 cachedRewardBucket;
    uint256 cachedClaimedRewards;

    for (uint256 i = 0; i < rewardsLength; ++i) {
        // Cache management (reading and writing)
        {
            uint256 rewardBucket = rewardIds[i] / 256;

            if (i == 0) {
                // @audit cachedRewardBucket is not initialized
                cachedClaimedRewards = userToRewardBucketToClaimedRewards[msg.sender][rewardBucket];
            } else if (cachedRewardBucket != rewardBucket) {             
                // @audit invalid claim data written into storage
                userToRewardBucketToClaimedRewards[msg.sender][cachedRewardBucket] = cachedClaimedRewards;
                
                // Updated cache with the new reward bucket and rewards bit field
                cachedRewardBucket = rewardBucket;
                cachedClaimedRewards = userToRewardBucketToClaimedRewards[msg.sender][rewardBucket];
            }
            // ...
    }
}

However, the caching mechanism contains an issue. Specifically, during the first iteration of the loop (i = 0), only cachedClaimedRewards is set, but cachedRewardBucket is not. As a result, cachedClaimedRewards may contain claim statuses from any reward bucket, while cachedRewardBucket defaults to 0.

This causes an inconsistency: if the rewardId in the first iteration belongs to a reward bucket other than 0 (i.e., rewardId > 255), the subsequent iteration will incorrectly store the updated claim data in bucket 0, enabling users to claim the same reward multiple times for any rewardId greater than 255, by only passing one rewardId in the argument.

Recommendation

Consider assigning cachedRewardBucket in the first iteration of the loop.

6.3 Rewards including epochs with no eligible shares cannot be claimed

The claimRewards function takes an array of rewardIds and calls _getEpochRangeForRewards to fetch the epoch range for all rewards to claim, along with the corresponding Reward structs. This epoch range is then passed to the _computeUserShareRatiosAndDurations function, which calculates the user's share ratios and epoch durations for each epoch within the specified range from minEpoch to maxEpoch.

function _computeUserShareRatiosAndDurations(
    uint48 minEpoch,
    uint48 maxEpoch,
    BalanceUpdate[] storage userBalanceUpdates
) internal view returns (uint256[] memory userShareRatios, uint256[] memory epochDurations) {
    uint256 epochCount = maxEpoch - minEpoch + 1;
    userShareRatios = new uint256[](epochCount);
    epochDurations = new uint256[](epochCount);

    // ...

    // Loop over each epoch from minEpoch to maxEpoch.
    for (uint256 epoch = minEpoch; epoch <= maxEpoch; ++epoch) {
        // ...

        // Retrieve the epoch data.
        Epoch storage epochData = epochs[uint48(epoch)];
        uint256 epochIndex = epoch - minEpoch;
        // Calculate the user's fraction of shares for this epoch.
        // @audit It will revert for epochData.eligibleShares equals to 0
        userShareRatios[epochIndex] = epochSharesBalance.divWadDown(epochData.eligibleShares);
        // Calculate the epoch duration.
        epochDurations[epochIndex] = epochData.endTimestamp - epochData.startTimestamp;
    }
}

However, an issue arises when calculating the user's share ratios. The function divides the user's shares by the total eligible shares for each epoch, causing the transaction to revert if eligibleShares is zero. Since rewards are distributed across multiple epochs, if any epoch in the range has zero eligible shares, users cannot claim rewards for that rewardId.

Recommendation

Implement a check to handle the case where eligibleShares for an epoch is 0 in _computeUserShareRatiosAndDurations.

Resolution

Fix reversions: a4f768c398c269298c1a51b0e9e8875ddebfd2f6

6.4 Incorrect totalSharesBalance update leads to an underflow error

The _decreaseCurrentAndNextEpochParticipation function implements the logic to decrease the user's participation for the current or next epoch. This function is invoked in several cases: when a user disables reward accrual, when a share transfer occurs, or when a user exits the vault and burns a specified number of shares. It is designed to adjust the user’s participation for the current or next epoch accordingly.

However, instead of decreasing the balance by the specified amount, the function overrides the stored shares balance with the user's current ERC20 balance. This logic can lead to inaccuracies, particularly when the function is triggered by the disableRewardAccrual flow. In such cases, the contract should set the user balance update to zero (i.e., reducing the balance update by the full balance), while the user's ERC20 balance remains unchanged. The current code mistakenly overrides the balance with the user’s ERC20 balance, leading to potential underflow errors.

function _decreaseCurrentAndNextEpochParticipation(address user, uint128 amount) internal {// ...int128 userBalance = uint128(balanceOf[user]);
// ...
// CASE 1: No deposit for the next epoch.
if (lastBalanceUpdate.epoch <= currEpoch) {
    if (lastBalanceUpdate.epoch == currEpoch) {
        // @audit totalSharesBalance set to current user ERC20 balance
        lastBalanceUpdate.totalSharesBalance = userBalance;
    } else {
        // @audit totalSharesBalance set to current user ERC20 balance
        userBalanceUpdates.push(BalanceUpdate({epoch: currEpoch, totalSharesBalance: userBalance}));
    }
    // ...
}

// CASE 2: Only deposit made is for the next epoch.
if (balanceUpdatesLength == 1) {
    // @audit totalSharesBalance set to current user ERC20 balance
    lastBalanceUpdate.totalSharesBalance = userBalance;
    // ...
}

// @audit underflow error occures due to incorrect balanceUpdate value 
uint128 amountDepositedIntoNextEpoch =
    lastBalanceUpdate.totalSharesBalance - secondLastBalanceUpdate.totalSharesBalance;

// CASE 3: Deposit Made for the next epoch and the full withdrawal amount cannot be removed solely from the last update.
if (amount > amountDepositedIntoNextEpoch) {
      uint128 amountToWithdrawFromCurrentEpoch = (amount - amountDepositedIntoNextEpoch);
    if (secondLastBalanceUpdate.epoch == currEpoch) {... } else {
        // ...
        // @audit  totalSharesBalance set to current user ERC20 balance
        // ...
    }
    // ...
} else {...}
}

Recommendation

Consider updating the _decreaseCurrentAndNextEpochParticipation function logic to decrease the stored balance update by the provided amount instead of relying on the user's current ERC20 balance.

Update from Nethermind Security

The following line incorrectly reduces the lastBalanceUpdate.totalSharesBalance by amountToWithdrawFromCurrentEpoch, it should be reduced by the full amount instead. This is because the lastBalanceUpdate contains the shares amount of the next epoch.

6.5 Epoch eligible shares can be incorrectly increased

The enableRewardAccrual function allows rewards to be enabled for a specified user. It increases the user's next epoch participation based on their current balance, effectively enabling reward accrual starting from the next epoch. However, the function lacks a check to determine whether the user's rewards accrual is already enabled, allowing the function to be called multiple times.

function enableRewardAccrual(address user) external requiresAuth {
    //@audit Absence of check if rewards are enabled already
    addressToIsDisabled[user] = false;
    _increaseNextEpochParticipation(user, uint128(balanceOf[user]));
}

The internal _increaseNextEpochParticipation function updates the user's totalSharesBalance using their current ERC20 balance and increases the next epoch's eligible shares by the provided amount. The vulnerability allows repeatedly calling the function and artificially increasing the next epoch's eligible shares, reducing other users' share ratios and preventing them from claiming their eligible rewards.

function _increaseNextEpochParticipation(address user, uint128 amount) internal {
    // ...
    // Get the epoch data for the current epoch and next epoch (epoch to deposit for)
    Epoch storage epochData = epochs[nextEpoch];
    // @audit the epoch `eligibleShares` can be incorrectly increased 
    epochData.eligibleShares += amount;

    // Emit event for this deposit
    emit UserDepositedIntoEpoch(user, nextEpoch, amount);
}

Recommendation

Implement a check in the enableRewardAccrual function to prevent it from being called if rewards accrual is already enabled for the user. It is also recommended to implement a similar check in the disableRewardAccrual function.

6.6 Incorrect epoch in the UserWithdrawnFromEpoch event

The _decreaseCurrentAndNextEpochParticipation function emits the UserWithdrawnFromEpoch event, indicating the withdrawn amount from each epoch for the specified user. However, in the 4th case of the function, where the full withdrawal amount is taken from the next epoch, the event erroneously refers to the current epoch ( currEpoch) instead of the next epoch ( nextEpoch).

function _decreaseCurrentAndNextEpochParticipation(address user, uint128 amount) internal {
    // ...
     // CASE 4: The full amount can be withdrawn from the next epoch. Modify the next epoch (last) update.
        lastBalanceUpdate.totalSharesBalance -= amount;
        epochs[uint48(nextEpoch)].eligibleShares -= amount;
        // Emit event for this withdrawal.
        emit UserWithdrawnFromEpoch(user, currEpoch, amount); //@audit must be `nextEpoch` instead of `currEpoch`
        return;
    }
}

Recommendation

Update the event argument to use the correct epoch: nextEpoch instead of currEpoch.

6.7 claimRewards allows claiming invalid reward Ids

The claimRewards function enables users to claim rewards by providing an array of reward IDs. Each claimed reward's bit is marked as 1 in the claimedRewards bit field variable, preventing double claims. However, the function does not validate the provided reward IDs. If a user inputs a reward ID that exceeds the maxRewardId, the corresponding bit is set to true, effectively blocking future claims for that reward.

Recommendation

Implement a validation check within the claimRewards function to ensure that each provided reward ID is valid.

6.8 Unused and redundant code

We provide a non-exhaustive list of unused code below:

• The RewardClaimInfo struct is defined in the BoringChef contract but has never been used in the code;

/// @dev A record of a user's balance changing at a specific epoch
struct RewardClaimInfo {
    /// @dev The epoch in which the deposit was made
    uint48 epoch;
    /// @dev The total number of shares the user has at this epoch
    uint128 totalSharesBalance;
}

• The claimRewards function processes multiple reward IDs by grouping rewards into unique tokens and amounts, minimizing the number of transfer calls to save gas. It initializes arrays for uniqueTokens and tokenAmounts, using uniqueCount to track their lengths. A check ensures that reward campaigns with zero rewards are skipped. The function then loops over the tokens to transfer the accumulated rewards. However, the check if (tokenAmounts[i] > 0) before transferring is redundant, as rewards with zero owed have already been excluded;

function claimRewards(uint256[] calldata rewardIds) external {
    // ...
    for (uint256 i = 0; i < rewardIds.length; ++i) {
        // Calculate the total rewards owed for this reward campaign.
        uint256 rewardsOwed = _calculateRewardsOwed(rewardsToClaim[i], minEpoch, userShareRatios, epochDurations);

        if (rewardsOwed > 0) {
            // ...
            if (!found) {
                uniqueTokens[uniqueCount] = rewardsToClaim[i].token;
                tokenAmounts[uniqueCount] = rewardsOwed;
                uniqueCount++;
            }
            // Emit the reward-claim event per reward campaign.
            emit UserRewardsClaimed(msg.sender, rewardIds[i], rewardsOwed);
        }
    }

    // Finally, do one transfer per unique reward token.
    for (uint256 i = 0; i < uniqueCount; ++i) {
        // @audit Redundant check
        if (tokenAmounts[i] > 0) {
            boringSafe.transfer(uniqueTokens[i], msg.sender, tokenAmounts[i]);
        }
    }
}

Recommendation

Remove the unused and redundant code.