The first staker can get all of the staking rewards

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L82-L137

Vulnerability details

Impact

There are rewards distributed among all users that staked. It is assumed that the rewards would be sent when all of the users staked and will be distributed to each of the users depending on the amount they stake. After that, the users can get their reward with claimAllRewards or unstake.

A problem arises because everyone can call the performUpkeep and send the rewards at any time. The first staker can call the performUpkeep immediately after his stake and all other stakes would be made after the rewards are sent which makes the first staker get all of the rewards.

Proof of Concept

Paste the following test inside src/staking/tests/Staking.t.test

 function testOnlyTheFirstStakerGetsRewards() public {
        vm.prank(alice);
        staking.stakeSALT(50 ether); // Alice balance is 100 and stakes 50

        AddedReward[] memory addedRewards = new AddedReward[](3);
        addedRewards[0] = AddedReward(poolIDs[0], 150 ether);
        staking.addSALTRewards(addedRewards);

        vm.prank(bob);
        staking.stakeSALT(50 ether); // Bob balance is 100 and stakes 50
        vm.prank(charlie);
        staking.stakeSALT(50 ether); // Charlie balance is 100 and stakes 50

        assertEq(salt.balanceOf(alice), 50 ether);
        assertEq(salt.balanceOf(bob), 50 ether);
        assertEq(salt.balanceOf(charlie), 50 ether);

        // Unstake to see who gets what rewards
        // Since everyone staked 50 ether they should get an evenly distributed amount
        // 150 ether rewards / 3 stakers = 50 per staker
        vm.prank(alice);
        staking.unstake(50 ether, 52);
        vm.prank(bob);
        staking.unstake(50 ether, 52);
        vm.prank(charlie);
        staking.unstake(50 ether, 52);

        assertEq(salt.balanceOf(alice), 200 ether); // Alice balance increase since she gets all of the rewards
        assertEq(salt.balanceOf(bob), 50 ether); // Bob doesn't receive any rewards and his balance remains the same
        assertEq(salt.balanceOf(charlie), 50 ether); // Charlie doesn't receive any rewards and his balance remains the same
    }

Tools Used

Manual Review

Recommended Mitigation Steps

I'm not sure what would be the best fix for that problem

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #614

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)