code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

user can claim rewards more than they deserve #871

Closed c4-bot-2 closed 7 months ago

c4-bot-2 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L81-L84

Vulnerability details

Impact

cause of rounding up at total rewards ,user can claim more rewards than they derserved.

Proof of Concept

      if ( existingTotalShares != 0 ) // prevent / 0
        {
        // Round up in favor of the protocol.
    ----->> uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

        user.virtualRewards += uint128(virtualRewardsToAdd);
        ---->>  totalRewards[poolID] += uint128(virtualRewardsToAdd);
        }       

VirtualRewards is rounding up in favor of protocol ,But TotalRewards also rounding up , this will lead to in favour of user .User can claim more than worth of their shares .

   // Determine the share of the rewards for the amountToDecrease (will include previously added virtual rewards)
---->>  uint256 rewardsForAmount = ( totalRewards[poolID] * decreaseShareAmount ) / totalShares[poolID];

    // For the amountToDecrease determine the proportion of virtualRewards (proportional to all virtualRewards for the user)
    // Round virtualRewards down in favor of the protocol
    uint256 virtualRewardsToRemove = (user.virtualRewards * decreaseShareAmount) / user.userShare;

    // Update totals
    totalRewards[poolID] -= rewardsForAmount;
    totalShares[poolID] -= decreaseShareAmount;

    // Update the user's share and virtual rewards
    user.userShare -= uint128(decreaseShareAmount);
    user.virtualRewards -= uint128(virtualRewardsToRemove);

    uint256 claimableRewards = 0;

    // Some of the rewardsForAmount are actually virtualRewards and can't be claimed.
    // In the event that virtualRewards are greater than actual rewards - claimableRewards will stay zero.
    if ( virtualRewardsToRemove < rewardsForAmount )
        claimableRewards = rewardsForAmount - virtualRewardsToRemove;

rounding up totalrewards is multiplied by share of user , rewardAmount will also be effected by rounding up .

Tools Used

manual view

Recommended Mitigation Steps

rounding down in totalRewards in favor protocol

Assessed type

Context