The rewardIndex only increases, leading to eventual inability to claim rewards
Description
The StakingV2 contract uses rewardIndex, which acts as an epoch-based rewards tracker. Due to rounding errors in Solidity, it is possible that rewardIndex increases to the point where _verifyAndPrepareClaim() will revert.
The rewardIndex variable is an ever-increasing denominator in the totalUserVaultShares calculation on line 348, while totalVaultShares (the numerator) can decrease in value over time.
This leads to cases where long-term use of the staking contract may result in totalUserVaultShares = 0 when userReward * totalVaultShares < rewardIndex.
The following test case illustrates cases where the user is claiming full withdrawals, but as rewardIndex increases, _verifyAndPrepareClaim() eventually reverts with withdrawal amount exceeds available rewards.
function test_increment_rewardIndex_decrement_totalVaultShares() public {
// Setup
address staker1 = makeAddr("staker_one");
address staker2 = makeAddr("staker_two");
// 1. stake() with 10 ether
vm.startPrank(staker1);
token.mint(address(staker1), 1000 ether);
token.approve(address(stakeV2), 10 ether);
stakeV2.stake(10 ether);
vm.stopPrank();
// Assert Initial Test State
uint256 initialStakeBalance = stakeV2.balanceOf(staker1);
uint256 earnedStaker1 = stakeV2.calculateRewardsEarned(staker1);
assertEq(initialStakeBalance, 10 ether, "invalid initialStakeBalance");
assertEq(earnedStaker1, 0, "invalid earnedStaker1");
uint256 totalIterations = 20;
for(uint256 x = 0; x < totalIterations; ++x) {
// 2. depositReward()
uint256 rewardAmount = 1 ether;
vm.deal(address(this), rewardAmount);
stakeV2.depositReward{value: rewardAmount}();
// 3. executeRewardDistribution()
uint256 expectedIslandTokens = 0 ether;
mockZapper.setReturnValues(expectedIslandTokens, rewardAmount); // simulates the zapper logic at a 1:1 ratio for vaultsSharesMinted:accumulatedRewards
uint256 expectedRewardIndex = (x + 1) * rewardAmount * 1 ether / stakeV2.totalSupply();
vm.expectEmit(true, true, false, true);
emit StakeV2.RewardsDistributed(rewardAmount, expectedRewardIndex);
stakeV2.executeRewardDistribution(
IZapper.SingleTokenSwap(0, 0, 0, address(0), ""),
IZapper.SingleTokenSwap(0, 0, 0, address(0), ""),
IZapper.KodiakVaultStakingParams(address(0), 0, 0, 0, 0, 0, address(0)),
IZapper.VaultDepositParams(address(0), address(0), 0)
);
// Assert Intermediate Test State
assertEq(stakeV2.accumulatedRewards(), 0, "Accumulated rewards should be reset to 0");
assertEq(stakeV2.rewardIndex(), expectedRewardIndex, "Reward index should be updated correctly");
// 4. claimRewardsInNative()
vm.startPrank(staker1);
uint256 totalUserVaultShares = rewardAmount * stakeV2.totalVaultShares() / stakeV2.rewardIndex();
uint256 amountToWithdraw = stakeV2.totalVaultShares() * stakeV2.totalVaultShares() / stakeV2.rewardIndex(); // @audit withdraw total Vault shares
mockZapper.setZapOutNativeReturn(amountToWithdraw); // simulates the zapper logic at a 1:1 ratio for vaultsSharesMinted:accumulatedRewards
stakeV2.claimRewardsInNative( // REVERT: On the 20th claim this reverts
amountToWithdraw,
IZapper.SingleTokenSwap(0, 0, 0, address(0), ""),
IZapper.SingleTokenSwap(0, 0, 0, address(0), ""),
IZapper.KodiakVaultUnstakingParams(address(0), 0, 0, address(0)),
IZapper.VaultRedeemParams(address(0), address(0), 0, 0)
);
}
}Impact
Due to cumulative rounding errors, users with smaller rewards are incapable of withdrawing rewards. Over time, this issue worsens as rewardIndex is never decremented.
Recommendations
We recommend simplifying the _verifyAndPrepareClaim() function as much as possible.
Stake.sol allowed full, direct withdrawals of exactly all earned[msg.sender]. StakeV2.sol uses the exact same metric for earned[msg.sender], which is units of rewards, meaning that withdrawals can simply also be in reward unit measurements rather than in some fractional representation that must be converted to rewards and vice versa.
Remediation
This issue has been acknowledged by Sanguine Labs LTD, and a fix was implemented in commit 2172fc05↗.