Directly calling BaseRewardPool.getReward locks rewards in Vault

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/core/Vault.sol#L187-L200

Vulnerability details

Impact

A malicious actor can call IBaseRewardPool.getReward for any vault address, which will transfer rewards into the vault without a way to get them out.

Proof of Concept

The function Vault.claimRewards contains logic to claim rewards individually:

if (_crvReward != 0) {
  // Claim the CRV reward
  _totalCrvReward += _crvReward;
  //WARDEN: the false-flag prevents extra rewards from being sent to the contract
  _rewardsContract.getReward(address(this), false);
  _totalCvxReward += _calculateCVXReward(_crvReward);
}
...

// Loop and claim all virtual rewards
uint256 _extraRewards = _rewardsContract.extraRewardsLength();
for (uint256 _j; _j < _extraRewards;) {
  IVirtualBalanceRewardPool _virtualReward = _rewardsContract.extraRewards(_j);
  IERC20 _rewardToken = _virtualReward.rewardToken();
  uint256 _earnedReward = _virtualReward.earned(address(this));
  if (_earnedReward != 0) {
    _virtualReward.getReward();
    _rewardToken.transfer(_msgSender(), _earnedReward);
    emit ClaimedReward(address(_rewardToken), _earnedReward);
  }
  unchecked {
    ++_j;
  }
}

The issue is that BaseRewardPool.getReward is openly accessible and the Vault does not account for that:

Link

    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;
    }

So anyone can claim extra rewards for the vault, but the vault only sends them to the minter/owner if the vault has claimed them itself.

Tools Used

Manual review

Recommended Mitigation Steps

A possible solution might be to not check for if (_earnedReward != 0) but rather send out the balance that the vault has of the extra reward tokens.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as duplicate of #301

[c4-judge]

dmvt marked the issue as satisfactory