Users will not get their deserved rewards once the farming startBlock has changed.

Summary

Once the farming is initialised and added pools with startBlock having the value of block number in the future. And later , but before the startBlock has reached , if the startBlock is changed to a blockNumber which is lower than the inital value but greater than the current block number , users will lose all their rewards for their userAmount until the initial startBlock reaches.

Vulnerability Detail

The farm is initialised along with the pools by passing future block number for startBlock. The lastRewardBlock for the pool is set to startBlock since we are checking this condition in the function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate).

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L163-L164

        uint256 lastRewardBlock =
            getBlockNumber() > startBlock ? getBlockNumber() : startBlock;

At the end of this function add() we have assigned lastRewardBlock and allocPoint for each pool for the reward Calculation.

The Vulenerability occurs when the startBlock is then changed to a block number which is greater than/equalto curent block.number but lower than initial startBlock using the function setStartBlock(uint256 _startBlock);

After we change the start Block, now we have

pool.lastRewardBlock = initial startBlock number for all the initialised pools , and we expect to update this value once the state of pool the gets changed.

The owner can change the pointsPerBlock using the function setPointsPerBlock(uint256 _pointsPerBlock) and thereby it can save the current state of all the added pools before updating pointsPerBlock.

lets look what happens when the setPointsPerBlock(uint256 _pointsPerBlock) is called.

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L310-L317

    function setPointsPerBlock(uint256 _pointsPerBlock) public onlyOwner {
        if (isFarmingEnded()) {
            revert FarmingIsEnded();
        }
        massUpdatePools();
        pointsPerBlock = _pointsPerBlock;
    }

Here when we are trying to update the pools by calling massUpdatePools() , the function updatePool() will return in the beginning itself without executing the remaining code since we have this condition.

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L413-L415

        if (getBlockNumber() <= pool.lastRewardBlock) { // pool.lastRewardBlock has the value of initial StartBlock
            return;
        }

we have saved the pools initial state and then we are assigning the pointsPerBlock = _pointsPerBlock.

Now a user has deposited to pool 'A' . So we have lpsupply !=0 , pointsPerBlock!=0 and _allocPoint!=0 and expected to calculate the accPointsPerShare of the pool 'A' once the state variables are going to change.

In the next transaction , the owner wants to change the pointsPerBlock again ,

    the expected behaviour => inside setPointsPerBlock(uint256 _pointsPerBlock)  updatePools() will get  executed for all pools and all their current state is saved.And then change the pointsPerBlock value.

    the actual behaviour => inside setPointsPerBlock(uint256 _pointsPerBlock)  updatePools() will get  executed till this check  "if (getBlockNumber() <= pool.lastRewardBlock)" which is true because we havent yet updated the  pool.lastRewardBlock. Hence  pool.accPointsPerShare is not getting updated for all pools.

We can confirm this by executing pendingPoints(uint256 _pid, address _user) for the pool "A" after some blocks and it will return 0 if the current blocknumber < initial startBlock and will return wrong value if current blocknumber > initial startBlock.

Impact

Users will lose all the rewards for all the blocks created before the initial startBlock since the pools' accPointsPerShare is not updated.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L163-L164 https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L310-L317 https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L413-L415

Tool used

Manual Review

Recommendation

When the startBlock is getting changed we need to update the state of all the added pools and then execute

        if (getBlockNumber() < pool.lastRewardBlock) {
            pool.lastRewardBlock = startBlock;
        }

for all the added pools , since startBlock is affecting the pool.lastRewardBlock which is directly affecting the state of the Pool.

Duplicate of #108

sherlock-admin3 commented 1 month ago

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

valid because lastRewardBlock for each pool should indeed sync with the latest startBlock

mystery0x commented 1 month ago

This report should be valid as it described scenario 2 of https://github.com/sherlock-audit/2024-05-sophon-judging/issues/108.

sherlock-audit / 2024-05-sophon-judging

dhank - Users will not get their deserved rewards once the farming startBlock has changed. #201