Lack of overflow protection / low precision can allow attacker to steal SALT rewards

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L57-L92 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L131-L135 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L102-L107

Vulnerability details

Impact

There is a general issue with the casting of virtualRewardsToAdd in StakingRewards:_increaseUserShare to the lower precision uint128, as this makes it likely that an overflow can happen. Using this, an attacker can deposit a large amount of LP while having their virtualRewards not increase at all. This then means that they can steal whatever amount is currently cached in totalRewards[poolID].

Proof of Concept

As I stated in another submitted issue, there is a general problem in which attackers can snipe newly whitelisted pools to get up to 1% of the emissions rewards (2_000e18 SALT by default). In this attack they will deposit 101 wei of both tokens into the pool. Therefore, we will consider this our starting state - in particular lets say stETH was newly added and this snipe has already occurred. This means theres 101 stETH, 101 WETH, totalShares[poolID]=202, and totalRewards[poolID]=2_000e18.

Let's now assume userA is depositing WETH & stETH into this pool to steal rewards. Ultimately Liquidity:_depositLiquidityAndIncreaseShare is run which has the following logic:

...
(addedAmountA, addedAmountB, addedLiquidity) = pools.addLiquidity( tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]);
_increaseUserShare( msg.sender, poolID, addedLiquidity, true );
...

The addedLiquidity is calculated by Pools:addLiquidity, which ultimately calls Pools:_addLiquidity, which has the following logic for calculating the amount of LP issued to the user:

...
if ( addedAmount0 > addedAmount1)
   addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
else
   addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;
}
...

After this, StakingRewards:_increaseUserShare is used to calculate the virtualRewards changes:

...
uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

user.virtualRewards += uint128(virtualRewardsToAdd);
totalRewards[poolID] += uint128(virtualRewardsToAdd);
...

Now that we've seen all the relevant equations, let's calculate what deposit of WETH and stETH will allow the attacker to overflow the virtualRewardsToAdd calculation to get user.virtualRewards=0. We have the following set of equations (added WETH and stETH will be the same):

> 202 * addedLiq / 101 = x
> type(uint128).max+1 = 2_000e18 * x / 202

> type(uint128).max+1 = 2_000e18 * (202 * addedLiq / 101) / 202

> type(uint128).max+1 = 4_000e18 * addedLiq / 202

> 202*(type(uint128).max+1)/4000e18 = addedLiq

> addedLiq = ~17.18e18

Therefore, the attacker can add ~17.18e18 of WETH and stETH to this pool to cause uint128(virtualRewardsToAdd) to overflow and equal 0. When this occurs (and since the user effectively LPed the entire pool, they will be able to withdraw ~2_000e18 SALT, or whatever the current totalRewards[poolID] amount is). You can see this in the StakingRewards:userRewardForPool logic:

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

Assessed type

Math

[c4-judge]

Picodes marked the issue as duplicate of #341

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory