decrease userShare might result in `totalRewards` equal to 0

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

When totalRewards is equal to 0, the attacker can steal the salt token in the contract.

Proof of Concept

When the DAO initializes the pool, it adds SALTRewards:

    function _finalizeTokenWhitelisting( uint256 ballotID ) internal
        {
        if ( proposals.ballotIsApproved(ballotID ) )
            {
            ......
            AddedReward[] memory addedRewards = new AddedReward[](2);
            addedRewards[0] = AddedReward( pool1, bootstrappingRewards );
            addedRewards[1] = AddedReward( pool2, bootstrappingRewards );

            exchangeConfig.salt().approve( address(liquidityRewardsEmitter), bootstrappingRewards * 2 );
            liquidityRewardsEmitter.addSALTRewards( addedRewards );

            emit WhitelistToken(IERC20(ballot.address1));
            }
        proposals.markBallotAsFinalized(ballotID);
        }

However, liquidityRewardsEmitter does not increase the pool's totalRewards immediately, but records it in the pendingReward first:

    function addSALTRewards( AddedReward[] calldata addedRewards ) external nonReentrant
        {
        uint256 sum = 0;
        for( uint256 i = 0; i < addedRewards.length; i++ )
            {
            AddedReward memory addedReward = addedRewards[i];
            require( poolsConfig.isWhitelisted( addedReward.poolID ), "Invalid pool" );

            uint256 amountToAdd = addedReward.amountToAdd;
            if ( amountToAdd != 0 )
                {
                // Update pendingRewards so the SALT can be distributed later
@>              pendingRewards[ addedReward.poolID ] += amountToAdd;
                sum += amountToAdd;
                }
            }

        // Transfer the SALT from the sender for all of the specified rewards
        if ( sum > 0 )
            salt.safeTransferFrom( msg.sender, address(this), sum );
        }

Therefore, when the user is staking or adding liquidity, the totalRewards in the StakingRewards may be 0.

In addition, the StakingRewards#_decreaseUserShare function reduces totalRewards:

    function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
        {
        ......
        totalRewards[poolID] -= rewardsForAmount;
        totalShares[poolID] -= decreaseShareAmount;
        ....
        }

    function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
        {
        ....
        if ( existingTotalShares != 0 ) // prevent / 0
            {
            uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );
            user.virtualRewards += uint128(virtualRewardsToAdd);
            totalRewards[poolID] += uint128(virtualRewardsToAdd);
            }

        // Update the deposit balances
        user.userShare += uint128(increaseShareAmount);
        totalShares[poolID] = existingTotalShares + increaseShareAmount;

        emit UserShareIncreased(wallet, poolID, increaseShareAmount);
        }

Assume that some users increase UserShare when totalRewards equals 0, totalRewards will not increase at this time, after totalRewards is greater than 0, For these users after decrease UserShare, if they reduced their rewards by an amount equal to totalRewards, it would still result in totalRewards equal to 0.

Let's look at the problems that totalRewards equals 0 can cause:

1. When totalRewards is 0, user.virtualRewards = 0. After adding SALTRewards, the user can get extra userReward.
2. With a decrease in UserShare, since user.virtualRewards equals 0 and virtualRewardsToRemove equals 0, an attacker could obtain the salt token with _decreaseUserShare.
3. A decrease in UserShare by a user could cause totalRewards to go back to 0 (_increaseUserShare,totalRewards will not increase if totalRewards=0 at the first time, but totalRewards will always decrease at _decreaseUserShare).

In addition, the first depository can also use _decreaseUserShare to increase totalRewards to 0. totalShares are 0 on the first deposit, so there will also be no increase in totalRewards when _increaseUserShare. If the first user adds a large number of userShare(greater than totalRewards), that user can use _decreaseUserShare to set totalRewards to 0.

Tools Used

vscode, manual

Recommended Mitigation Steps

Ensure that totalRewards is not 0 when initializing the pool.

Assessed type

Error

[c4-judge]

Picodes marked the issue as duplicate of #614

[c4-judge]

Picodes marked the issue as satisfactory

[zhaojio]

This is not the same problem as #614 : In #614 ,in the initial state totalShares = 0 In this case is totalRewards = 0

uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];

Fixing the problem in 614 will not solve the problem

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

Indeed this is not a duplicate of #614. The report is unclear because addSALTRewards of the staking contract updates totalRewards so it's not clear how rewards end up on this contract.

[zhaojio]

Indeed this is not a duplicate of #614. The report is unclear because addSALTRewards of the staking contract updates totalRewards so it's not clear how rewards end up on this contract.

StakingRewards#addSALTRewards and _increaseUserShare both increase totalRewards, This has no effect on the attacker, who can manipulate totalRewards to equal 0

There are 2 addSALTRewards StakingRewards#addSALTRewards and liquidityRewardsEmitter#addSALTRewards

pool initialization doesn't call StakingRewards#addSALTRewards immediately, so totalRewards could be equal to 0.

[Picodes]

Yes but clearly https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L57 doesn't send SALT to the StakingRewards contract but only stores them in the rewardsEmitter contract.

On a wider scale, please only comment with coded PoCs once your finding has been judged unclear once.