eeyore - Loss of user-earned rewards due to lack of recovery mechanism and insufficient reward token balances in the VaultRewarderLib _claimRewardToken() function

[sherlock-admin4]

eeyore

Medium

Loss of user-earned rewards due to lack of recovery mechanism and insufficient reward token balances in the VaultRewarderLib _claimRewardToken() function

Summary

The protocol zeroes out user-earned rewards if there are no reward tokens to transfer within the vault contract.

Vulnerability Detail

In the VaultRewarderLib, users can earn extra APR from incentive tokens provided by booster protocols. There are three ways these incentive tokens can be handled:

1. Automatic Reinvest: Tokens are claimed by the vault (from booster protocols) and reinvested into the vault.
2. Account Claim: Tokens are claimed by the vault (from booster protocols) and distributed per vault share.
3. Emission Rate: Extra tokens are distributed per emissionRatePerYear per vault share. These tokens are not claimed but are transferred to the vault externally.

These methods can be used in combinations.

In the default flow, vaults operate in the Automatic Reinvest mode, ensuring there is no loss for the user as rewards are reinvested into the vault.

However, when Account Claim and Emission Rate are used together, users may unexpectedly lose rewards because their rewards are zeroed out during the claim process. This happens because rewards for the Emission Rate are transferred to the vault indirectly and are not reserved for the user. In the worst case, the emissionRatePerYear can be set, but rewards may never be transferred into the vault.

If the rewards for the Emission Rate are not transferred, users who claim rewards first will cannibalize the rewards from the Account Claim functionality, leaving later users without rewards from either source. Essentially, rewards are distributed on a first-come, first-served basis.

In the _claimRewardToken() function, where reward tokens should be transferred to the users, the user rewards are zeroed if there is an insufficient balance of reward tokens, and information about untransferred/pending rewards is not stored.

function _claimRewardToken(
        address rewardToken,
        address account,
        uint256 vaultSharesBefore,
        uint256 vaultSharesAfter,
        uint256 rewardsPerVaultShare
    ) internal returns (uint256 rewardToClaim) {
        rewardToClaim = _getRewardsToClaim(
            rewardToken, account, vaultSharesBefore, rewardsPerVaultShare
        );

        VaultStorage.getAccountRewardDebt()[rewardToken][account] = (
            (vaultSharesAfter * rewardsPerVaultShare) /
                uint256(Constants.INTERNAL_TOKEN_PRECISION)
        ); // <-- This equates to zeroing the user rewards if the transfer fails in the next step.

        if (0 < rewardToClaim) {
            // Ignore transfer errors here so that any strange failures here do not
            // prevent normal vault operations from working. Failures may include a
            // lack of balances or some sort of blacklist that prevents an account
            // from receiving tokens.
            try IEIP20NonStandard(rewardToken).transfer(account, rewardToClaim) {
                bool success = TokenUtils.checkReturnCode();
                if (success) {
                    emit VaultRewardTransfer(rewardToken, account, rewardToClaim);
                } else {
                    emit VaultRewardTransfer(rewardToken, account, 0);
                }
            // Emits zero tokens transferred if the transfer fails.
            } catch {
                emit VaultRewardTransfer(rewardToken, account, 0);
            }
        }
    }

Token transfers are most likely to fail due to a lack of balances rather than blacklisting. Valid, non-blacklisted users should not suffer losses because of this. This leads to a direct loss of funds/rewards for the users and unfair reward distribution.

Impact

User-earned rewards are lost both from Account Claim and Emission Rate.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L295-L328

Tool used

Manual Review

Recommendation

In case of a transfer failure, which is most likely due to a lack of balances, the unclaimed value should be stored for future attempts if rewards for the Emission Rate are transferred later.

Introduce new storage values and update the functions as in this example:

/** Reward Claim Methods **/
    function _getRewardsToClaim( 
        address rewardToken,
        address account,
        uint256 vaultSharesBefore,
        uint256 rewardsPerVaultShare
    ) internal view returns (uint256 rewardToClaim) {
        // Vault shares are always in 8 decimal precision
        rewardToClaim = (
            (vaultSharesBefore * rewardsPerVaultShare) / uint256(Constants.INTERNAL_TOKEN_PRECISION)
        ) - VaultStorage.getAccountRewardDebt()[rewardToken][account]
+       + VaultStorage.getAccountRewards()[rewardToken][account];
    }

if (0 < rewardToClaim) {
            // Ignore transfer errors here so that any strange failures here do not
            // prevent normal vault operations from working. Failures may include a
            // lack of balances or some sort of blacklist that prevents an account
            // from receiving tokens.
            try IEIP20NonStandard(rewardToken).transfer(account, rewardToClaim) { // @audit-issue - M - most likely will revert due to insufficient balance
                bool success = TokenUtils.checkReturnCode();
                if (success) {
                    emit VaultRewardTransfer(rewardToken, account, rewardToClaim);
+                   VaultStorage.getAccountRewards()[rewardToken][account] = 0;
                } else {
                    emit VaultRewardTransfer(rewardToken, account, 0); // @audit-issue - M - missing recovery mechanism
+                   VaultStorage.getAccountRewards()[rewardToken][account] = rewardToClaim;
                }
            // Emits zero tokens transferred if the transfer fails.
            } catch {
                emit VaultRewardTransfer(rewardToken, account, 0);
+               VaultStorage.getAccountRewards()[rewardToken][account] = rewardToClaim;
            }
        }

Duplicate of #1

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Hash01011122 commented:

Invalid, No possible attack path mentioned nor justified how insufficient reward token balances scenario can take place