JrNet - Design Issue: add() and set() relay on owner to update all pools can cause reward (points) loss.

[sherlock-admin4]

JrNet

medium

Design Issue: add() and set() relay on owner to update all pools can cause reward (points) loss.

Summary

_withUpdate essentially allowing owners to skip the updation of pool.accPointsPerShare value but increase totalAllocPoints. However this is a tough design decision to choose whether

1. Giving owner the responsibility of adding the correct alloc points or
2. To protect the add/set functions from bricking (as if we remove _withUpdate and on every call we do massUpdatePool, when a new pool gets added, it should iterate though poolInfo[] which can get quite larger and cause gas errors. Also once a pool added it can't be removed. It only allows pool by to reset by updating pool.allocPoints. On pools getting added it grows and brick the system)

If owner compromised or malicious this can go bad on rewards distribution system. As totalAllocPoints increases the pool.accPointsPerShare decreases and users face loss of rewards in scale.

Refer issue IDX-003 in LuckyLion_Farm_Audit_Report

Vulnerability Detail

State Variable totalAllocPoint is used to determine the portion that each pool would get from the total reward and it is one of the main consideration in the rewards calculation.

         uint256 pointReward =
                blockMultiplier *
                pointsPerBlock *
                pool.allocPoint /
@>           totalAllocPoint;

@>   pool.accPointsPerShare = pointReward / lpSupply + pool.accPointsPerShare;

When a pool gets added with _withUpdate=false it only increases the totalAllocPoint values

function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
        ...
        if (_withUpdate) {
            massUpdatePools();
        }
        uint256 lastRewardBlock = getBlockNumber() > startBlock ? getBlockNumber() : startBlock; 
  @>    totalAllocPoint = totalAllocPoint + _allocPoint; // only increases 
        poolExists[_lpToken] = true;

        uint256 pid = poolInfo.length;

        poolInfo.push(
            PoolInfo({
               ...
            })
        );

This shows it is crucial to set removing _withUpdate (_withUpdate=true) irrespective of owner decision to prevent reward (points) loss to user.

Impact

Loss of rewards (points)

Code Snippet

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

Tool used

Manual Review

Recommendation

Remove the _withUpdate in the add() and set() functions and always call the massUpdatePools() function before updating totalAllocPoint variable and design the protocol to remove or avoid gas errors to mitigate the risks.

[sherlock-admin3]

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.