Users can unstake and then cancel their stake immediately to game the rewards in StakingRewards.sol

[c4-bot-9]

Lines of code

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

Vulnerability details

Proof Of Concept

In Staking.sol, users can call unstake() to withdraw their xSALT. When unstake() is called, _decreaseUserShare() is called in StakingRewards.sol and the user receives some salt

                // 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;

                // Send the claimable rewards
                if ( claimableRewards != 0 )
>                        salt.safeTransfer( wallet, claimableRewards );

Once _decreaseUserShare() executes, the user waits for the unstake duration to recover their salt.

In the Staking contract, there is another function to cancel their unstaked position via cancelUnstake() This function will increase the user share based on what has been unstaked previously through _increaseUserShare(). _increaseUserShare() refills the totalRewards for the pool as well as the virtualRewards of the user.

        function cancelUnstake( uint256 unstakeID ) external nonReentrant
                {
                Unstake storage u = _unstakesByID[unstakeID];

                require( u.status == UnstakeState.PENDING, "Only PENDING unstakes can be cancelled" );
                require( block.timestamp < u.completionTime, "Unstakes that have already completed cannot be cancelled" );
                require( msg.sender == u.wallet, "Sender is not the original staker" );

                // Update the user's share of the rewards for staked SALT
>               _increaseUserShare( msg.sender, PoolUtils.STAKED_SALT, u.unstakedXSALT, false );

                u.status = UnstakeState.CANCELLED;
                emit UnstakeCancelled(msg.sender, unstakeID);
                }

Users with a staked position can call unstake() and cancelUnstake() repeatedly to drain the contract of the rewards.

Impact

Users can drain the rewards of the Staking contract.

Tools Used

Recommended Mitigation Steps

Recommend applying a penalty if the user decides to call cancelUnstake(). Otherwise, users can take advantage of the cancellation to get more rewards.

Also, if there is a proposal that cannot reach quorum, a whale can simply call unstake() and wait for the proposal to pass before calling cancelUnstake() to continue staking his funds.

Assessed type

Context

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Canceling the unstake does not impact the amount of rewards that a user can claim as adding shares only entitles the user to rewards that are added after the shares have been added.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[Picodes]

This report shows how by calling cancelUnstake a user could increase its shares back to their previous level. It opens the doors to users front-running rewards distributions to increase their shares back but it seems to be a desirable behaviour