search

sherlock-audit / 2024-05-sophon-judging

1 stars 1 forks source link

0xAadi - Incorrect Reward Calculation Due to Unupdated `lastRewardBlock` When `startBlock` is Reduced or Increased #205

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

0xAadi

medium

Incorrect Reward Calculation Due to Unupdated lastRewardBlock When startBlock is Reduced or Increased

Summary

The SophonFarming contract has a vulnerability where reducing/Increasing the startBlock after pools have been created does not update the lastRewardBlock for those pools. This can lead to incorrect reward calculations, as the _getBlockMultiplier function will use the outdated lastRewardBlock.

Vulnerability Detail

When the startBlock is reduced, the lastRewardBlock for existing pools is not updated to reflect the new startBlock. This misalignment causes the _getBlockMultiplier function to calculate rewards based on an outdated lastRewardBlock, leading to incorrect reward distribution.

Impact

Code Snippet

The issue lies in the setStartBlock function:

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

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

Tool used

Manual Review

Recommendation

Update the lastRewardBlock for all pools when the startBlock is changed. Here is a suggested fix:

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

    // Update lastRewardBlock for all pools
    for (uint256 pid = 0; pid < poolInfo.length; pid++) {
        PoolInfo storage pool = poolInfo[pid];
        if (pool.lastRewardBlock < _startBlock) {
            pool.lastRewardBlock = _startBlock;
        }
    }
}

Duplicate of #108

sherlock-admin2 commented 1 month 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 1 month ago

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