code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L341-L367 https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L285-L294

Vulnerability details

Overview

The First Depositor Attack

Within the context of Compound v2, a First Depositor Attack occurs when an attacker becomes the inaugural minter of a cToken. This enables them to establish the first exchange rate between the underlying asset and the cToken. By minting a small amount (1 wei) and subsequently transferring a substantial amount of the underlying asset directly to the cToken contract, the attacker can significantly inflate the exchange rate. As a result, subsequent mints yield a disproportionately small return of cTokens or none at all. The attacker can then redeem their 1 wei cToken for all the underlying assets in that cToken contract.

The Transaction Execution Process of the Forge Script

The Forge script executes its transactions by simulating the script against the latest state of the blockchain, generating calldatas and nonces according to the calls within the broadcast scope of the script. These prepared transactions are then individually submitted to the blockchain. Since the atomicity of execution can never be assured, the script is exposed to potential front-running and alterations of the on-chain state.

Cross-chain Governance Through TemporalGovernor

Admin-level functions of contracts on the Base chain are executed via the TemporalGovernor contract. This uses the Wormhole protocol to receive messages from the main governor contract situated on the Moonbeam chain. When a proposal is executed on the Moonbeam governor, a Wormhole message is created and a Validated Action Attestation (VAA) can be obtained. Anyone in possession of the VAA is granted the ability to queue the proposal on the Base chain governor and, once the proposal delay has lapsed, can execute it.

Impact

Recognizing the threat of the first depositor attack, the mip00.sol script has already implemented a countermeasure to mitigate this risk:

Creation of mToken
Addition to Comptroller via _supportMarket
Pausing of minting to prevent anyone from minting
Execution of other setups and transfer of ownership to governance
Resumption of minting
Admin-initiated minting of the initial mToken to prevent a first depositor issue

Assuming atomic execution, this sequence is capable of thwarting the attack. However, in the context of real-world deployment, this is not the case. Two instances of vulnerability occur due to different causes.

Vulnerability Instance 1

During the time gap between unpausing and minting initial mTokens. As afterDeploy concludes execution, the mToken is created, and minting is paused. The process of unpausing and minting the initial mTokens is carried out in the build function. Assuming _pushCrossChainAction publishes a cross-chain message, the proposal to unpause and mint initial mTokens must be queued and executed via the TemporalGovernor. As any party holding a valid VAA can execute the proposal after the delay has elapsed, an attacker can front-run honest actors and execute the first depositor attack. By executing the unpause proposal first, followed by their minting of initial mTokens instead of executing the mToken minting proposal, the first depositor attack is executed, and that mToken deployment becomes compromised.

                _pushCrossChainAction(
                    unitrollerAddress,
                    abi.encodeWithSignature(
                        "_setMintPaused(address,bool)",
                        cTokenAddress,
                        false
                    ),
                    "Unpause MToken market"
                );

        // @audit - Front/back run here
                /// Approvals
                ...

                /// Initialize markets
                _pushCrossChainAction(
                    cTokenAddress,
                    abi.encodeWithSignature("mint(uint256)", initialMintAmount),
                    "Initialize token market to prevent exploit"
                );

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L341-L367

Vulnerability Instance 2

During the gap between the listing of the market and the pausing of minting. In this instance, an attacker could front-run the mint pausing transaction to mint the initial mTokens due to transactions executed by running Forge is not executed atomically. This scenario, however, is not applicable in sequencer-based chains such as Base, where the protocol is intended to be deployed, as there is no mempool for the attacker to observe, and transaction ordering is a privilege reserved solely for the sequencer.

                /// list both mUSDC and mETH in the comptroller
                Comptroller(address(unitroller))._supportMarket(
                    MToken(addresses.getAddress(config.addressesString))
                );

        // @audit - Front/back run here

                /// set mint paused for all MTokens
                Comptroller(address(unitroller))._setMintPaused(
                    MToken(addresses.getAddress(config.addressesString)),
                    true
                );

Proof of Concept

The following Forge test demonstrates how a first depositor attack can be performed on this codebase:

    function testFirstDepositorAttack() public {
        // Setup
        address victim = makeAddr("victim");
        ERC20 mockToken = new ERC20("MockToken", "MTKN");
        Comptroller comptroller = new Comptroller();
        MErc20Immutable mToken = new MErc20Immutable(
            address(mockToken),
            comptroller,
            irModel,
            1e18,
            "Test mToken",
            "mTEST",
            8,
            payable(address(this))
        );
        comptroller._supportMarket(mToken);

        // Attacker mint 1 wei mToken and directly transfer large amount to mToken to inflate exchangeRate
        deal(address(mockToken), address(this), 100 ether);
        mockToken.approve(address(mToken), 1);
        mToken.mint(1);
        mockToken.transfer(address(mToken), 100 ether - 1);

        // Victim deposit normally but receive nothing back since exchangeRate is inflated
        deal(address(mockToken), victim, 1 ether);
        vm.startPrank(victim);
        mockToken.approve(address(mToken), 1 ether);
        mToken.mint(1 ether);
        vm.stopPrank();

        assertEq(mToken.balanceOf(victim), 0);

        // Attacker can redeem all underlying in mToken
        mToken.redeem(1);
        assertEq(mockToken.balanceOf(address(this)), 100 ether + 1 ether);
    }

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L285-L294

Recommended Mitigation Steps

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.

Assessed type

Timing

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #92

c4-judge commented 1 year ago

alcueca marked the issue as not a duplicate

alcueca commented 1 year ago

@ElliotFriedman, this report hinges on the possibility of mip00 not being executed atomically, to exploit the first deposit inflating vulnerability.

The second scenario described is clearly out of scope. On the first scenario I'm not sure about this statement:

As any party holding a valid VAA can execute the proposal after the delay has elapsed, an attacker can front-run honest actors and execute the first depositor attack.

I know how other Timelocks work, but I'm not sure of this one. Can anyone get hold of a valid VAA without any kind of permissions?

ElliotFriedman commented 1 year ago

VAA execution is permissionless as long as the temporal governor is unpaused. However, the first finding is simply untrue that you could front/back run a proposal because the VAA execution is all or nothing, meaning all actions in the gov proposal execute in a single tx.

c4-sponsor commented 1 year ago

ElliotFriedman marked the issue as sponsor disputed