Audinarey - New pool cannot be added to the farm if the `lpSupply` of at least one pool is zero

[sherlock-admin3]

Audinarey

high

New pool cannot be added to the farm if the lpSupply of at least one pool is zero

Summary

In order to add a pool to the farm, the owner calls SophonFarming::add(...). If this call is made with the option (_withUpdate = true) to update the accounting for all pools, then the massUpdatePools() function is called to loop over and update accounting for all pools.

File: SophonFarming.sol
153:     function add(..., bool _withUpdate) public onlyOwner {
...
160:  @>     if (_withUpdate) {
161:  @>         massUpdatePools(); // @audit if the is called when the lpSupply of any pool is zero, it will return without updating the parameters

The massUpdatePools() function loops over ALL the pools and calls the updatePool(...) for each of the pools. However, the updatePool(...) function returns and ends execution if it encounters any empty pool without updating the allocation point for the pool of interest even if it's own lpSupply is greater than zero.

File: SophonFarming.sol
411:     function updatePool(uint256 _pid) public {
412:         PoolInfo storage pool = poolInfo[_pid];
413:         if (getBlockNumber() <= pool.lastRewardBlock) {
414:             return;
415:         }
416:         uint256 lpSupply = pool.amount;
417:         uint256 _pointsPerBlock = pointsPerBlock;
418:         uint256 _allocPoint = pool.allocPoint;
419:  @>     if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
420:             pool.lastRewardBlock = getBlockNumber();
421:  @>         return;
422:         }

Vulnerability Detail

• Consider 3 pools lp1, lp2 and lp3 respectively with lpSupply 1000lp1, 0lp2, 1000lp3
• admin calls add(...) for lp4 to add a 4th pool
• as seen on L419 above, the call to add will return and end because the lpSupply of lp2 is zero.
• hence the lp4 is not added to the pools in the farm

Impact

• This breaks core protocol functionality as the lpSupply of one pool affects adding another pool.

Code Snippet

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

Tool used

Manual Review

Recommendation

Modify the add(...) function to update only the pool whose allocation point is being set as shown below:

File: SophonFarming.sol
153:     function add(..., bool _withUpdate) public onlyOwner {
...
160:  -      if (_withUpdate) {
161:  -          massUpdatePools(); // @audit if the is called when the lpSupply of any pool is zero, it will return without updating the parameters

[sherlock-admin4]

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

0xmystery commented:

invalid because massUpdatePools() will not revert and still update as intended

[Audinarey]

Escalate

invalid because massUpdatePools() will not revert and still update as intended

your conclusion is totally wrong because

• no where in this report did I mention that the massUpdatePools() will revert
• the report mentions that the empty pool will cause the add(...) function to end/terminate execution because it encounters a return statement in the updatePool(...) as shown on L421 without actually adding the pool

Kindly look again at Summary and Vulnerability Detail again

[sherlock-admin3]

Escalate

invalid because massUpdatePools() will not revert and still update as intended

your conclusion is totally wrong because

• no where in this report did I mention that the massUpdatePools() will revert
• the report mentions that the empty pool will cause the add(...) function to end/terminate execution because it encounters a return statement in the updatePool(...) as shown on L421 without actually adding the pool

Kindly look again at Summary and Vulnerability Detail again

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.

[mystery0x]

L419 is meant solely to ensure that pool.lastRewardBlock will be updated with getBlockNumber() if lpSupply == 0 (one of the 3 conditions in the if block).

massUpdatePools() in this case will still have pools 1-3 updated, and then add() will have pool 4 added to the array poolInfo. Everything works flawlessly as intended.

[Audinarey]

You are mistaking a return for a continue statement

This is the case of an early return issue that breaks core protocol functionality. the return statement COMPLETES the execution of a function call. Quite alright pool.lastRewardBlock will be updated but the call end when the return is hit hence L163 to L186 is never reached in the scenario described in this finding

For reference, copy the following into remix to understand how return works in solidity

// SPDX-License-Identifier: GPL-3.0

pragma solidity ^0.8.17;

contract Attacker{
    address public pre;
    uint256 public trim;

    constructor () {}

    function changeParam() public {

        for (uint8 i = 0; i < 3; i++) {
            if (i == 2) {
                return;
            }
        }

        pre = address(type(uint160).max);
        trim = 700;
    }

    function getPre() public view returns(address, uint256) {
        return (pre, trim);
    }
}

Notice that pre and trim are never updated as long as the return is hit in the loop,

Therefore as explained in this and other separate findings, immediately one of the pools that is being looped over in massUpdatePools() has an lpSuppy = 0 is encountered, pool.lastRewardBlock is updated and the call completes, but the pool is never added because the push command is never reached and the pool is not added.

[Audinarey]

Hence this finding is valid, because the implementation of the add(...) function breaks core protocol functionality

I implore you to please read the summary and vulnerability detail sections again.

[Audinarey]

@mystery0x I await your response

[WangSecurity]

@Audinarey firstly, please be more respectful, calm and patient.

Secondly, I modified your test to actually show how the return would act in that case:

// SPDX-License-Identifier: GPL-3.0

pragma solidity ^0.8.17;

contract Attacker{
    address public pre;
    uint256 public trim;

    constructor () {}

    function changeParam() public {

        for (uint8 i = 0; i < 3; i++) {
            if (i == 2) {
                testReturn();
            }
        }

        pre = address(type(uint160).max);
        trim = 700;
    }

    function testReturn() public pure {
        return;
    }

    function getPre() public view returns(address, uint256) {
        return (pre, trim);
    }
}

In the Sophons code, we call massUpdatePools and loop over each pool calling updatePool. Then the updatePool hits return. With that return only updatePool stops, but massUpdatePool and add won't return and will go on with their execution and add a new pool.

In your example, the return is hit directly in the function we call, so it stops. But, if you take my version, the loop calls another function, and it hits return, but changeParam will continue execution and change pre and trim.

If you think this is incorrect and Sophon's code will not add the new pool, make a POC on sophons contracts, not an arbitrary test.

Planning to reject the escalation and leave the issue as it is, since the function works as expected and will add a new pool.

[Audinarey]

Apologies if I came off sounding harsh.

Thanks for the clarification.

I accept your decision

[WangSecurity]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Audinarey: rejected

[WangSecurity]

Result: Invalid Unique