code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MErc20.sol#L148-L153

Vulnerability details

Impact

sweepToken() is designed to allow the market owner to withdraw any ERC20 token which might have ended up at MToken address. Underlying token must not be swept, therefore sweepToken() ensures token is not underlying. However, this can be bypassed if the underlying token is a double-entrypoint token.

Proof of Concept

Here it ensures that token address is different.

    function sweepToken(EIP20NonStandardInterface token) override external {
        require(msg.sender == admin, "MErc20::sweepToken: only admin can sweep tokens");
        require(address(token) != underlying, "MErc20::sweepToken: can not sweep underlying token");
        uint256 balance = token.balanceOf(address(this));
        token.transfer(admin, balance);
    }

Double-entrypoint token has multiple addresses, but all the contracts operate on single storage. Examples of such tokens: TUSD (2.8B USD market cap), SNX (740M USD market cap), and other Synthetix tokens

Tools Used

Manual Review

Recommended Mitigation Steps

Check that underlying balance didn't change after transfer

    function sweepToken(EIP20NonStandardInterface token) override external {
        require(msg.sender == admin, "MErc20::sweepToken: only admin can sweep tokens");
+       uint256 balanceBefore = IERC20(underlying).balanceOf(this);
-       require(address(token) != underlying, "MErc20::sweepToken: can not sweep underlying token");
        uint256 balance = token.balanceOf(address(this));
        token.transfer(admin, balance);
+       require(IERC20(underlying).balanceOf(this) == balanceBefore, "MErc20::sweepToken: can not sweep underlying token");
    }