c4-bot-5 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Users can continue accumulating and claiming rewards after their wallets have been excluded

Proof of Concept

The idea of having authorized users is that they can stake assets to earn rewards.

When some country is excluded, the now unautorized users should still be able to withdraw their assets or repay debts, but not interact in ways that take assets from the protocol, like borrowing, or in this case earning and claiming new rewards.

There are already restrictions implemented for staking SALT, and depositing in the staking pools.

Both of these actions allows users to claim rewards, but the problem is that there is no check in the function to prevent unauthorized users from claiming rewards from previous staking:

    function claimAllRewards(bytes32[] calldata poolIDs) external nonReentrant returns (uint256 claimableRewards) {
        mapping(bytes32 => UserShareInfo) storage userInfo = _userShareInfo[msg.sender];

        claimableRewards = 0;
        for (uint256 i = 0; i < poolIDs.length; i++) {
            bytes32 poolID = poolIDs[i];

            uint256 pendingRewards = userRewardForPool(msg.sender, poolID);

            // Increase the virtualRewards balance for the user to account for them receiving the rewards without withdrawing
            userInfo[poolID].virtualRewards += uint128(pendingRewards);

            claimableRewards += pendingRewards;
        }

        if (claimableRewards > 0) {
            salt.safeTransfer(msg.sender, claimableRewards);
            emit RewardsClaimed(msg.sender, claimableRewards);
        }
    }

StakingRewards.sol#L147-L171

Pending rewards are calculated considering the amount of shares of the user compared to the total shares.

This means that as long as new rewards are deposited for the pools, these unauthorized users will continue to accrue rewards that they can periodically claim.

Also, when withdrawing their collateral, they will still receive in full all the rewards they may have been accumulating after being excluded

Recommended Mitigation Steps

The most straightforward way is adding a check on the claimAllRewards() function to prevent unauthorized wallets to continue claiming rewards, and give unauthorized wallets zero rewards when withdrawing. This way the shares will be reduced, but the total rewards will remain available for the stakers that continue on the platform.

In case excluded wallets are exepected to claim the rewards up to the point when they were excluded, the corresponding rewards for each pool should be stored and tracked whenever the geoVersion is updated, for later distribution.

Assessed type

Other

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #964

c4-judge commented 8 months ago

Picodes changed the severity to QA (Quality Assurance)

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