sherlock-audit / 2024-05-sophon-judging

7 stars 6 forks source link

MightyRaju - lastRewardBlock Of Each Pool Should Be Updated With startBlock Is Updated #141

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

MightyRaju

medium

lastRewardBlock Of Each Pool Should Be Updated With startBlock Is Updated

Summary

It is possible that the startblock has been assigned to a future block in the initialize function. Also , lastRewardBlock is assigned to this start block in the add function (called through initialize) . When the startBlock is updated (let's say it is assigned to 10 blocks earlier now) via the setStartBlock function , we can see that lastrRewardBlock for each pool would point to the older startBlock and is not updated to the new startBlock . Therefore in this case the pool would start accruing rewards 10 blocks later. PoC attached

Vulnerability Detail

It would be best understood with a PoC , In the PoC ->

1.) Paste the PoC in the SophonFarming.t.sol , and edit the line L125 to startBlock = block.number + 30;

2.) We first assign startBlock to 30 block in the future , therefore lastRewardBlock can be seen as 31

3.) In the PoC , we update the startBlock to 25 blocks in the future , therefore for each pool the lastRewardBlock should be now 26 , But it is 31 since it was not updated.


function test_PoC() public {

        uint256 amountToDeposit1 = 100e18;
        uint256 poolId1 = sophonFarming.typeToId(SophonFarmingState.PredefinedPool.wstETH);

        vm.startPrank(account1);
        deal(address(wstETH), account1, amountToDeposit1);

        wstETH.approve(address(sophonFarming), amountToDeposit1);

        vm.stopPrank();

        vm.prank(deployer);
        sophonFarming.setStartBlock(block.number + 25);

        sophonFarming.getPoolInfo();

    }

Run the test with -vvvvv and you'll see the lastRewardBlock is still 31 , This means the pools would still accrue rewards from block 31 rather than 26 which is loss of points/rewards.

Impact

Incorrect lastRewardBlock for pools would lead to incorrect rewards accrual.

Code Snippet

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

Tool used

Foundry , Manual Review

Recommendation

Update the lastRewardBlock for each pool in the setStartBlock function

Duplicate of #108

sherlock-admin4 commented 5 months 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 5 months ago

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