EgisSecurity - SophonFarming.sol - If a pool doesn't have any deposits, after it has started, it will eat up the allocation of points of other pools

[sherlock-admin2]

EgisSecurity

medium

SophonFarming.sol - If a pool doesn't have any deposits, after it has started, it will eat up the allocation of points of other pools

Summary

SophonFarming.sol - If a pool doesn't have any deposits, after it has started, it will eat up the allocation of points of other pools

Vulnerability Detail

The protocol implements a system of allocation points, which basically dictate how much pointsPerBlock each pool has to receive each block, based on the totalAllocPoints.

function updatePool(uint256 _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        if (getBlockNumber() <= pool.lastRewardBlock) {
            return;
        }
        uint256 lpSupply = pool.amount;
        uint256 _pointsPerBlock = pointsPerBlock;
        uint256 _allocPoint = pool.allocPoint;
        if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
            pool.lastRewardBlock = getBlockNumber();
            return;
        }
        uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());

        uint256 pointReward =
            blockMultiplier *
            _pointsPerBlock *
            _allocPoint /
            totalAllocPoint;

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

        pool.lastRewardBlock = getBlockNumber();
    }

The pool.allocPoints represents said allocation points per pool, which are used to calculate the pointReward when updatePool is called. The pool only starts accruing points, after lpSupply != 0, if pool.allocPoints == 0 the the pool is "disabled" and so it doesn't affect other pools.

The issue here is the fact that, totalAllocPoints represent how much are the total pool.allocPoints of each pool, but it doesn't account for the fact, that the pool might not have depositors in it yet.

Let's imagine the following:

1. We have 2 pools and their allocation points are 50/50, while pointsPerBlock = 10, meaning that every block each pool has to accumulate a total of 5 points.
2. Before the pool has started, someone deposits in the first pool, but no one has still deposited in the second pool.
3. 10 block pass and now all the pools must have accrued a total of 100 points, but only pool 1 has accrued any points, it has accrued it's 50.
4. Because the pool.amount = 0 for pool 2, updatePool doesn't do anything as even if it's called, we will go into this if statement and just set lastRewardBlock = block.number.

   if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
           pool.lastRewardBlock = getBlockNumber();
           return;
       }

5. Whenever someone deposits into pool 2, that's when it will start accruing points, thus after said block, all pointsPerBlock will start accruing, not just 50% of them as in the example.

Active pools are punished for the inactive/empty pools and accrue less points because of it.

Note that the variable explicitly states:

    // Points created per block.
    uint256 public pointsPerBlock;

Because of this issue, not all points are created per block.

Impact

Not all pointsPerBlock will be accrued per each block.

Code Snippet

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

Tool used

Manual Review

Recommendation

If there are any inactive pools, don't take them into account when calculating the pointReward for active pools.

[sherlock-admin4]

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

0xmystery commented:

valid because only active pools should be included in totalAllocPoints (best because of more detailed POC)

[RomanHiden]

the issue description doesn't match the issue a little bit. if a pool is inactive its allocation points will not be rewarded.

We have 2 pools and their allocation points are 50/50, while pointsPerBlock = 10, meaning that every block each pool has to accumulate a total of 5 points.
Before the pool has started, someone deposits in the first pool, but no one has still deposited in the second pool.
10 block pass and now all the pools must have accrued a total of 100 points, but only pool 1 has accrued any points, it has accrued it's 50.

pool 1 is entitled to 5 points per block. and it gets in the end 5 points per block 10 blocks = 50. pool 1 earnings doesn't depend on pool1 being active or not. it will always get 50 points

[mystery0x]

The report is trying the stress that pool 1 should justifiably get 100 points instead of 50 points for the first 10 blocks when pool 2 is empty. The denominator, totalAllocPoint , could have been been 50 instead of 100.

[RomanHiden]

That's not our intended design. Pool1 should get 50 points

[mystery0x]

If a fix was made as suggested, this would help mitigate existing issues when _withUpdate was accidentally inputted false in add() and set().

[imp0wd3r]

Escalate

This is not a vulnerability.

As the developer said, this is a reasonable design. It does not mean that if there is only one active pool, the rewards will be given to that pool entirely. A pool getting points does not necessarily mean it can get pointsReward.

the issue description doesn't match the issue a little bit. if a pool is inactive its allocation points will not be rewarded.

We have 2 pools and their allocation points are 50/50, while pointsPerBlock = 10, meaning that every block each pool has to accumulate a total of 5 points.
Before the pool has started, someone deposits in the first pool, but no one has still deposited in the second pool.
10 block pass and now all the pools must have accrued a total of 100 points, but only pool 1 has accrued any points, it has accrued it's 50.

pool 1 is entitled to 5 points per block. and it gets in the end 5 points per block 10 blocks = 50. pool 1 earnings doesn't depend on pool1 being active or not. it will always get 50 points

[sherlock-admin3]

Escalate

This is not a vulnerability.

As the developer said, this is a reasonable design. It does not mean that if there is only one active pool, the rewards will be given to that pool entirely. A pool getting points does not necessarily mean it can get pointsReward.

the issue description doesn't match the issue a little bit. if a pool is inactive its allocation points will not be rewarded.

We have 2 pools and their allocation points are 50/50, while pointsPerBlock = 10, meaning that every block each pool has to accumulate a total of 5 points.
Before the pool has started, someone deposits in the first pool, but no one has still deposited in the second pool.
10 block pass and now all the pools must have accrued a total of 100 points, but only pool 1 has accrued any points, it has accrued it's 50.

pool 1 is entitled to 5 points per block. and it gets in the end 5 points per block 10 blocks = 50. pool 1 earnings doesn't depend on pool1 being active or not. it will always get 50 points

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[aslanbekaibimov]

https://github.com/sherlock-protocol/sherlock-v2-docs/blob/de6519d1cffad7dbae4b3c7a27e9425adf7be3d2/audits/judging/judging/README.md#iii-sherlocks-standards

III. Sherlock's standards: Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

README: no information on how many points should be emitted each block.

COMMENTS:

    // Points created per block.
    uint256 public pointsPerBlock;

This issue shows that in fact significantly less points than pointsPerBlock will be created each block as long as there's a pool without stakers, which is quite likely, as pools can be added after the start of farming.

[imp0wd3r]

The point has not decreased, but having a point does not mean it will be rewarded. Even if an inactive pool is assigned a point, it will not receive actual rewards. Points are used to calculate rewards, but only if this pool can obtain rewards.

if a pool is inactive its allocation points will not be rewarded.

I think this is a reasonable design and it has been clearly explained. Let Sherlock be the judge.

[0xdeth]

I want to quickly explain, why this issue is valid by Sherlock rules, even though it is "design".

Points represent how many tokens each address will receive, during the airdrop phase of the protocol. As @aslanbekaibimov stated, comments are also a source of truth, so pointsPerBlock represents how many points are created for each block.

    // Points created per block.
    uint256 public pointsPerBlock;

If the above mentioned issue happens, not all pointsPerBlock will be accumulated, which is a permanent loss of funds, as the points represent tokens, which are supposed to be, but are not, distributed.

This is a quote from Sherlock's Criteria for Issue Validity

Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.

[mystery0x]

If a fix was made as suggested, this would help mitigate existing issues when _withUpdate was accidentally inputted false in add() and set().

Let me elaborate my earlier reasoning:

1. It's noted that the protocol team plans on fixing add() by making massUpdate() compulsory so that reward distribution will be based on old terms prior to having new terms (i.e. new pool allocation point added to totalAllocPoint) introduced.
2. However, if _withUpdate was accidentally inputted as false in add(), for instance, it would not be a problem if the recommended fix in this report was introduced. This is because the old terms would still be intact and totalAllocPoint remain the same.
3. Refactoring add() is optional contingent on careful input of _withUpdate whereas pointReward complete usage isn't possible with needed code refactoring on updatePool() and _pendingPoints().
4. The contract design already allows depositing beforehand for users standing by to capitalize on full period farming from the very first starting block permissible. Having totalAllocPoint only include allocation points from active pools would further enhance/optimize such capitalization, drawing more users to deposit.

[WangSecurity]

Firstly, if we take the scenario and there had to be 100 points. 50 were allocated to pool1, the other 50 were allocated to pool2. Pool1 has depositors, while Pool2 hasn't. What if someone deposits into the Pool2? Will these 50 points somehow taken into account? Or they're completely gone and Pool2 would have 0 points? Not sure I understand the following line:

Whenever someone deposits into pool 2, that's when it will start accruing points, thus after said block, all pointsPerBlock will start accruing, not just 50% of them as in the example.

Secondly, what if Pool1 has 100 depositors and Pool2 has 1 depositor, the points are still rewarded 50/50, correct? This allocation of 50/50 is just an arbitrary example and it's always even (33/33/33 for 3 pools, 25/25/25/25 for 4 pools)? Isn't it then the incentive to make users deposit into new pools and get more rewards?

[aslanbekaibimov]

@WangSecurity

1. Firstly, if we take the scenario and there had to be 100 points. 50 were allocated to pool1, the other 50 were allocated to pool2. Pool1 has depositors, while Pool2 hasn't. What if someone deposits into the Pool2? Will these 50 points somehow taken into account? Or they're completely gone and Pool2 would have 0 points? Not sure I understand the following line:

Whenever someone deposits into pool 2, that's when it will start accruing points, thus after said block, all pointsPerBlock will start accruing, not just 50% of them as in the example.

50/100 * pointsPerBlock will be distributed to all stakers of pool1 each block - that's the 50%; the other 50% are gone, because pool2 does not have stakers yet - and that's the very point of the report. This is unlikely to happen with the pools which are added few weeks before the start of farming, but almost guaranteed to happen with pools added after the start of farming.

Once pool2 has stakers, they will start receiving the same amount of rewards as pool1 (50/100 * pointsPerBlock each block), and that's the point from which 100% of pointsPerBlock will be created each block.

2. Secondly, what if Pool1 has 100 depositors and Pool2 has 1 depositor, the points are still rewarded 50/50, correct?

100 depositors of Pool1 will receive a half of rewards, and 1 depositor of Pool2 will receive another half.

3.

This allocation of 50/50 is just an arbitrary example and it's always even (33/33/33 for 3 pools, 25/25/25/25 for 4 pools)? Isn't it then the incentive to make users deposit into new pools and get more rewards?

Each pool can have a different allocPoint, but it does not really matter: users are always incentivized to stake in the pool where they can get more rewards per staked USD. If the fix is implemented, the incentive to stake in the most "underrated" pool will still be there.

[0xdeth]

Firstly, if we take the scenario and there had to be 100 points. 50 were allocated to pool1, the other 50 were allocated to pool2. Pool1 has depositors, while Pool2 hasn't. What if someone deposits into the Pool2? Will these 50 points somehow taken into account? Or they're completely gone and Pool2 would have 0 points?

Yes, the points are gone and each block Pool2 doesn't have any deposits, 50 out of the 100 points that are supposed to be allocated per block are lost.

Secondly, what if Pool1 has 100 depositors and Pool2 has 1 depositor, the points are still rewarded 50/50, correct? This allocation of 50/50 is just an arbitrary example and it's always even (33/33/33 for 3 pools, 25/25/25/25 for 4 pools)? Isn't it then the incentive to make users deposit into new pools and get more rewards?

It's an incentive, yes, but there is no guarantee all pools will have depositors when they start. Empty pools will not accumulate any points and this is amplified by each passing block, the active pool is also unfairly punished by still accumulating like Pool 2 has depositors.

[sherlock-admin2]

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

[WangSecurity]

After additional discussions, I would say that this is indeed a design decision and the protocol works as it's expected.

Yes, the comment says Points created per block but there is no problem in creating these points, the problem is that there is no one to distribute these rewards. The design is to give each pool a specific number of points (based on allocPoint), regardless of how many funds are invested into the pool, be that 1000 ETH, be that 1 wei or 0, the depositors of the pool receive the number of rewards based on pool's allocPoint. If there are no depositors, then there's no one to receive the points, then there are no points. I think in that sense, it's a design of the protocol and an incentive to deposit into the pool as soon as it's live to get as many points as possible.

Planning to accept the escalation and invalidate the issue.

[aslanbekaibimov]

@WangSecurity

1. Aren't rewards counted as funds for this contest, and therefore less funds than intended will be distributed? If so, shouldn't it be considered a loss of funds and qualify as a medium, regardless if it's a design choice? E.g. if instead of points there were USDC, and contract had farmingDurationBlocks * pointsPerBlock of USDC, some of these USDC would be effectively locked in the contract / burned.

Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.

How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

2. If there are no depositors, then there's no one to receive the points, then there are no points.

Yes, but this will happen every time a pool is added after the start of farming, as long as people do not set up bots to backrun new pools.

3. an incentive to deposit into the pool as soon as it's live to get as many points as possible.

As I said earlier, the fix does not remove that incentive. There just won't be a drop in distributed rewards per block between the moment the pool is added and the first staker appears.

4. If this is a design choice, why is it being fixed?

[WangSecurity]

Aren't rewards counted as funds for this contest, and therefore less funds than intended will be distributed? If so, shouldn't it be considered a loss of funds and qualify as a medium, regardless if it's a design choice? E.g. if instead of points there were USDC, and contract had farmingDurationBlocks * pointsPerBlock of USDC, some of these USDC would be effectively locked in the contract / burned.

As I understand these points represent the amount of rewards that will be received. As I've said above, the design is to distribute each pool the points based on allocPoints, but if there are no stakers in the pool, then rewards/points are going to nobody. And I believe it's not a loss of funds. There are two pools, for example, they get 50/50 of points each block. Stakers of the first pool get their 50% and stakers of the second pool as well. But, there are no stakers in the second pool, so these points are not going to anyone. Everyone get as much as they're supposed to, so I don't think it's a loss of funds.

Yes, but this will happen every time a pool is added after the start of farming, as long as people do not set up bots to backrun new pools.

Yes, I understand, but I believe it's a strategy to work with the protocol. I see that this argument of course is not strong, but I don't see it strongly supporting any of the sides and is a design.

As I said earlier, the fix does not remove that incentive. There just won't be a drop in distributed rewards per block between the moment the pool is added and the first staker appears.

But, there would be a low weaker incentive to users withdraw from their current pool and deposit into new one. With that drop of rewards for their pool with the emergence of a new one, gives them an instant urge to deposit into new one. With the fix, it would be a much weaker incentive.

If this is a design choice, why is it being fixed?

The fix also confused me, but I confirmed with the team once again that it's a design choice and they said it's intended and expected to work that way. Moreover, we don't know what the fix is.

I hope I addressed all of your points. I want to emphasise that the main reason why it's invalid is cause it's design choice and I don't see it qualifying for a loss of funds as explained in the first paragraph. All the other points are an answer to your points to show my point of view and show that your points are not missed in any case, but they play a rather minor role in this decision relative to the other 2 main ones. Hope for your understanding.

The decision remains the same, accept the escalation and invalidate the report.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• imp0wd3r: accepted