search

code-423n4 / 2022-05-vetoken-findings

1 stars 1 forks source link

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

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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