ZeroTrust - In the _claimAccountRewards function, skipping the third-party reward claim might cause losses for users.

[sherlock-admin4]

ZeroTrust

Medium

In the _claimAccountRewards function, skipping the third-party reward claim might cause losses for users.

Summary

In the _claimAccountRewards function, skipping the third-party reward claim might cause losses for users.

Vulnerability Detail

   function _claimAccountRewards(
        address account,
        uint256 totalVaultSharesBefore,
        uint256 vaultSharesBefore,
        uint256 vaultSharesAfter
    ) internal {
        (VaultRewardState[] memory state, StrategyVaultSettings memory s, RewardPoolStorage memory r) = getRewardSettings();
@>>        if (r.lastClaimTimestamp + s.forceClaimAfter < block.timestamp) {
@>>             _claimVaultRewards(totalVaultSharesBefore, state, r);
        }

        for (uint256 i; i < state.length; i++) {
            if (0 < state[i].emissionRatePerYear) {
                // Accumulate any rewards with an emission rate here，
                _accumulateSecondaryRewardViaEmissionRate(i, state[i], totalVaultSharesBefore);
            }

            _claimRewardToken(
                state[i].rewardToken,
                account,
                vaultSharesBefore,
                vaultSharesAfter,
                state[i].accumulatedRewardPerVaultShare
            );
        }
    }

Skipping the third-party reward claim during a certain time period, although it can save some gas, may cause a loss of rewards for users. This is especially true if the third-party rewards increase non-linearly, as there might be a significant amount of rewards available during that specific period.

Impact

This can result in users losing a portion of their rewards.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L255

Tool used

Manual Review

Recommendation

-     if (r.lastClaimTimestamp + s.forceClaimAfter < block.timestamp) {
             _claimVaultRewards(totalVaultSharesBefore, state, r);
-        }

[sherlock-admin4]

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

0xmystery commented:

Lacks proof to substantiate the bug

[ZeroTrust01]

Escalate

This issue is very obvious and simple: skipping the call to _claimAccountRewards will result in the loss of the rewards from the last time _claimAccountRewards was called up to the current time. This is especially true if the third-party rewards increase non-linearly, as there might be a significant amount of rewards available during that specific period.

[sherlock-admin3]

Escalate

This issue is very obvious and simple: skipping the call to _claimAccountRewards will result in the loss of the rewards from the last time _claimAccountRewards was called up to the current time. This is especially true if the third-party rewards increase non-linearly, as there might be a significant amount of rewards available during that specific period.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

Escalate

This issue is very obvious and simple: skipping the call to _claimAccountRewards will result in the loss of the rewards from the last time _claimAccountRewards was called up to the current time. This is especially true if the third-party rewards increase non-linearly, as there might be a significant amount of rewards available during that specific period.

But you can't violate the intended check regardless right?

[WangSecurity]

This report doesn't explain how and when it can cause an issue.

Skipping the third-party reward claim during a certain time period

Which time period, is it even possible?

may cause a loss of rewards for users

How it can cause any losses? There's no evidence of it in the report.

This is especially true if the third-party rewards increase non-linearly, as there might be a significant amount of rewards available during that specific period

Which 3rd-parties have such a design? Does Notional integrate with them? Is there any evidence it plans on integrating with them?

In general, the report's not certain there can be a loss, it only may cause losses without any evidence it indeed can.

The report is not sufficient to be valid. Planning to reject the escalation and leave the issue as it is.

[ZeroTrust01]

(1) Which time period, is it even possible?

if block.timestamp is not bigger than (r.lastClaimTimestamp + s.forceClaimAfter) the _claimVaultRewards function will not be called.

(2) How it can cause any losses? There’s no evidence of it in the report.

The _claimVaultRewards function is used to claim rewards from other protocols (3rd-parties like AURA, CONVEX), then it accumulates the reward per share coefficient, and users receive rewards based on their share and the reward per share. So the loss is the unclaimed rewards during the skipped period.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L148

function _claimVaultRewards(
        uint256 totalVaultSharesBefore,
        VaultRewardState[] memory state,
        RewardPoolStorage memory rewardPool
    ) internal {
        uint256[] memory balancesBefore = new uint256[](state.length);
        // Run a generic call against the reward pool and then do a balance
        // before and after check.
        for (uint256 i; i < state.length; i++) {
            // Presumes that ETH will never be given out as a reward token.
            balancesBefore[i] = IERC20(state[i].rewardToken).balanceOf(address(this));
        }

    @>>    _executeClaim(rewardPool);

        rewardPool.lastClaimTimestamp = uint32(block.timestamp);
        VaultStorage.setRewardPoolStorage(rewardPool);

        // This only accumulates rewards claimed, it does not accumulate any secondary emissions
        // that are streamed to vault users.
        for (uint256 i; i < state.length; i++) {
            uint256 balanceAfter = IERC20(state[i].rewardToken).balanceOf(address(this));
    @>>        _accumulateSecondaryRewardViaClaim(
                i,
                state[i],
                // balanceAfter should never be less than balanceBefore
                balanceAfter - balancesBefore[i],
                totalVaultSharesBefore
            );
        }
    }

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/common/VaultRewarderLib.sol#L181

/// @notice Executes the proper call for various rewarder types.
    function _executeClaim(RewardPoolStorage memory r) internal {
@>        if (r.poolType == RewardPoolType.AURA) {
            require(IAuraRewardPool(r.rewardPool).getReward(address(this), true));
@>        } else if (r.poolType == RewardPoolType.CONVEX_MAINNET) {
            require(IConvexRewardPool(r.rewardPool).getReward(address(this), true));
        } else if (r.poolType == RewardPoolType.CONVEX_ARBITRUM) {
            IConvexRewardPoolArbitrum(r.rewardPool).getReward(address(this));
        } else {
            revert();
        }
    }

[WangSecurity]

Thank you for clarification, but it should've been in the report. Since it's not there and the report lacks sufficient proof the vulnerability is real, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZeroTrust01: rejected