ZdravkoHr. - SophonFarming.updatePool doesn't check if the farming has started

[sherlock-admin3]

ZdravkoHr.

medium

SophonFarming.updatePool doesn't check if the farming has started

Summary

The contract should start accumulating rewards after the current block.number passes the startBlock. However, the function that accrues rewards does not check this.

Vulnerability Detail

In the initialize function the startBlock is set and the predifined pools are added.

        startBlock = _startBlock;
        ...
        typeToId[PredefinedPool.sDAI] = add(sDAIAllocPoint_, sDAI, "sDAI", false);
        typeToId[PredefinedPool.wstETH] = add(ethAllocPoint_, wstETH, "wstETH", false);
        typeToId[PredefinedPool.weETH] = add(ethAllocPoint_, weETH, "weETH", false);

When adding the pool, the lastRewardBlock is set to the bigger value of the numbers of the current block and the startBlock.

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

Then, in updatePool() it's checked that we are past the lastRewardBlock. This is a sufficient check to ensure rewards are not distributed before the initial startBlock.

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

However, the owner should be able to update the startBlock by calling setStartBlock()

    function setStartBlock(uint256 _startBlock) public onlyOwner {
        if (_startBlock == 0 || (endBlock != 0 && _startBlock >= endBlock)) {
            revert InvalidStartBlock();
        }
        if (getBlockNumber() > startBlock) {
            revert FarmingIsStarted();
        }
        startBlock = _startBlock;
    }

When the owner updates the value of the startBlock, the pools' lastRewardTime will not be synced. Since updatePool checks only lastRewardTime and not the startBlock, reward distribution will start earlier than expected.

Impact

Reward distribution starts earlier. It's like setStartBlock was never called.

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#L413C1-L415C10
https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L272-L280

Tool used

Manual Review

Recommendation

Check the startBlock as well

-        if (getBlockNumber() <= pool.lastRewardBlock) {
+        if (getBlockNumber() <= pool.lastRewardBlock || getBlockNumber <= startBlock) {
            return;
        }

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

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
sophon-org/farming-contracts@1f8d4e5