zzykxx - setStartBlock() doesn't change the block at which already existing pools will start accumulating points

[sherlock-admin2]

zzykxx

high

setStartBlock() doesn't change the block at which already existing pools will start accumulating points

Summary

The function setStartBlock() can be called by the owner to change the block number at which points will start accumulating. When it's called, the block at which already existing pools will start accumulating points will not change. Already existing pools will:

1. Start accumulating points from the old startBlock if the new startBlock is set after the old one.
2. Not accumulate rewards until the old startBlock is reached if the new startBlock is set before the old one.

Vulnerability Detail

This happens because updatePool() considers the pool lastRewardBlock as the block number from which points should start accumulating and setStartBlock() never updates the lastRewardBlock of the already existing pools to the new startBlock.

POC

Runnable POC that showcases point 1 explained above. Can be copy-pasted in SophonFarming.t.sol:

function test_SettingStartBlockDoesntUpdatePools() public {
    address alice = makeAddr("alice");
    uint256 amountToDeposit = sDAI.convertToAssets(1e18);

    vm.prank(alice);
    dai.approve(address(sophonFarming), type(uint256).max);
    deal(address(dai), alice, amountToDeposit);

    //-> Pools original `startBlock` is `1`
    //-> Admin changes `startBlock` to `100`
    vm.prank(deployer);
    sophonFarming.setStartBlock(100);

    //-> Alice deposits at block `90`, which is after the previous `startBlock` (1) but before the current `startBlock` (100)
    vm.roll(90);
    vm.prank(alice);
    sophonFarming.depositDai(amountToDeposit, 0);

    //-> After 9 blocks, at block `99`, Alice has accumulated rewards but she shouldn't have because the current `startBlock` (100) has not been reached yet
    vm.roll(99);
    vm.prank(alice);
    sophonFarming.withdraw(0, type(uint256).max);
    assertEq(sophonFarming.pendingPoints(0, alice), 74999999999999999999);
}

Can be run with:

forge test --match-test test_SettingStartBlockDoesntUpdatePools -vvvvv

Impact

When setStartBlock() is called the block at which already existing pools will start accumulating points will not change.

Code Snippet

Tool used

Manual Review

Recommendation

In setStartBlock() loop over all of the existing pools and adjust each pool lastRewardBlock to the new startBlock. Furthermore setStartBlock() should revert if the new startBlock is lower than the current block.number as this would create problems in points distribution accounting if the above fix is implemented.

[sherlock-admin4]

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 (best because the report is succinct and comprehensive explaining each of the two scenarios)

[CryptoMan0x]

Issue is no longer relevant since we removed the global startBlock setting: sophon-org/farming-contracts@18b59d5