MToken May be Inflation Attack

[code423n4]

Lines of code

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

Vulnerability details

Impact

MToken May be Inflation Attack like ERC4626 Inflation Attack

Proof of Concept

Currently mToken still has the common ERC4626 Inflation Attack
The number of assets can be increased by donating
MToken.sol#exchangeRateStoredInternal()

    function exchangeRateStoredInternal() virtual internal view returns (MathError, uint) {
        uint _totalSupply = totalSupply;
        if (_totalSupply == 0) {
---SKIP---
            uint totalCash = getCashPrior(); // @audit <---------------asset from getCashPrior()
            uint cashPlusBorrowsMinusReserves;
            Exp memory exchangeRate;
            MathError mathErr;
---SKIP---
            return (MathError.NO_ERROR, exchangeRate.mantissa);
        }
    }

MErc20.sol#getCashPrior()

    function getCashPrior() virtual override internal view returns (uint) {
        EIP20Interface token = EIP20Interface(underlying);
        return token.balanceOf(address(this)); // @audit //<---------------from balaceOf()
    }

So it is still possible to mint first and then redeem, and finally 1 shares left, and then donate by direct transfer assert,Amplify exchangeRate, thus carrying out Inflation Attack
Note: Although the initial mint initialSupply can be executed when creating a MToken, there is no restriction on initialSupply==0
It is recommended to refer to the new 4.9.0 release of OpenZeppelin, which has a special version of ERC4626 Inflation Attack for this
https://twitter.com/OpenZeppelin/status/1656066698410328064?s=20

Tools Used

Manual Review

Recommended Mitigation Steps

1. Keeping track of the total assets internally
2. Creating "dead shares"
   Reference: Implement or recommend mitigations for ERC4626 inflation attacks OpenZeppelin/openzeppelin-contracts#3706 (comment)

Assessed type

ERC4626

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

without a PoC, this is an invalid finding because yes, you could donate to the protocol and increase the share price, but you have to show a way to do this in a way that's profitable to an attacker.

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[alcueca]

@ElliotFriedman, I think that #264 does a good job in explaining how this would be profitable for an attacker:

The attacker can then redeem their 1 wei cToken for all the underlying assets in that cToken contract.

I also like that #126 links to existing literature on this specific bug.

#264 also goes to explain how you are protecting yourself against this vulnerability in mip00, but how the protection seems to be not enough given the cross-chain context.

To mitigate the potential for front-running attacks, it is advised that the initial mToken minting process is finalized prior to transferring administrative rights to the governance or bundle unpausing and mToken minting into the same VAA to be executed atomically.

[c4-judge]

alcueca marked the issue as selected for report

[ElliotFriedman]

This isn't a valid finding because known issues here explained in the readme: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/README.md#known-compound-v2-issues

"New markets must be added with no collateral factor, and some small amount of the collateral token supply must be burned in order to avoid market manipulation. This is a known issue."

[ElliotFriedman]

So yes, it's a valid issue, but it is out of scope as we recognize this is an issue and don't want to award a payout for something that is clearly stated to be out of scope.

[alcueca]

Agree with the sponsor, except for #264. I'll continue the conversation there.

[c4-judge]

alcueca marked the issue as unsatisfactory:
Out of scope