search

sherlock-audit / 2024-05-sophon-judging

1 stars 1 forks source link

4th05 - `endBlock` can be set equal to `0` once the `farm` is ongoing #191

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

4th05

high

endBlock can be set equal to 0 once the farm is ongoing

Summary

endBlock can be changed to be 0 when the Farm is ongoing. This should never be allowed as by definition and condition should be: 

 if (_startBlock == 0) {
            revert InvalidStartBlock();
        }
function setStartBlock(uint256 _startBlock) public onlyOwner {
        if (_startBlock == 0 || (endBlock != 0 && _startBlock >= endBlock)) {
            revert InvalidStartBlock();
        }

Vulnerability Detail

The setEndBlock function allows in its if condition to change the endBlock to 0 for all ongoing pools and so for all the users of the contract. Changing the endBlock to 0 should not be allowed because pools have been already started.

Impact

Changing the endBlock to 0 once the Farm is ongoing could impact the pending points of all the users on all pools because it changes the pool.lastRewardBlock to the current block.number on all the pools impacting on the BlockMultiplier every time the pendingPoints function is called. Therefore, a wrong pending points calculation is provided to the users for every pool.

Pending points in this contract can be considered as rewards for all users because based on these points there could be in the future airdrops, and reward distributions. Therefore, it could have an impact on all ongoing pools and not just on one of them.

POC

    function test_PendingPoints () public {

        uint256 start = block.number;

        uint256 amountToDeposit1 = 100e25;
        uint256 poolId1 = sophonFarming.typeToId(SophonFarmingState.PredefinedPool.wstETH);

        vm.startPrank(account1);
        deal(address(wstETH), account1, amountToDeposit1);

        wstETH.approve(address(sophonFarming), 50e25);
        sophonFarming.deposit(poolId1, 50e25, 0);
        vm.stopPrank();

        vm.startPrank(account2);
        deal(address(wstETH), account2, amountToDeposit1);

        wstETH.approve(address(sophonFarming), amountToDeposit1);
        sophonFarming.deposit(poolId1, amountToDeposit1, 0);
        vm.stopPrank();

        uint256 amountToDeposit2 = 10000e25;
        uint256 poolId2 = sophonFarming.typeToId(SophonFarmingState.PredefinedPool.sDAI);

        vm.startPrank(account2);
        deal(address(sDAI), account2, amountToDeposit2);

        sDAI.approve(address(sophonFarming), amountToDeposit2);
        sophonFarming.deposit(poolId2, amountToDeposit2, 0);
        vm.stopPrank();

        // some time passes 
        vm.roll(start + 30);

        vm.startPrank(account1);
        uint256 ppvalue_1 = sophonFarming.pendingPoints(poolId1, account1);
        vm.stopPrank();

        vm.prank(account2);
        uint256 ppvalue_2 = sophonFarming.pendingPoints(poolId1, account2);

        SophonFarmingState.PoolInfo[] memory PoolInfo;
        PoolInfo = sophonFarming.getPoolInfo();
        emit log(PoolInfo[1].lastRewardBlock);

        // make deposit to update lastRewardBlock.

        vm.startPrank(account1);
        wstETH.approve(address(sophonFarming), 30e25);
        sophonFarming.deposit(poolId1, 30e18, 0);
        vm.stopPrank();
        PoolInfo = sophonFarming.getPoolInfo();
        emit log(PoolInfo[1].lastRewardBlock);
        sophonFarming._getBlockMultiplier(PoolInfo[1].lastRewardBlock,block.number);

        // some time passes     
        vm.roll(start + 3500);

        uint256 PP_oldendblock = sophonFarming.pendingPoints(poolId1, account1);
        vm.prank(deployer);
        sophonFarming.setEndBlock(0,0);
        uint256 PP_newendblock = sophonFarming.pendingPoints(poolId1, account1);

        // difference in pending points calculated before and after change of endBlock

        uint256 PP_difference = PP_oldendblock-PP_newendblock;
        assertGt(PP_difference, 0, "No difference in pending points");
        emit log (PP_difference);
├─ [9982] SophonFarming::getPoolInfo() [staticcall]
    │   └─ ← [Return] [PoolInfo({ lpToken: 0xa513E6E4b8f2a923D98304ec87F64353C4D5C853, l2Farm: 0x0000000000000000000000000000000000000000, amount: 100000000000000000000000000000 [1e29], boostAmount: 0, depositAmount: 100000000000000000000000000000 [1e29], allocPoint: 20000 [2e4], lastRewardBlock: 1, accPointsPerShare: 0, description: "sDAI" }), PoolInfo({ lpToken: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0, l2Farm: 0x0000000000000000000000000000000000000000, amount: 1500000030000000000000000000 [1.5e27], boostAmount: 0, depositAmount: 1500000030000000000000000000 [1.5e27], allocPoint: 20000 [2e4], lastRewardBlock: 31, accPointsPerShare: 166666666666 [1.666e11], description: "wstETH" }), PoolInfo({ lpToken: 0x5FC8d32690cc91D4c39d9d3abcBD16989F875707, l2Farm: 0x0000000000000000000000000000000000000000, amount: 0, boostAmount: 0, depositAmount: 0, allocPoint: 20000 [2e4], lastRewardBlock: 1, accPointsPerShare: 0, description: "weETH" })]
    ├─ emit log(: 31)
    ├─ [657] SophonFarming::_getBlockMultiplier(31, 31) [staticcall]
    │   └─ ← [Return] 0
    ├─ [0] VM::roll(3501)
    │   └─ ← [Return] 
    ├─ [3423] SophonFarming::pendingPoints(1, 0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return] 9722222607777436733333 [9.722e21]
    ├─ [0] VM::prank(0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266)
    │   └─ ← [Return] 
    ├─ [41482] SophonFarming::setEndBlock(0, 0)
    │   └─ ← [Stop] 
    ├─ [2028] SophonFarming::pendingPoints(1, 0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return] 9722222607777321766660 [9.722e21]
    ├─ [0] VM::assertGt(114966673 [1.149e8], 0, "No difference in pending points") [staticcall]
    │   └─ ← [Return] 
    ├─ emit log(: 114966673 [1.149e8])
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.50ms (2.32ms CPU time)

Ran 1 test suite in 34.61ms (13.50ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Code Snippet

function setEndBlock(uint256 _endBlock, uint256 _withdrawalBlocks) public onlyOwner {
        uint256 _endBlockForWithdrawals;
@>        if (_endBlock != 0) {
            if (_endBlock <= startBlock || getBlockNumber() > _endBlock) {
                revert InvalidEndBlock();
            }
            if (isFarmingEnded()) {
                revert FarmingIsEnded();
            }
            _endBlockForWithdrawals = _endBlock + _withdrawalBlocks;

@>        } else {
        //withdrawal blocks needs an endBlock
        _endBlockForWithdrawals = 0; 
        //revert InvalidEndBlock();
        }
function massUpdatePools() public {
        uint256 length = poolInfo.length;
        for(uint256 pid = 0; pid < length;) {
            updatePool(pid);
            unchecked { ++pid; }
        }
    }
       function massUpdatePools() public {
        uint256 length = poolInfo.length;
        for(uint256 pid = 0; pid < length;) {
            updatePool(pid);
            unchecked { ++pid; }
        }
    }

    /**
     * @notice Updating accounting of a single pool
     * @param _pid pid to update
     */
    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();
    }

Tool used

Foundry

Recommendation

To change the code as below:

    function setEndBlock(uint256 _endBlock, uint256 _withdrawalBlocks) public onlyOwner {
        uint256 _endBlockForWithdrawals;
        if (_endBlock != 0) {
            if (_endBlock <= startBlock || getBlockNumber() > _endBlock) {
                revert InvalidEndBlock();
            }
            if (isFarmingEnded()) {
                revert FarmingIsEnded();
            }
            _endBlockForWithdrawals = _endBlock + _withdrawalBlocks;

        } else {
-       //withdrawal blocks needs an endBlock
-       _endBlockForWithdrawals = 0;
+      revert InvalidEndBlock();
        }
-      massUpdatePools();                
+      massUpdatePoolsduetochangeendBlock();
        endBlock = _endBlock;
        endBlockForWithdrawals = _endBlockForWithdrawals;
    } 
/**
     * @notice Update accounting of all pools
     */
    function massUpdatePools() public {
        uint256 length = poolInfo.length;
        for(uint256 pid = 0; pid < length;) {
            updatePool(pid);
            unchecked { ++pid; }
        }
    }

    /**
     * @notice Updating accounting of a single pool
     * @param _pid pid to update
     */
    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();
    }

+    function massUpdatePoolsduetochangeendBlock() public {
+        uint256 length = poolInfo.length;
+        for(uint256 pid = 0; pid < length;) {
+            updatePoolduetochangeendBlock(pid);
+            unchecked { ++pid; }
+        }
+    }

+    function updatePoolduetochangeendBlock (uint256 _pid) public {

+        PoolInfo storage pool = poolInfo[_pid];
+        if (getBlockNumber() <= pool.lastRewardBlock) {
+            return;
+        }
+        uint256 lpSupply = pool.amount;
+        uint256 _pointsPerBlock = pointsPerBlock;
+        uint256 _allocPoint = pool.allocPoint;
+        }