search

sherlock-audit / 2024-05-sophon-judging

7 stars 6 forks source link

yamato - Pools will remain with their old `startTime` if `startTime` is changed to earlier. #72

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

yamato

medium

Pools will remain with their old startTime if startTime is changed to earlier.

Summary

Pools might be forced to start the reward accrual later than supposed to.

Vulnerability Detail

When a pool is added, if startBlock is in the future, the pool's lastRewardBlock gets set to startBlock

        uint256 lastRewardBlock =
            getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
        totalAllocPoint = totalAllocPoint + _allocPoint;
        poolExists[_lpToken] = true;

        uint256 pid = poolInfo.length;

        poolInfo.push(
            PoolInfo({
                lpToken: IERC20(_lpToken),
                l2Farm: address(0),
                amount: 0,
                boostAmount: 0,
                depositAmount: 0,
                allocPoint: _allocPoint,
                lastRewardBlock: lastRewardBlock,
                accPointsPerShare: 0,
                description: _description
            })
        );

The problem is that startBlock value can be changed

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

In the case that startBlock is reduced, pools created prior to that will only start accruing points after the old startBlock passes.

Any new added pool (or adjusted via set) will start accruing points from the new startBlock on.

Impact

Some pools will start accruing rewards later than the others

Code Snippet

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

Tool used

Manual Review

Recommendation

upon changing startBlock, adjust all pools accordingly.

Duplicate of #108

sherlock-admin3 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

sherlock-admin2 commented 5 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/sophon-org/farming-contracts/commit/1f8d4e5ccec63052fcaf751b867294472e2d20a7