typicalHuman - Fee on transfer tokens are not supported

[sherlock-admin2]

typicalHuman

Medium

Fee on transfer tokens are not supported

Summary

MasterChef doesn't fully support fee on transfer tokens which could lead to potential loss of funds.

Vulnerability Detail

MasterChef supports all kinds of ERC20 tokens - including Fee On Transfer tokens, but in code it's not fully supported. Because provided amounts are written inside farm.amounts field based on amount parameter provided by user and not by real amount received by the contract - it would create difference between token amounts accounted by contract and real tokens amount inside the contract. More context here - https://github.com/d-xo/weird-erc20#fee-on-transfer.

Impact

User assets can stuck inside a protocol because of the difference between real amount of tokens inside contract and amount of tokens accounted by protocol.

Code Snippet

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

PoC

Add mock fee on transfer token to test/mocks:

// Copyright (C) 2017, 2018, 2019, 2020 dbrock, rain, mrchico, d-xo
// SPDX-License-Identifier: AGPL-3.0-only

pragma solidity >=0.6.12;

contract MinMath {
    // --- Math ---
    function add(uint x, uint y) internal pure returns (uint z) {
        require((z = x + y) >= x);
    }
    function sub(uint x, uint y) internal pure returns (uint z) {
        require((z = x - y) <= x);
    }
}

contract ERC20Custom is MinMath {
    // --- ERC20 Data ---
    string public constant name = "Token";
    string public constant symbol = "TKN";
    uint8 public decimals = 18;
    uint256 public totalSupply;

    mapping(address => uint) public balanceOf;
    mapping(address => mapping(address => uint)) public allowance;

    event Approval(address indexed src, address indexed guy, uint wad);
    event Transfer(address indexed src, address indexed dst, uint wad);

    // --- Init ---
    constructor(uint _totalSupply) public {
        totalSupply = _totalSupply;
        balanceOf[msg.sender] = _totalSupply;
        emit Transfer(address(0), msg.sender, _totalSupply);
    }

    // --- Token ---
    function transfer(address dst, uint wad) public virtual returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }
    function transferFrom(
        address src,
        address dst,
        uint wad
    ) public virtual returns (bool) {
        require(balanceOf[src] >= wad, "insufficient-balance");
        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            require(
                allowance[src][msg.sender] >= wad,
                "insufficient-allowance"
            );
            allowance[src][msg.sender] = sub(allowance[src][msg.sender], wad);
        }
        balanceOf[src] = sub(balanceOf[src], wad);
        balanceOf[dst] = add(balanceOf[dst], wad);
        emit Transfer(src, dst, wad);
        return true;
    }
    function approve(address usr, uint wad) public virtual returns (bool) {
        allowance[msg.sender][usr] = wad;
        emit Approval(msg.sender, usr, wad);
        return true;
    }
}

contract TransferFeeToken is ERC20Custom {
    uint immutable fee;

    // --- Init ---
    constructor(uint _totalSupply, uint _fee) public ERC20Custom(_totalSupply) {
        fee = _fee;
    }

    // --- Token ---
    function transferFrom(
        address src,
        address dst,
        uint wad
    ) public override returns (bool) {
        require(balanceOf[src] >= wad, "insufficient-balance");
        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            require(
                allowance[src][msg.sender] >= wad,
                "insufficient-allowance"
            );
            allowance[src][msg.sender] = sub(allowance[src][msg.sender], wad);
        }

        balanceOf[src] = sub(balanceOf[src], wad);
        balanceOf[dst] = add(balanceOf[dst], sub(wad, fee));
        balanceOf[address(0)] = add(balanceOf[address(0)], fee);

        emit Transfer(src, dst, sub(wad, fee));
        emit Transfer(src, address(0), fee);

        return true;
    }
}

Add new file to test folder:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "forge-std/Test.sol";

import "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol";

import "../src/interfaces/IMasterChefRewarder.sol";
import "../src/interfaces/IVoter.sol";
import "../src/interfaces/IMasterChef.sol";
import "../src/interfaces/ILum.sol";
import "../src/rewarders/MasterChefRewarder.sol";
import "../src/MasterChefV2.sol";
import "../src/rewarders/BaseRewarder.sol";
import "../src/rewarders/RewarderFactory.sol";
import "../src/transparent/TransparentUpgradeableProxy2Step.sol";
import "./mocks/ERC20.sol";
import "./mocks/TransferFeeToken.sol";
import "./mocks/VoterMock.sol";
import {MlumStaking} from "../src/MlumStaking.sol";

contract MasterChefTest is Test {
    address payable immutable ADMIN = payable(makeAddr("admin"));
    address payable immutable DEPOSITOR = payable(makeAddr("depositor"));
    address payable immutable DEPOSITOR2 = payable(makeAddr("depositor2"));
    address payable immutable ALICE = payable(makeAddr("alice"));
    address payable immutable BOB = payable(makeAddr("bob"));
    address extraRewarder = makeAddr("extraRewarder");
    MlumStaking private _pool;
    ERC20Mock lum;
    TransferFeeToken transferFeeToken;
    IERC20 rewardToken;
    IERC20 stakingToken;
    MasterChef masterChef;
    IVoter voter;
    MasterChefRewarder rewarder;
    RewarderFactory factory;
    function setUp() public {
        vm.startPrank(ADMIN);
        lum = new ERC20Mock("LUM", "LUM", 18);
        transferFeeToken = new TransferFeeToken(10e18, 1e6);
        stakingToken = new ERC20Mock("Staking Token", "ST", 18);
        rewardToken = new ERC20Mock("Reward Token", "RT", 6);
        voter = new VoterMock();
        address poolImpl = address(new MlumStaking(stakingToken, rewardToken));

        address factoryImpl = address(new RewarderFactory());

        factory = RewarderFactory(
            address(
                new TransparentUpgradeableProxy2Step(
                    factoryImpl,
                    ProxyAdmin2Step(address(1)),
                    abi.encodeWithSelector(
                        RewarderFactory.initialize.selector,
                        ADMIN,
                        new uint8[](0),
                        new address[](0)
                    )
                )
            )
        );

        rewarder = MasterChefRewarder(payable(ADMIN));
        factory.setRewarderImplementation(
            IRewarderFactory.RewarderType.MasterChefRewarder,
            IRewarder(address(rewarder))
        );

        address masterChefImpl = address(
            new MasterChef(
                ILum(address(lum)),
                voter,
                factory,
                address(0),
                0.02e18
            )
        );
        masterChef = MasterChef(
            address(
                new TransparentUpgradeableProxy2Step(
                    masterChefImpl,
                    ProxyAdmin2Step(address(1)),
                    abi.encodeWithSelector(
                        MasterChef.initialize.selector,
                        ADMIN,
                        address(1)
                    )
                )
            )
        );
        masterChef.setVoter(voter);
        masterChef.setLumPerSecond(10);
        masterChef.updateOperator(ADMIN);
        masterChef.add(rewardToken, IMasterChefRewarder(address(0)));
        masterChef.add(
            ERC20(address(transferFeeToken)),
            IMasterChefRewarder(address(0))
        );
        vm.stopPrank();
    }

    function test_transferFeeToken() public {
        deal(address(transferFeeToken), address(DEPOSITOR), 1e17);
        deal(address(transferFeeToken), address(DEPOSITOR2), 1e17);
        vm.startPrank(DEPOSITOR2);
        transferFeeToken.approve(address(masterChef), 1 ether);
        masterChef.deposit(1, 1e17);
        vm.stopPrank();
        console.log(transferFeeToken.balanceOf(address(masterChef)));
        vm.startPrank(DEPOSITOR);
        transferFeeToken.approve(address(masterChef), 1 ether);
        masterChef.deposit(1, 1e17);
        masterChef.emergencyWithdraw(1);
        vm.stopPrank();
        console.log(transferFeeToken.balanceOf(address(masterChef)));
        vm.startPrank(DEPOSITOR2);
        vm.expectRevert();
        masterChef.emergencyWithdraw(1); // BUG IS HERE, DEPOSITOR 2 NOW CAN"T WITHDRAW
        vm.stopPrank();
    }
}

Tool used

Foundry.

Recommendation

Calculate difference between balance of the contract before safeTransfer and after it - the difference would be actual amount of tokens that MasterChef received.

Duplicate of #545