Read-only reentrancy in MasterChefV2::deposit allows an address to take control of all lp tokens and inflate total supply of a pid

Summary

MasterChefV2::deposit(pid, arbitrary_amount) ==> MasterChefRewarder.onModify ==> MasterChefRewarder._claim ==> call account (depositor) with reward amount of ETH ==> attacker contract ==> MasterChefV2::withdraw(pid, arbitrary_amount) ==> send lp tokens of pid to attacker contract

Vulnerability Detail

MasterChefV2::deposit(pid,amount) modifies user state, before transferring LP tokens from user to the pool:

    function deposit(uint256 pid, uint256 amount) external override {
        //@audit-info increase msg.sender balance by amount
        _modify(pid, msg.sender, amount.toInt256(), false);
        //@audit-info transfer tokens from msg.sender to the contract after all state changes
        if (amount > 0)
            _farms[pid].token.safeTransferFrom(
                msg.sender,
                address(this),
                amount
            );
    }

the balance of msg.sender is increased by amount: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L544

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

then if pid has an extraRewarder, MasterChefRewarder::onModify hook is called: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L560

    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;
        //...
        if (address(extraRewarder) != address(0)) {
            //@audit here, onModify is called
            extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
        }
        //...
    }

looking at implementation of MasterChefRewarder::onModify, we can see that at the end of the call, _claim function sends reward to account using _safeTransferTo function: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/MasterChefRewarder.sol#L77

    function onModify(address account, uint256 pid, uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply)
        public
        override(BaseRewarder, IBaseRewarder)
        returns (uint256 reward)
    {
        /...
        //@audit claim rewards for account if any
        _claim(account, reward);
    }

and if reward token is address(0), it calls account with reward amount of ETH: https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BaseRewarder.sol#L324

    function _safeTransferTo(
        IERC20 token,
        address account,
        uint256 amount
    ) internal virtual {
        if (amount == 0) return;
        if (address(token) == address(0)) {
            //@audit call account here
            (bool s, ) = account.call{value: amount}("");
            if (!s) revert BaseRewarder__NativeTransferFailed();
        } else {
            token.safeTransfer(account, amount);
        }
    }

at this point, account which is a malicious contract takes control of the call and withdraws all the lp tokens from pid:

contract MaliciousContract {
      receive() external payable {
           //1- received ETH
           //2- withdraw assets from MasterChefV2
           masterChef.emergencyWithdraw(target_pid);
           //3- do whatever with this tokens
      }
}

However at the end of this call, all LP tokens must be paid back:

    function deposit(uint256 pid, uint256 amount) external override {
        _modify(pid, msg.sender, amount.toInt256(), false);
        //@audit-info enough hacking, pay the tokens back!
        if (amount > 0)
            _farms[pid].token.safeTransferFrom(
                msg.sender,
                address(this),
                amount
            );
    }

this issue provides a flash-loan functionality to take flash loan of LP tokens but with zero fees!

Impact

currently, i could not find any impacts for this issue, as user has to pay all the LP tokens back to the MasterChefV2 and all state changes will be reset to what was before. there might be some parts of the system which are OOS for this audit, but could be affected by this read-only reentrancy since total supply of the target pid is inflated by this attack.

    function deposit(uint256 pid, uint256 amount) external override {
        //@audit-info take control of the assets
        _modify(pid, msg.sender, amount.toInt256(), false);
        //@audit-info finally pay back all the tokens here
        if (amount > 0)
            _farms[pid].token.safeTransferFrom(
                msg.sender,
                address(this),
                amount
            );
    }

Code Snippet

Tool used

Manual Review

Recommendation

use a nonReentrant modifier on MasterChefV2 functions.

sherlock-audit / 2024-06-magicsea-judging

Sticky Hickory Hare - Read-only reentrancy in MasterChefV2::deposit allows an address to take control of all lp tokens and inflate total supply of a pid #724