0xboriskataa - Attacker can withdraw all tokens deposited in a farm

[sherlock-admin4]

0xboriskataa

High

Attacker can withdraw all tokens deposited in a farm

Summary

Attacker can withdraw all tokens deposited in a farm

Vulnerability Detail

There is a bug in the Math.sol library in the addDelta() function:

    function addDelta(uint256 x, int256 delta) internal pure returns (uint256 y) {
        uint256 success;

        assembly {
            y := add(x, delta)

            success := iszero(or(gt(x, MAX_INT256), gt(y, MAX_INT256)))
        }

        if (success == 0) revert Math__UnderOverflow();
    }

The issue here is that the line y := add(x, delta) will not revert on underflow. If for example x = 10 and delta = -20 the result will be UINT_MAX - 10. You might think this won't happen because we are using solidity version 0.8 which has overflow and underflow protection but that is not the case for assembly operations. More can be read here

An attacker can use this bug to drain all of the deposited tokens in MasterchefV2.sol using the withdraw() function:

    function withdraw(uint256 pid, uint256 amount) external override {
        _modify(pid, msg.sender, -amount.toInt256(), true);

        if (amount > 0) _farms[pid].token.safeTransfer(msg.sender, amount);
    }

It calls _modify() so let's take a look at it:

    function _modify(uint256 pid, address account, int256 deltaAmount, bool isPayOutReward) private {
        Farm storage farm = _farms[pid];
        Rewarder.Parameter storage rewarder = farm.rewarder;
        IMasterChefRewarder extraRewarder = farm.extraRewarder;

        (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = farm.amounts.update(account, deltaAmount);

        uint256 totalLumRewardForPid = _getRewardForPid(rewarder, pid, oldTotalSupply);
        uint256 lumRewardForPid = _mintLum(totalLumRewardForPid);

        uint256 lumReward = rewarder.update(account, oldBalance, newBalance, oldTotalSupply, lumRewardForPid);

        if (isPayOutReward) {
            lumReward = lumReward + unclaimedRewards[pid][account];
            unclaimedRewards[pid][account] = 0;
            if (lumReward > 0) _lum.safeTransfer(account, lumReward);
        } else {
            unclaimedRewards[pid][account] += lumReward;
        }

        if (address(extraRewarder) != address(0)) {
            extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
        }

        emit PositionModified(pid, account, deltaAmount, lumReward);
    }

As you can see it uses the amounts library to update the balance of the caller:

    function update(Parameter storage amounts, bytes32 key, int256 deltaAmount)
        internal
        returns (uint256 oldAmount, uint256 newAmount, uint256 oldTotalAmount, uint256 newTotalAmount)
    {
        oldAmount = amounts.amounts[key];
        oldTotalAmount = amounts.totalAmount;

        if (deltaAmount == 0) {
            newAmount = oldAmount;
            newTotalAmount = oldTotalAmount;
        } else {
            newAmount = oldAmount.addDelta(deltaAmount);
            newTotalAmount = oldTotalAmount.addDelta(deltaAmount);

            amounts.amounts[key] = newAmount;
            amounts.totalAmount = newTotalAmount;
        }
    }

Here is where the problematic addDelta() function from the Math.sol library is used.

What an attacker can do is to enter an amount of tokens to withdraw in withdraw() that is bigger than the balance he has. There is no check to validate if he is withdrawing more than he has in his balance. Perhaps the intention was that if he tries to do that then this line will simply revert because of an underflow:

     newAmount = oldAmount.addDelta(deltaAmount);

However as I explained above a revert in addDelta() will not happen. The Amounts.sol library will simply use this underflowed new balance of the user to calculate how much rewards he should get:

     uint256 lumReward = rewarder.update(account, oldBalance, newBalance, oldTotalSupply, lumRewardForPid);

After that the function will simply transfer both the calculated reward and the specified tokens to withdraw:

        if (isPayOutReward) {
            lumReward = lumReward + unclaimedRewards[pid][account];
            unclaimedRewards[pid][account] = 0;
            if (lumReward > 0) _lum.safeTransfer(account, lumReward);
        } else {
            unclaimedRewards[pid][account] += lumReward;
        }

if (amount > 0) _farms[pid].token.safeTransfer(msg.sender, amount);

Impact

Attacker can drain every deposited token from a farm. He will also get additional rewards that will be calculated incorrectly.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Math.sol#L20-L30

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L306-L310

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L539-L564

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Amounts.sol#L68-L85

Tool used

Manual Review

Recommendation

You can use this implementation for addDelta() that is from uniswap:

    function addDelta(uint128 x, int128 y) internal pure returns (uint128 z) {
        if (y < 0) {
            require((z = x - uint128(-y)) < x, 'LS');
        } else {
            require((z = x + uint128(y)) >= x, 'LA');
        }
    }

Also do a check in the withdraw function that the user is not withdrawing more than he has in his balance:

    function withdraw(uint256 pid, uint256 amount) external override {
        _modify(pid, msg.sender, -amount.toInt256(), true);

+       if (amount > _farms[pid].amounts.getAmountOf(msg.sender)) revert MasterChef__NotEnoughBalance();
        if (amount > 0) _farms[pid].token.safeTransfer(msg.sender, amount);
    }

[0xSmartContract]

This error occurs particularly when x is greater than MAX_INT256. However, such large values ​​are rarely used. For most users and processes, such large values ​​will not be valid.

That is, the probability of encountering the error in real-world use is low.

The error may affect the correct operation of the function, but it does not directly harm user funds or create a security vulnerability.