RaymondFam - _accumulateExternalRewards() could turn into an infinite loop if the check condition is true

[github-actions]

RaymondFam

medium

_accumulateExternalRewards() could turn into an infinite loop if the check condition is true

Summary

In WstethLiquidityVault.sol, the for loop in _accumulateExternalRewards() utilizes continue so it could proceed to the next iteration upon having a true condition in the sanity check. This will however turn the function into an infinite loop because ++i has been included at the end of the loop logic. As a result, this skipped increment leads to the same externalRewardTokens[i] repeatedly assigned to rewardToken where newBalance < rewardToken.lastBalance continues to equal true until the same executions make the gas run out.

Vulnerability Detail

Here is a typical scenario:

1. _accumulateExternalRewards() gets invoked via one of the functions embedding it, i.e. claimRewards(), _depositUpdateRewardState() or _withdrawUpdateRewardState() of SingleSidedLiquidityVault.sol.
2. It happens that newBalance < rewardToken.lastBalance returns true for a specific reward token.
3. Because continue comes before ++i, this non-incremented iteration is repeatedly executed till gas is run out.

Impact

This will persistently cause DOS on _accumulateExternalRewards() for all function calls dependent on it. Depending on how big the deficiency is, the situation can only be remedied by:

• having the deficiency of contract balance on this particular reward token separately topped up at the expense of accounting mess up and/or the protocol resorting to a portion of its reward token(s) locked in the contract whenever this incident happens,
• waiting for a long enough time till the harvested reward is going to be larger than the deficiency entailed, or
• getting the contract deactivated to temporarily prevent further deposits, withdrawals, or reward claims which will nonetheless break other things when deactivate() is called.

Note: The situation could be worse if more than 1 elements in the array ExternalRewardToken[] were similarly affected.

Code Snippet

File: WstethLiquidityVault.sol#L192-L216

    function _accumulateExternalRewards() internal override returns (uint256[] memory) {
        uint256 numExternalRewards = externalRewardTokens.length;

        auraPool.rewardsPool.getReward(address(this), true);

        uint256[] memory rewards = new uint256[](numExternalRewards);
        for (uint256 i; i < numExternalRewards; ) {
            ExternalRewardToken storage rewardToken = externalRewardTokens[i];
            uint256 newBalance = ERC20(rewardToken.token).balanceOf(address(this));

            // This shouldn't happen but adding a sanity check in case
            if (newBalance < rewardToken.lastBalance) {
                emit LiquidityVault_ExternalAccumulationError(rewardToken.token);
                continue;
            }

            rewards[i] = newBalance - rewardToken.lastBalance;
            rewardToken.lastBalance = newBalance;

            unchecked {
                ++i;
            }
        }
        return rewards;
    }

Tool used

Manual Review

Recommendation

Consider having the affected code logic refactored as follows:

    function _accumulateExternalRewards() internal override returns (uint256[] memory) {
        uint256 numExternalRewards = externalRewardTokens.length;

        auraPool.rewardsPool.getReward(address(this), true);

        uint256[] memory rewards = new uint256[](numExternalRewards);

+    unchecked {
-        for (uint256 i; i < numExternalRewards; ) {
+        for (uint256 i; i < numExternalRewards; ++i;) {
            ExternalRewardToken storage rewardToken = externalRewardTokens[i];
            uint256 newBalance = ERC20(rewardToken.token).balanceOf(address(this));

            // This shouldn't happen but adding a sanity check in case
            if (newBalance < rewardToken.lastBalance) {
                emit LiquidityVault_ExternalAccumulationError(rewardToken.token);
                continue;
            }

            rewards[i] = newBalance - rewardToken.lastBalance;
            rewardToken.lastBalance = newBalance;

-            unchecked {
-                ++i;
-            }
        }
+    }
        return rewards;
    }

This will safely increment i when continue is hit and move on to the next i + 1 iteration while still having SafeMath unchecked for the entire scope of the for loop.