First depositor of a new pool can manipulate the totalRewards balance, making subsequent depositors unable to remove liquidity or earn rewards

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Users cannot earn rewards from the pool. Also, users cannot withdraw their liquidity once they deposit since _decreaseUserShare will always fail as claimableRewards will be too big.

Explanation

When a user deposits liquidity into a pool, _increaseUserShare() will be called and the totalRewards, totalShares, virtualRewards and userShares will be updated.

                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);
                }

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

Take note that only if existingTotalShares is not zero, then the virtual rewards will be calculated.

When a pool is first whitelisted and created, there is no default liquidity deposited into the pool.

The first depositor can deposit 1 wei of share (or rather, a small amount of share, will be explained later) so that the existingTotalShare becomes a small number. Then, he can deposit a huge liquidity so that the totalRewards of the pool will become a huge amount. Subsequently, anyone who deposits in the same pool will also have more rewards than intended.

This assumes that the pool has some salt reward added to it in the first place, or if the depositor adds the reward himself.

1 wei of added share is not possible because of the check of dust amount when adding liquidity. The minimum added share would then be 200 (100 wei each)

        function addLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 totalLiquidity ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
                {
                require( msg.sender == address(collateralAndLiquidity), "Pools.addLiquidity is only callable from the CollateralAndLiquidity contract" );
                require( exchangeIsLive, "The exchange is not yet live" );
                require( address(tokenA) != address(tokenB), "Cannot add liquidity for duplicate tokens" );

>               require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
>               require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

Proof of concept

1. The pool is whitelisted, no one added any liquidity yet
2. The first depositor or someone else adds rewards to the pool, say 10000e18 SALT.
3. The first depositor adds in 200 wei of shares to the pool. Since the existingTotalShares is 0, the totalRewards part is skipped.
4. The first depositor then adds 10e18 worth of shares to the pool (say the pool tokens is in 18 decimals). The totalRewards becomes 10000e18 * 10e18 / 100 which is 1000e36. The totalRewards become 1000e36.
5. ExistingTotalShares is updated to 10e18 + 200. The totalRewards variable is now manipulated already

The pool is now unable to earn rewards because totalRewards is too big.

Also, users will find themselves adding liquidity into the pool but unable to withdraw any liquidity since the value of totalRewards is too big. As a result, the value of claimableRewards will be too big as well, which will lead to a revert when transferring salt tokens.

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

Tools Used

VSCode

Recommended Mitigation Steps

Recommend the protocol start out with an initial liquidity for all whitelisted pool to prevent users from manipulating the totalRewards variable. 1e18 worth of pool tokens will be enough to start, and the DAO can pool the liquidity from the Protocol-owned liquidity.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as duplicate of #341

[c4-judge]

Picodes marked the issue as satisfactory