0xreadyplayer1 - Delayed withdraw transaction will lead to accruing less rewards/points to users when the points per block has been changed in the meanwhile

[sherlock-admin3]

0xreadyplayer1

medium

Delayed withdraw transaction will lead to accruing less rewards/points to users when the points per block has been changed in the meanwhile

Summary

If a user initiates their withdraw transaction , the transaction gets delayed to next block but the transactions like decreasing the

points per block by admin succeeds , the user will incur a huge loss of rewards/points that effectively will decrease amount of

airdrop tokens amount the user is entitled to get.

Vulnerability Detail

The Withdraw and deposit transactions rely on the block multiplier and points per block to calculate the amount of rewards/points

settled for user . These rewards will be later used for deciding the exact amount of tokens the owner of the airdrop tokens

will send to the user and regarded as activity.

However , if a user initiates their withdraw transaction at Block X , and their transaction is delayed upto X+Y blocks where Y >=1

But in the same block X , there are set of owner only transactions that alter the state of farming contracts especially points per block

Then the user , when withdraw transaction is executed will incur a loss of funds compared to their anticipated rewards calculation.

The transaction delays are well known on ETH Mainnet & can occur due to following reasons :

        - Network congestion scenarios 
        - Low gas fee for tx 
        - Random choice of block proposers

and some other miscellaneous factors.

Please read the well-detailed-documented PoC

PoC

Exploit Scenario

1. User Deposits amount
2. After some blocks , User initiates the withdraw tx , it goes into mempool , Tx is delayed due to one of the following reasons
   • network congestion scenarios
   • low gas fee for tx
   • random choice of block proposers
   • some other miscelenious factors
3. Points per block and some other parameters related to rewards accumulation are modified by owner
4. User's withdraw is executed one block after the intended current block
5. User's rewardsSettled are manipulated to have less rewards than anticipated.

Code

Please insert this test inside test/SophonFarming.t.sol and run test using

forge test --mt test_lessRewardsDueToTxDelay -vvvv --via-ir

    function test_lessRewardsDueToTxDelay() public {

        // We are changing critical params so they don't overflow and we can focus on main things

        vm.startPrank(deployer);
        sophonFarming.setEndBlock(1000000, 1000);
        harnessImplementation.setEndBlock(1000000, 1000);
        sophonFarming.setPointsPerBlock(pointsPerBlock);

        // A helping role to formalize update pool calls
        address keeper = vm.addr(76);

        // get config

        uint _startBlock = sophonFarming.startBlock();
        uint _endBlock = sophonFarming.endBlock();
        console.log("start block is ", _startBlock);
        console.log("end block is ", _endBlock);

       /*
        * ############# Exploit Scenario
        *
        - User Deposits amount

        - After some blocks , User initiates the withdraw tx , it goes into mempool , 
        - Tx is delayed due to one of the following reasons 
            - network congestion scenarios 
            - low gas fee for tx 
            - random choice of block proposers
            - some other miscelenious factors
        - Points per block and some other parameters related to rewards accumulation are modified by owner 
        - User's withdraw is executed one block after the intended current block
        - User's rewardsSettled are manipulated to have less rewards than anticipated.

        */

        // ########## Step1. User deposit
        // #######################
        vm.roll(_startBlock + 10);
        // we are targetting wstETH pool for Account 1
        uint256 amountToDeposit1 = 100e18;
        uint256 poolId1 = sophonFarming.typeToId(
            SophonFarmingState.PredefinedPool.wstETH
        );

        vm.startPrank(account1);
        deal(address(wstETH), account1, amountToDeposit1);
        wstETH.approve(address(sophonFarming), amountToDeposit1);
        sophonFarming.deposit(poolId1, amountToDeposit1, 0);

        // User has waited for some time to accrque rewards 
        // roll block to 10 more blocks to get some points unlocked
        vm.roll(block.number + 10);
        // fetch pool details to calculate how many rewards user will get when they do a withdraw in the next block
        // for simplicity , say the current block.number is 10 to ease calculations

        (
            uint user_amount,,
            uint user_depositAmount,
            uint rewardSettled,
            uint rewardDebt
        ) = sophonFarming.userInfo(poolId1, account1);

        (  IERC20 lpToken, ,uint pool_amount , , , uint256 allocPoint, uint lastRewardBlock, uint256 accPointsPerShare,) = sophonFarming.poolInfo(poolId1);

        // ##########################
        // Book Keeping for old values of variables to calculate expected rewards of user on time of withdraw
        // ##########################

       // User's accounting
       uint    oldUserRewardSettled=rewardSettled;
       uint    oldUserRewardDebt=rewardDebt;
       uint    old_userAmount=user_amount;
       // Pool's Accounting
       uint    old_accPointsPerShare = accPointsPerShare * 1e18;
       uint    old_lpSupply = pool_amount;
       uint    old_pointsPerBlock=pointsPerBlock;
       uint    old_allocPoint=allocPoint;
       uint    old_totalAllocPoint=sophonFarming.totalAllocPoint();
       uint    old_lastRewardBlock=lastRewardBlock;

        // Step 3. User initiates withdraw TX but it is delayed for some reason stated in exploit scenario section

        // sophonFarming.withdraw(userInfo.depositAmount);

        // Admin's function calls to change parameters are executed
        vm.startPrank(deployer);
        uint _pointsPerBlock = 1e10;
        /// set paramas to decrease user's rewardsSettled

        sophonFarming.setPointsPerBlock(_pointsPerBlock);
        sophonFarming.setEndBlock(1000000+1, 1000);

        vm.roll(block.number+1);

        vm.startPrank(account1);
        {
        sophonFarming.withdraw(1,user_depositAmount); //wstETH withdraw
        }

        ( lpToken,,,  , , allocPoint,, accPointsPerShare, ) = sophonFarming.poolInfo(poolId1);
        (user_amount,,,rewardSettled,rewardDebt) = sophonFarming.userInfo(poolId1, account1);

        uint actualRewardsSettled =sophonFarming.pendingPoints(poolId1,account1);

        // since the transaction has been executed after one block from it's origination ,
        // additional blockMultiplier has to be added which is 1e18 
        /**
         * 
         *  function _getBlockMultiplier(uint256 _from, uint256 _to) internal view returns (uint256) {
        uint256 _endBlock = endBlock;
        //snip
        if (_to > _from) {
            return (_to - _from) * 1e18;
        } 

        //snip
    }
         */

        /**
         * User's calculation for rewards settled is based on let's say block 10
         * Block number 11 , when user had to withdraw , block 11 has been conested , user's TX has been delayed
         * If user's TX was executed at that time , the multiplier would have been 1e18 because 
         * (to-from)=1 where from = lastRewardsBlock number and to is current block number 
         * 
         * uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());

         * SO when the TX is executed at block 12 , the difference (to-from)=2 and them 2+1e18 is 2e18
         */

        uint blockMultiplier = (block.number-old_lastRewardBlock)*1e18;
        uint expected_PointReward =blockMultiplier *
                old_pointsPerBlock *
                old_allocPoint /
                old_totalAllocPoint; 

        uint expected_accPointsPerShare= expected_PointReward *
                1e18 /
                old_lpSupply +
                old_accPointsPerShare;

        uint expectedRewardsSettled=old_userAmount *
            expected_accPointsPerShare /
            1e36 +
            oldUserRewardSettled -
            oldUserRewardDebt;

        console.log("expectedRewardsSettled",expectedRewardsSettled);
        console.log("actualRewardsSettled",actualRewardsSettled);

        console.log("User's loss := ",expectedRewardsSettled-actualRewardsSettled);

    }

PoC output

This test demonstrates how this small delay in witdraw tx can lead to user losing their 8.33e18 Tokens which is huge amount of rewards.

The issue might escalate when their is a big price change in pointsPerBlock and transaction delays for more blocks like 5 or 10

in the time of high network demand.

Impact

Loss of user's rewards/points which effectively means loss of token funds as they will determine the amount of airdrop tokens users will receive.

Code Snippet

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

Tool used

Manual Review

Recommendation

There can be multiple steps to minimize ( as fixing is very hard - would require revamping entire implementation )

• Allow time-boxed change of critical params like points per block and other params that affect's the accounting price per share .

[sherlock-admin3]

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

0xmystery commented:

invalid because an admin action can break certain assumptions about the functioning of the code

[0xreadyplayer1]

@mystery0x ,Thank you for your judgement.

However , i disagree with the decision .

The point is the user who owed more tokens before the change MUST receive same tokens after the change and this new change should be applied to new users. That's how a lot of standard systems work.

However if a Person has deposited for a long time and they have EARNED 1 Million rewards , the admin has no rights to decrease his rewards by a major amount .

The point here is that this is not malicious behaviour by admin , but it is about the insufficiency of the code to handle the rewards accrual and retaining those rewards for existing users in time of block points per change because existing users who has staked for so long bearing in mind the points they will get based on the config of the protocol at their time of deposit , and withdraw , they MUST receive same amount of tokens otherwise the system is just bricked for users and their trust and will cause huge losses to users.

This is clearly a valid medium according to sherlock rules because their are Definite loss of funds involved but due to some external conditions.

[mystery0x]

Actually, there's no issue here. massUpdatePools() is triggered before having pointsPerBlock assigned a new value:

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

    function setPointsPerBlock(uint256 _pointsPerBlock) public onlyOwner {
        if (isFarmingEnded()) {
            revert FarmingIsEnded();
        }
        massUpdatePools();
        pointsPerBlock = _pointsPerBlock;
    }

pool.accPointsPerShare for each _pid is updated before the change, that could also possibly increase instead of just decrease pointsPerBlock, ensuring fair reward accumulation for all existing users.