Incorrect increase of `totalRewards` in `_increaseUserShare` function

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L78-L85

Vulnerability details

Issue Description

The totalRewards represent the total pending SALT rewards for each poolID. When the StakingRewards.addSALTRewards function is called during the global upkeep call, it updates the totalRewards value for each pool according to the SALT reward received.

The issue is that the totalRewards is incorrectly increased in the StakingRewards._increaseUserShare function when a user tries to increase their shares:

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

        user.virtualRewards += uint128(virtualRewardsToAdd);
        //@audit should not increase totalRewards
        totalRewards[poolID] += uint128(virtualRewardsToAdd);
    }

This is incorrect because virtual rewards virtualRewardsToAdd are only used to deduct a portion of the user rewards that they are not allowed to claim as they increase their shares at the given totalRewards/totalShares ratio.

By increasing the totalRewards value by virtualRewardsToAdd, the function is adding virtual rewards that don't exist in the contract yet in the form of SALT tokens. This will make the reward calculation incorrect and compromise the whole shares/rewards system.

Users who stake afterward will be promised rewards that don't exist and may not be able to claim them later due to insufficient SALT funds in the contract.

Impact

The incorrect update of totalRewards in the StakingRewards._increaseUserShare function will lead to bad shares/rewards accounting and result in some users being unable to claim rewards due to insufficient SALT funds in the contract.

Proof of Concept

Let's take a simple scenario of a user Bob who will deposit 1000 shares in the pool poolID == 10.

• Before Bob's call, we had the following states:

Bob shares: user.userShare = 0

Bob virtual rewards: user.virtualRewards = 0

Pool total shares: totalShares[poolID] = 8000

Pool total rewards: totalRewards[poolID] = 10000

• When Bob calls the depositLiquidityAndIncreaseShare function, it will internally invoke StakingRewards._increaseUserShare function to add the 1000 shares.

• After StakingRewards._increaseUserShare is called, we get to these states using the formula above:

Bob shares: user.userShare = 1000

Bob virtual rewards: user.virtualRewards = 1250

Pool total shares: totalShares[poolID] = 9000

Pool total rewards: totalRewards[poolID] = 11250

• We see here that the totalRewards was increased from 10000 to 11250, but we know that no SALT rewards were added to the contract to reflect that change. These rewards are virtual, having no existence, yet they are recorded as normal rewards.

After Bob's call, anyone who increases their shares will receive an incorrect rewards amount. If later on, all users try to claim their rewards, some won't be able to do so as they will have insufficient SALT balance in the contract to account for all the rewards. It will come down to who claims first.

The magnitude of this issue will increase with each instance of a user increasing shares.

Tools Used

Manual review, VS Code

Recommended Mitigation

To solve this issue, the totalRewards value should not be increased in the StakingRewards._increaseUserShare function:

function _increaseUserShare(
    address wallet,
    bytes32 poolID,
    uint256 increaseShareAmount,
    bool useCooldown
) internal {
    ...
    uint256 existingTotalShares = totalShares[poolID];

    // Determine the amount of virtualRewards to add based on the current ratio of rewards/shares.
    // The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool.
    // The virtual rewards will be deducted later when calculating the user's owed rewards.
    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);
    }
    ...
}

Assessed type

Error

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Regarding virtualRewards: the idea is that the ratio of totalRewards/totalShare before the user increases share needs to equal (totalRewards+virtualRewards)/(totalShare+shareIncrease). Namely that the ratio of rewards to shares before and after needs to remain the same. It's akin to a liquidity pool where the two tokens are "rewards" and "shares". When a user want add shares they borrow the rewards needed to create the correct proportion of virtualRewards / addedShares.

[Picodes]

Unless I am missing something the code seems correct. These rewards are virtual and are an accountability trick to compute the actual rewards accumulated by the staker later on.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof