If a new extra reward is added later, existing stakes will not be able to withdraw

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L121-L129
https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L175-L178
https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L198-L201
https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L217-L220
https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L244-L247

Vulnerability details

Impact

When the user stakes token, it iterates over all the extraRewards and adds to the user stake:

  function stake(uint256 _amount) public updateReward(msg.sender) returns (bool) {
    ...
    //also stake to linked rewards
    for (uint256 i = 0; i < extraRewards.length; i++) {
        IRewards(extraRewards[i]).stake(msg.sender, _amount);
    }

When withdrawing it also iterates over all the list and claims the rewards:

  //also withdraw from linked rewards
  for (uint256 i = 0; i < extraRewards.length; i++) {
      IRewards(extraRewards[i]).withdraw(msg.sender, amount);
  }

The problem is that extraRewards list might change between the stake and withdrawal because a reward manager can add a new extra reward:

  function addExtraReward(address _reward) external returns (bool) {
    ...
    extraRewards.push(_reward);
    ...
  }

Existing stakes will not be updated and while the exact implementation of extra rewards is not clear, I expect that it should revert if the amount exceeds user's balance.

The same situation is present in both VE3DRewardPool and BaseRewardPool.

Recommended Mitigation Steps

I think claiming extra rewards should be extracted to a separate function where users can specify the exact rewards that they are interested in.

[solvetony]

Even if there is a new extra pool added after user stake, user will be able to claim reward from it also because it depends in the user balance of the main reward pool not the virtual pool

[GalloDaSballo]

From my understanding extraRewards are contracts that do not require a direct deposit nor interaction, see

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VirtualBalanceRewardPool.sol#L61-L62

        return deposits.balanceOf(account);

So, while there may be additional risks that can come from the interaction of the BaseRewardsPool and the extraRewards, in lack of further details, I believe the finding to be invalid.

Please consider adding a coded POC to prove loose of funds / unfair distribution of rewards