0x52 - rewardTokens removed from WAuraPool/WConvexPools will be lost forever

[sherlock-admin]

0x52

high

rewardTokens removed from WAuraPool/WConvexPools will be lost forever

Summary

pendingRewards pulls a fresh count of reward tokens each time it is called. This is problematic if reward tokens are ever removed from the the underlying Aura/Convex pools because it means that they will no longer be distributed and will be locked in the contract forever.

Vulnerability Detail

WAuraPools.sol#L166-L189

    uint extraRewardsCount = IAuraRewarder(crvRewarder)
        .extraRewardsLength();
    tokens = new address[](extraRewardsCount + 1);
    rewards = new uint256[](extraRewardsCount + 1);

    tokens[0] = IAuraRewarder(crvRewarder).rewardToken();
    rewards[0] = _getPendingReward(
        stCrvPerShare,
        crvRewarder,
        amount,
        lpDecimals
    );

    for (uint i = 0; i < extraRewardsCount; i++) {
        address rewarder = IAuraRewarder(crvRewarder).extraRewards(i);
        uint256 stRewardPerShare = accExtPerShare[tokenId][i];
        tokens[i + 1] = IAuraRewarder(rewarder).rewardToken();
        rewards[i + 1] = _getPendingReward(
            stRewardPerShare,
            rewarder,
            amount,
            lpDecimals
        );
    }

In the lines above we can see that only tokens that are currently available on the pool. This means that if tokens are removed then they are no longer claimable and will be lost to those entitled to shares.

Impact

Users will lose reward tokens if they are removed

Code Snippet

WAuraPools.sol#L152-L190

Tool used

Manual Review

Recommendation

Reward tokens should be stored with the tokenID so that it can still be paid out even if it the extra rewardToken is removed.

[Gornutz]

https://github.com/Blueberryfi/blueberry-core/commit/f57a1db99639cb4f09836829aee5e2a89687b1be

[IAm0x52]

Fix uses incorrect index for accExtPerShareusing. Using i in accExtPerShare is incorrect and should instead use extrawRewardsIdx[rewarder]. If a reward is removed it will cause the order of the indexes to change and accExtPerShare can return an incorrect value, sending the user an incorrect amount of the asset or causing the entire call to revert.