First Deposit Bug

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L340-L370

Vulnerability details

Impact

The affected implementation contains a critical bug which can be exploited to steal funds of initial depositors of a freshly deployed MToken market.

What happens if exchange rate can be increased to a value greater than the user's deposit? (https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L363)

A sophisticated attack can impact all initial user deposits until the lending protocols owners and users are notified and contracts are paused. Since this attack is a replicable attack it can be performed continuously to steal the deposits of all depositors that try to deposit into the new MToken contract.

The loss amount will be the sum of all deposits done by users into the MToken multiplied by the underlying token's price.

Suppose there are 10 users and each of them tries to deposit 1,000,000 underlying tokens into the MToken contract. Price of underlying token is $1.

Total loss (in $) = $10,000,000

Proof of Concept

Detailed Steps

As the exchange rate is dependent upon the ratio of MToken's total supply and underlying token balance of MToken contract, the attacker can craft transactions to manipulate the exchange rate.

Steps to attack:

1- Once the MToken has been deployed and added to the lending protocol, the attacker mints the smallest possible amount of MTokens.

2- Then the attacker does a plain underlying token transfer to the MToken contract, artificially inflating the underlying.balanceOf(MToken) value. Due to the above steps, during the next legitimate user deposit, the mintTokens value for the user will become less than 1 and essentially be rounded down to 0 by Solidity. Hence the user gets 0 MTokens against his deposit and the MToken's entire supply is held by the Attacker.

3- The Attacker can then simply reedem his MToken balance for the entire underlying token balance of the MToken contract.

The same steps can be performed again to steal the next user's deposit.

It should be noted that the attack can happen in two ways:

• The attacker can simply execute the Step 1 and 2 as soon as the MToken gets added to the lending protocol.

• The attacker watches the pending transactions of the network and frontruns the user's deposit transaction by executing Step 1 and 2 and then backruns it with Step 3.

Working POC

A working POC is available at https://github.com/akshaysrivastav/first-deposit-bug-compv2
You can also read the attack details here -> https://akshaysrivastav.hashnode.dev/first-deposit-bug-in-compoundv2-and-its-forks

Tools Used

Manual review + in-house tool

Recommended Mitigation Steps

The fix to prevent this issue would be to enforce a minimum deposit that cannot be withdrawn. This can be done by minting small amount of MToken units to 0x00 address on the first deposit.

function mintFresh(address minter, uint mintAmount) internal returns (uint, uint) {
    ......

    (vars.mathErr, vars.mintTokens) = divScalarByExpTruncate(vars.actualMintAmount, Exp({mantissa: vars.exchangeRateMantissa}));
    require(vars.mathErr == MathError.NO_ERROR, "MINT_EXCHANGE_CALCULATION_FAILED");

    /// THE FIX
    if (totalSupply == 0) {
        totalSupply = 1000;
        accountTokens[address(0)] = 1000;
        vars.mintTokens -= 1000;
    }

    /*
     * We calculate the new total supply of mTokens and minter token balance, checking for overflow:
     *  totalSupplyNew = totalSupply + mintTokens
     *  accountTokensNew = accountTokens[minter] + mintTokens
     */
    (vars.mathErr, vars.totalSupplyNew) = addUInt(totalSupply, vars.mintTokens);
    require(vars.mathErr == MathError.NO_ERROR, "MINT_NEW_TOTAL_SUPPLY_CALCULATION_FAILED");

    ......

    return (uint(Error.NO_ERROR), vars.actualMintAmount);
}

Instead of a fixed 1000 value an admin controlled parameterized value can also be used to control the burn amount on a per MToken basis.

Alternatively, a quick fix would be that the protocol owners perform the initial deposit themselves with a small amount of underlying tokens and then burn the received MTokens permanently by sending them to a dead address.

Credits

https://akshaysrivastav.hashnode.dev/first-deposit-bug-in-compoundv2-and-its-forks

Assessed type

Decimal

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #92

[c4-judge]

alcueca marked the issue as unsatisfactory:
Out of scope