0xShoonya - Improper reward points calculation (withUpdate)

[sherlock-admin2]

0xShoonya

high

Improper reward points calculation (withUpdate)

Summary

The add() and set() functions in the Sophon contract present a vulnerability due to the improper handling of reward points calculation when the totalAllocPoint variable is modified without updating the pending reward points first. If the _withUpdate parameter is set to false, the massUpdatePools() function, responsible for updating pending rewards, is not called. Consequently, the reward calculation for each pool may become incorrect.

Vulnerability Detail

The totalAllocPoint variable is used to determine the portion that each pool would get from the total reward pints minted, so it is one of the main factors used in the reward points calculation. Therefore, whenever the totalAllocPoint variable is modified without updating the pending reward points first, the reward of each pool will be incorrectly calculated.

In the add() and set() functions shown below, if _withUpdate is set to false, the totalAllocPoint variable will be modified without updating the rewards (massUpdatePools()).

    function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
        if (poolExists[_lpToken]) {
            revert PoolExists();
        }
        if (isFarmingEnded()) {
            revert FarmingIsEnded();
        }
        if (_withUpdate) {
            massUpdatePools();
        }
        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
            })
        );

        emit Add(_lpToken, pid, _allocPoint);

        return pid;
    }

    function set(uint256 _pid, uint256 _allocPoint, bool _withUpdate) public onlyOwner {
        if (isFarmingEnded()) {
            revert FarmingIsEnded();
        }
        if (_withUpdate) {
            massUpdatePools();
        }

        PoolInfo storage pool = poolInfo[_pid];
        address lpToken = address(pool.lpToken);
        if (lpToken == address(0) || !poolExists[lpToken]) {
            revert PoolDoesNotExist();
        }
        totalAllocPoint = totalAllocPoint - pool.allocPoint + _allocPoint;
        pool.allocPoint = _allocPoint;

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

        emit Set(lpToken, _pid, _allocPoint);
    }

Impact

This vulnerability can lead to inaccurate reward distribution among pools, potentially causing unfairness and inconsistency in the rewards received by users. Pools with updated allocation points may receive more rewards than intended, while pools with outdated allocation points may receive fewer rewards than expected.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L153-L187 https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L195-L216

Tool used

Manual Review

Recommendation

Remove the _withUpdate variable in the add() and set() functions and always call the massUpdatePools() function before updating totalAllocPoint variable.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

invalid because it would require the admin to make a mistake by inputting a false boolean

[sherlock-admin2]

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

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.