`claimRewards()` may run out of gas and revert due to long list of extra rewards

[code423n4]

Lines of code

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

Vulnerability details

Impact

Rewards will not be able to be transferred because attempts to do so might revert

Proof of Concept

The function claimRewards(address[] memory _tokenAddresses) will transfer the rewards for each token address in the params. For each of these tokens, CRV and CVX will be transferred along with all the tokens in the list of extra rewards.

Vault.sol claimRewards(...)

    // Loop and claim all virtual rewards
    uint256 _extraRewards = _rewardsContract.extraRewardsLength();
    for (uint256 _j; _j < _extraRewards; ) {
        // @audit may run out of gas and revert due to long list of extra rewards
        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 number of extra rewards doesn't have an upper limit, and there is no guarantee that these tokens will be efficient in their use of gas.

Even if not every extra reward token has a balance if (_earnedReward != 0), an attacker can sprinkle each one with dust, forcing a transfer by this function.

So due to these reasons, the gas usage cannot be estimated and the transaction can be reverted.

Tools Used

Manual review

Recommended Mitigation Steps

Include an offset and length for claiming the extra rewards

Assessed type

Loop

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

0xShaito marked the issue as disagree with severity

[0xShaito]

Adding rewards is not permissionless and is the job of convex governance. Adding too many extraRewards could lead to breaking their own ‘claimRewards(extras=true)’ function so it's against their interest to do so.

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid