Rewards are not distributed correctly, causing users to lose rewards

Summary

Rewards are calculated based off the number of blocks users have deposited for. The problem is that the lastRewardBlock is updated globally for all users rather than for individual users, causing rewards to be distributed incorrectly.

Vulnerability Detail

Let's assume Bob and Alice each have deposited 5e18 wstETH into the wstETH pool.

Assume the following values:

pool.allocPoint = 1000 totalAllocPoint = 10000 wstETH pool.amount = 10e18 (5e18 each) pointsPerBlock = 2e18 pool.accPointsPerShare = 2e18 user.rewardSettled == user.rewardDebt

Assume 10 blocks have passed since both users have deposited their tokens. Bob decides to withdraw his 5e18 wstETH.

SophonFarming.sol#L699-L742

    function withdraw(uint256 _pid, uint256 _withdrawAmount) external {
        if (isWithdrawPeriodEnded()) {
            revert WithdrawNotAllowed();
        }
        if (_withdrawAmount == 0) {
            revert WithdrawIsZero();
        }

        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
@>      updatePool(_pid);

        uint256 userDepositAmount = user.depositAmount;

        if (_withdrawAmount == type(uint256).max) {
            _withdrawAmount = userDepositAmount;
        } else if (_withdrawAmount > userDepositAmount) {
            revert WithdrawTooHigh(userDepositAmount);
        }

        uint256 userAmount = user.amount;
        user.rewardSettled =
            userAmount *
            pool.accPointsPerShare /
            1e18 +
            user.rewardSettled -
            user.rewardDebt;

        user.depositAmount = userDepositAmount - _withdrawAmount;
        pool.depositAmount = pool.depositAmount - _withdrawAmount;

        userAmount = userAmount - _withdrawAmount;

        user.amount = userAmount;
        pool.amount = pool.amount - _withdrawAmount;

        pool.lpToken.safeTransfer(msg.sender, _withdrawAmount); 

        user.rewardDebt = userAmount *
            pool.accPointsPerShare /
            1e18;

        emit Withdraw(msg.sender, _pid, _withdrawAmount);
    }

Firstly, the pool accounting is updated.

SophonFarming.sol#L411-L435

    function updatePool(uint256 _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        if (getBlockNumber() <= pool.lastRewardBlock) {
            return;
        }
        uint256 lpSupply = pool.amount;
        uint256 _pointsPerBlock = pointsPerBlock;
        uint256 _allocPoint = pool.allocPoint;
        if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
            pool.lastRewardBlock = getBlockNumber();
            return; 
        }
        uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
        uint256 pointReward =
            blockMultiplier *
            _pointsPerBlock *
            _allocPoint /
            totalAllocPoint;

        pool.accPointsPerShare = pointReward / 
            lpSupply +
            pool.accPointsPerShare;

@>      pool.lastRewardBlock = getBlockNumber(); //@audit next withdrawer will suffer, since rewards points depend on this value
    }

Note that at the end of the function, pool.lastRewardBlock is set to the current block.number. Since rewards are distributed based off how many blocks users have deposited for, this will make the next user lose a majority of their rewards. The correct solution is to update the lastRewardBlock for individual users, rather than for all users at once.

Continuing with the example:

Recall that 10 blocks have passed, therefore blockMultiplier = 10e18:

SophonFarming.sol#L344-L346

   if (_to > _from) {
            return (_to - _from) * 1e18; 
        }

We have pointReward = 10e18 * 2e18 * 1000 / 10000 = 2000000000000000000000000000000000000 => pool.accPointsPerShare = 2000000000000000000000000000000000000 / 10e18 + 2e18 = 2200000000000000000

Now, back to withdraw function.

user.rewardSettled = userAmount * pool.accPointsPerShare / 1e18 => user.rewardSettled = 5e18 * 2200000000000000000 / 1e18 = 11000000000000000000

Bob's reward points are settled at 11e18.

Assume 1 block has passed (total of 11 blocks since Alice's first deposit), now Alice decides to withdraw her 5e18 wstETH.

Again, a call to updatePool() first.

Since pool.lastRewardBlock was just updated 1 block ago during Bob's withdrawal, blockMultiplier is now 1e18. This is incorrect, as Alice has deposited for 11 blocks, which should give a blockMultiplier value of 11e18.

The problem was that when Bob called withdraw, the pool.lastRewardBlock was updated to the block.number at that time, and the blockMultiplier is calculated based off how many blocks have passed since pool.lastRewardBlock.

The changes to lastRewardBlock in that case should have only applied to Bob, not Alice. Now Alice only gets rewarded for depositing for 1 block, rather than the full 11 blocks, whereas Bob got rewarded for the full 10 blocks.

Alice in this case suffers and loses rewards.

Impact

Loss of rewards for users.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L699-L742

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L411-L435

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L344-L346

Tool used

Manual Review

Recommendation

The recommendation is to update the lastRewardBlock for individual users, for example:

    function updatePool(uint256 _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
+       UserInfo storage user = userInfo[_pid][msg.sender];
-       if (getBlockNumber() <= pool.lastRewardBlock) {
+       if (getBlockNumber() <= user.lastRewardBlock) {
            return;
        }
        uint256 lpSupply = pool.amount;
        uint256 _pointsPerBlock = pointsPerBlock;
        uint256 _allocPoint = pool.allocPoint;
        if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
-           pool.lastRewardBlock = getBlockNumber();
+           user.lastRewardBlock = getBlockNumber();
            return; 
        }
-       uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
+       uint256 blockMultiplier = _getBlockMultiplier(user.lastRewardBlock, getBlockNumber());
        uint256 pointReward =
            blockMultiplier *
            _pointsPerBlock *
            _allocPoint /
            totalAllocPoint;

        pool.accPointsPerShare = pointReward / 
            lpSupply +
            pool.accPointsPerShare;

-       pool.lastRewardBlock = getBlockNumber();
+       user.lastRewardBlock = getBlockNumber();
    }

Of course, this is a change that would require reimplementation of many of the core functionalities. But this way, users will have the correct lastRewardBlock updated for their respective reward. Coming back to the example from above, after Bob's withdrawal, Alice's lastRewardBlock would correctly be 11 blocks ago and only Bob's lastRewardBlock would be 1 block ago.

sherlock-audit / 2024-05-sophon-judging

cryptic - Rewards are not distributed correctly, causing users to lose rewards #47