serial-coder - Updating the startBlock state variable without affecting existing pools leads to incorrect points/rewards distributions

[sherlock-admin2]

serial-coder

medium

Updating the startBlock state variable without affecting existing pools leads to incorrect points/rewards distributions

Summary

The SophonFarming::setStartBlock() updates the startBlock state variable without affecting existing pools. Consequently, the SophonFarming contract will incorrectly distribute points/rewards to users who stake in those existing pools.

Vulnerability Detail

The startBlock state variable is initialized in the SophonFarming::initialize(). Then, the function creates the predefined pools (sDAI, wstETH, and weETH) by executing the SophonFarming::add().

    function initialize(uint256 ethAllocPoint_, uint256 sDAIAllocPoint_, uint256 _pointsPerBlock, uint256 _startBlock, uint256 _boosterMultiplier) public virtual onlyOwner {
        ...

@1      startBlock = _startBlock; //@audit -- The startBlock state variable gets initialized in the initialize()

        ...

        // sDAI
@2.1    typeToId[PredefinedPool.sDAI] = add(sDAIAllocPoint_, sDAI, "sDAI", false); //@audit -- The initialize() creates sDAI pool by executing the add()
        IERC20(dai).approve(sDAI, 2**256-1);

        // wstETH
@2.2    typeToId[PredefinedPool.wstETH] = add(ethAllocPoint_, wstETH, "wstETH", false); //@audit -- The initialize() creates wstETH pool by executing the add()
        IERC20(stETH).approve(wstETH, 2**256-1);

        // weETH
@2.3    typeToId[PredefinedPool.weETH] = add(ethAllocPoint_, weETH, "weETH", false); //@audit -- The initialize() creates weETH pool by executing the add()
        IERC20(eETH).approve(weETH, 2**256-1);

        ...
    }

• @1 -- The startBlock state variable gets initialized in the initialize(): https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L118
• @2.1 -- The initialize() creates sDAI pool by executing the add(): https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L131
• @2.2 -- The initialize() creates wstETH pool by executing the add(): https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L135
• @2.3 -- The initialize() creates weETH pool by executing the add(): https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L139

The add() initializes the lastRewardBlock variable based on the current block.number or the startBlock variable (depending on which is greater). The lastRewardBlock variable is finally assigned to the created pool.

    function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
        ...

        //@audit -- The add() initializes the lastRewardBlock variable based on the current block.number or the startBlock (depending on which is greater)
@3      uint256 lastRewardBlock =
@3          getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
        
        ...

        poolInfo.push(
            PoolInfo({
                lpToken: IERC20(_lpToken),
                l2Farm: address(0),
                amount: 0,
                boostAmount: 0,
                depositAmount: 0,
                allocPoint: _allocPoint,
@4              lastRewardBlock: lastRewardBlock, //@audit -- The lastRewardBlock is assigned to the created pool
                accPointsPerShare: 0,
                description: _description
            })
        );

        ...
    }

• @3 -- The add() initializes the lastRewardBlock variable based on the current block.number or the startBlock (depending on which is greater): https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L163-L164
• @4 -- The lastRewardBlock is assigned to the created pool: https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L178

The vulnerability resides in the SophonFarming::setStartBlock(), which updates the startBlock state variable without affecting existing pools.

In other words, the updated startBlock state variable will affect only new pools created after. However, this state variable should be globally shared with all pools. Therefore, the SophonFarming contract will distribute points/rewards to users staking in existing pools incorrectly (more or less than the actual depending on the difference between the new and old values of the startBlock variable).

    function setStartBlock(uint256 _startBlock) public onlyOwner {
        if (_startBlock == 0 || (endBlock != 0 && _startBlock >= endBlock)) {
            revert InvalidStartBlock();
        }
        if (getBlockNumber() > startBlock) {
            revert FarmingIsStarted();
        }
@5      startBlock = _startBlock; //@audit -- In the setStartBlock(), the startBlock state variable is updated without taking effect on existing pools
    }

• @5 -- In the setStartBlock(), the startBlock state variable is updated without taking effect on existing pools: https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L279

Coded PoC

The coded PoC is presented below.

Please place the code in the test file: farming-contracts/test/SophonFarming.t.sol. To run the code, execute the command: forge test -vvv --match-test test_PocSettingNewStartBlockNotEffectiveToExistingPools.

The PoC proves that the setStartBlock() updates the startBlock state variable without affecting existing pools.

function test_PocSettingNewStartBlockNotEffectiveToExistingPools() public {
    vm.startPrank(deployer);

    // Initially, the startBlock variable is 1
    assertEq(sophonFarming.startBlock(), 1);

    // The lastRewardBlock of existing pools are all 1 
    // Note that pools' lastRewardBlock was set to 1 as per the startBlock variable in the SophonFarming::add()
    SophonFarmingState.PoolInfo[] memory PoolInfo;
    PoolInfo = sophonFarming.getPoolInfo();
    for (uint256 i = 0; i < PoolInfo.length; i++) {
        assertEq(PoolInfo[i].lastRewardBlock, 1);
    }

    // Set a new value of 2001 (block.number + 2000) to the startBlock variable
    uint256 newStartBlock = block.number + 2000;
    sophonFarming.setStartBlock(newStartBlock);

    // Now, the startBlock variable becomes 2001
    assertEq(sophonFarming.startBlock(), newStartBlock);

    // However, the lastRewardBlock of all existing pools remains unchanged
    PoolInfo = sophonFarming.getPoolInfo();
    for (uint256 i = 0; i < PoolInfo.length; i++) {
        assertEq(PoolInfo[i].lastRewardBlock, 1); // Each pool's lastRewardBlock still remains 1
    }
}

Impact

The setStartBlock() updates the startBlock state variable without affecting existing pools. Consequently, the SophonFarming contract will incorrectly distribute points/rewards to users staking in existing pools.

In more detail, the updated startBlock state variable will affect only new pools created after. However, this state variable should be globally shared with all pools. Therefore, the SophonFarming contract will distribute points/rewards to users staking in existing pools incorrectly (more or less than the actual depending on the difference between the new and old values of the startBlock variable).

Code Snippet

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

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

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

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

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

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

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

Tool used

Manual Review

Recommendation

Apply the updated startBlock state variable on all existing pools in the setStartBlock().

Duplicate of #108

[sherlock-admin3]

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

[mystery0x]

This report should be valid as it described both scenarios 1 and 2 of #108.