araj - Updating `startBlock` in `SophonFarming::setStartBlock()` leads to wrong rewardPoints calculation

[sherlock-admin2]

araj

medium

Updating startBlock in SophonFarming::setStartBlock() leads to wrong rewardPoints calculation

Summary

Updating startBlock in SophonFarming::setStartBlock() leads to wrong rewardPoints calculation because it doesn't update the pool.lastRewardBlock

Vulnerability Detail

Lets go step by step:-

1. Suppose pool is initialize() with startBlock = block.number + 100(ie in future), which will set the pool.lastRewardBlock of all pools to startBlock ie block.number + 100

   function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
    ....
       uint256 lastRewardBlock =
           getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
     ....
   }

2. Now owner changed the startBlock to block.number + 50(ie wanted to start early)

   function setStartBlock(uint256 _startBlock) public onlyOwner {
     ....
       if (getBlockNumber() > startBlock) {
           revert FarmingIsStarted();
       }
       startBlock = _startBlock;
   }

3. User deposited 100e18 at startBlock(block.number + 50)
4. Now, user should get the rewardPoints for depositing from block.number + 50(startBlock) but will not get until block.number + 100(ie lastRewardBlock) because reward is calculated from lastRewardBlock & lastRewardBlock is not updated in setStartBlock

//Here is coded POC

 function test_setStartBlockLeadsToWrongCalculation() public {
        //Set startBlock = block.number + 100 in setUp()
        console.log("Initial startBlock:", sophonFarming.startBlock());

        uint256 poolId = sophonFarming.typeToId(
            SophonFarmingState.PredefinedPool.wstETH
        );
        SophonFarmingState.PoolInfo[] memory PoolInfo;
        PoolInfo = sophonFarming.getPoolInfo();
        console.log(
            "Initial lastRewardBlock:",
            PoolInfo[poolId].lastRewardBlock
        );

        //Setting startBlock = block.number + 50(ie starting early)
        vm.prank(deployer);
        sophonFarming.setStartBlock(block.number + 50);
        console.log("After changing startBlock:", sophonFarming.startBlock());

        //User deposits 100e18 at startBlock ie block.number + 50
        uint amountToDeposit = 100e18;
        vm.deal(account1, amountToDeposit);
        vm.startPrank(account1);

        vm.roll(block.number + 51);
        sophonFarming.depositEth{value: amountToDeposit}(
            0,
            SophonFarmingState.PredefinedPool.wstETH
        );
        //Reward accumulated from block.number 50(startBlock) to 100(lastRewardBlock) is zero
        vm.roll(block.number + 49);
        console.log(
            "Reward accu from startBlock to lastRewardBlock:",
            sophonFarming.pendingPoints(poolId, account1)
        );
    }

Results:-

[PASS] test_rewardSettledIsGamed() (gas: 282450)
Logs:
  Initial startBlock: 101
  Initial lastRewardBlock: 101
  After changing startBlock: 51
  Reward accu from startBlock to lastRewardBlock: 0

Impact

Users will loss on rewardPoints

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L272C4-L280C6 https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L153C5-L187C6

Tool used

Manual Review

Recommendation

Update lastRewardBlock of all pools when startBlock is changed

Duplicate of #108

[sherlock-admin2]

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]

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