Zellic - Audit Reports

Category: Coding Mistakes

Formulaic error allows stakers to steal excess rewards

Critical Severity

Critical Impact

High Likelihood

Description

The StakingV2 contract tracks total shares staked over reward-bearing epochs. The contract makes formulaic errors due to incorrect units of measurements being used during subtraction of earned[msg.sender]. This leads to less rewards being deducted than are withdrawn by the user.

The StakeV2._verifyAndPrepareClaim() internal function used to handle internal reward accounting calculates the shares to deduct as follows:

uint256 sharesToWithdraw = (amountToWithdraw * rewardIndex) / totalVaultShares;
require(sharesToWithdraw <= totalUserVaultShares, "Withdrawal amount exceeds available rewards");

uint256 rewardsToClaim = (sharesToWithdraw * rewardIndex) / totalVaultShares;
require(rewardsToClaim <= userReward, "Withdrawal amount exceeds rewards earned by the user");
earned[msg.sender] -= rewardsToClaim;
totalVaultShares -= sharesToWithdraw;

However, if we attempt to derive the units of measurement for earned[msg.sender],

earned[account] = stakingToken * rewardPerStakingToken
earned[account] = reward

As we can see on line 127 in executeRewardDistribution(), the totalVaultShares represents the total reward held by the contract. As such, the subtraction from earned[msg.sender] < totalVaultShares.

The following test case illustrates that despite the staking contract not having any totalVaultShares (implying full withdrawal of rewards), the only user staked still has rewards remaining.

function test_fails_to_decrement_earned() 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");
    
    // 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 = 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 expectedTotalUserVaultShares = 1e19;
    uint256 amountToWithdraw = expectedTotalUserVaultShares;
    
    mockZapper.setZapOutNativeReturn(1 ether); // simulates the zapper logic at a 1:1 ratio for vaultsSharesMinted:accumulatedRewards
    stakeV2.claimRewardsInNative(
        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)
    );
    // vm.stopPrank();

    // Assert Post Test State
    uint256 finalVaultShares = stakeV2.totalVaultShares();
    uint256 finalEarnedStaker1 = stakeV2.calculateRewardsEarned(staker1);
    assertEq(finalVaultShares, 0 ether, "invalid finalVaultShares");
    assertEq(finalEarnedStaker1, 0, "invalid finalEarnedStaker1"); // Reverts due to left over rewards 
}

Impact

Users may claim more rewards than they are eligible for, resulting in a potential loss of funds for those who are slower to claim their rewards.

Recommendations

We recommend revisiting the _verifyAndClaim() function's internal math operations, ensuring that the correct units of measurement are adhered to.

Remediation

This issue has been acknowledged by Sanguine Labs LTD, and a fix was implemented in commit 2172fc05↗.