sherlock-admin2 commented 5 months ago

0xreadyplayer1

medium

Loss of funds to users near the end of withdrawal block due to many real life scenarios

Summary

The user will not be able to withdraw their tokens after deposit if there withdraw is delayed due to certain real life scenarios .

Vulnerability Detail

The user might have deposited 100 million tokens inside the farming contract's to accrue big points for airdrop rewards.

However , if the withdrawal block limit is very narrow like in tests , it is 1000


        sophonFarming.setEndBlock(maxUint - 1000, 1000);

given ethereum has block time of 12 seconds , if the withdrawal is only allowed for 1000 blocks

that means every user who has staked their tokens has to withdraw in the time period of 12000 seconds = 200 minutes = 3.2 hours

which is quite unrealistic for everyone to withdraw in this period because people might have other priorities

Now even if the withdrawal time range is big ,

The user's might not have initiated their withdrawal transaction due to reasons like

User might not recognize withdrawl is enabled
User might be away from their laptop due to real life's not-perfect scenarios like health problems , internet problems , business or family matters to solve
Transaction getting delayed due to network congestion or low gas fees etc when they initiate the TX just around the corner of ending withdrawal block

and other real life scenarios .

The tragedy is the contract is designed in a way that causes following issues

After farming has been ended , no one can extend the withdrawal period blocks
There is no other emergency method to allow users to pull their funds when farming has been ended and withdrawal period has passed
There is no other method where LPs are being transferred to users

Now this situation might sound un-realistic but it in fact is the most realistic scenario we can have in real world.

When users are not allowed to withdraw their funds , this will simply lead to loss of their funds and funds will get stuck forever in the contract .

because withdraw was the only method to pull out funds from.

PoC

Here is my PoC that demonstrates the issue

Insert in test/SophonFarming.t.sol

 function test_WithdrawDisabled() 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();
        uint withdrwalBlockEnd=_endBlock + 1000;
        console.log("start block is ", _startBlock);
        console.log("end block is ", _endBlock);
        console.log("withdrwalBlockEnd is ", withdrwalBlockEnd);

        // ########## 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);

        // Step 2. 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

        /*

         Step 3. User initiates withdraw TX is executed after one block

         User's TX being executed after the withdrwal period can happen due Multiple reasons

         - User might not recognize withdrawl is enabled 
         - User might be away from their laptop due to real life's not-perfect scenarios like health problems , internet problems , 
           business or family matters to solve
        - Transaction getting delayed due to network congestion or low gas fees etc when they initiate the TX just around the corner of ending
          withdrawal block

        */ 

        vm.roll(withdrwalBlockEnd+1); 
        sophonFarming.withdraw(poolId1,amountToDeposit1);  

    }

PoC Output

Impact

Users will lose millions of dollars in tokens and funds will get stuck forever.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add another method to withdraw user funds by maybe deducting a small portion of their deposit amount like 1% but not entirely.

This will allow the flexible nature of protocol.

sherlock-admin4 commented 5 months ago

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

0xmystery commented:

invalid because admin can always set endBlock to zero to temporarily allow indefinite period of withdrawal. Otherwise there should be a resolution when pool.depositAmount is bridged

0xreadyplayer1 commented 5 months ago

Hi @mystery0x , thank you for your judgement . However your assumption is not correct .

For your first argument, admin can always set endBlock to zero to temporarily allow indefinite period of withdrawal

we can not assume that owner will call setendblock to set withdrawal block to 0 to allow infinite withdrawals. This is not told in either docs or contest readme so assumption like this is incorrect. We are talking about standard behaviour of the contract here and not something like owner moving params back and forth . Even though contract allowas this , As this behaviour was not communicated , we can not assume it

Secondly , Your second argument of bridging is not related to this audit as protocol said in the contest readme Any bridging related code is considered out of scope so bridging logic is not even the concern for this report.

I hope i've added enough information for you to review my report again . And you shall see its a valid report.

Thanks

mystery0x commented 5 months ago

This is the current design of the contract. Users will have to play by the rules. Users' negligence for not withdrawing user.depositAmount in time is not the protocol's responsibility. pool.depositAmount will eventually be bridged regardless. I'm sure realistic endBlockForWithdrawals will be given when the contract goes live. Let's see what the sponsor has to say when they review this report.

sherlock-audit / 2024-05-sophon-judging

0xreadyplayer1 - Loss of funds to users near the end of withdrawal block due to many real life scenarios #51