Moonwell - T1MOH's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L187-L198

Vulnerability details

Impact

fastTrackProposalExecution() is used to execute specific proposal while TemporalGovernor is paused. There is invariant in contract that guardian must be not allowed to pause when contract is paused. However this invariant can be broken unintentionally. And the only way to recover in this situation is to revoke guardian. Then protocol won't be able to execute fastTrackProposals and to pause governance.

Proof of Concept

Let's take a look at the following scenario:

1. Emergency situation occurred, one of these assumptions is broken: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L16-L19

/// There are a few assumptions that are made in this contract:
/// 1. Wormhole is secure and will not send malicious messages or be deactivated.
/// 2. Moonbeam is secure.
/// 3. Governance on Moonbeam cannot be compromised.

2. First of all guardian pauses contract, and now guardianPauseAllowed = false: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L270-L290

    function togglePause() external onlyOwner {
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

3. Then guardian executes fastTrackProposal, it contains action grantGuardiansPause() to allow pausing again. Now guardianPauseAllowed = true https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L187-L198

    function grantGuardiansPause() external {
        require(
            msg.sender == address(this),
            "TemporalGovernor: Only this contract can update grant guardian pause"
        );

        guardianPauseAllowed = true;
        lastPauseTime = 0;

        emit GuardianPauseGranted(block.timestamp);
    }

4. As a result, there was no adequate option left to unpause protocol.

There are 3 ways to call unpause:

1. togglePause() doesn't pass assert statement, because guardianPauseAllowed == true

    function togglePause() external onlyOwner {
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

2. permissionlessUnpause() also doesn't pass assert statement

    function permissionlessUnpause() external whenPaused {
        require(
            lastPauseTime + permissionlessUnpauseTime <= block.timestamp,
            "TemporalGovernor: not past pause window"
        );

        lastPauseTime = 0;
        _unpause();

        assert(!guardianPauseAllowed); /// this should never revert, statement for SMT solving

        emit PermissionlessUnpaused(block.timestamp);
    }

3. The only way left is to call revokeGuardian(). But you agree this is not ideal

    function revokeGuardian() external {
        address oldGuardian = owner();
        require(
            msg.sender == oldGuardian || msg.sender == address(this),
            "TemporalGovernor: cannot revoke guardian"
        );

        _transferOwnership(address(0));
        guardianPauseAllowed = false;
        lastPauseTime = 0;

        if (paused()) {
            _unpause();
        }

        emit GuardianRevoked(oldGuardian);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that current state is unpaused in grantGuardiansPause():

-   function grantGuardiansPause() external {
+   function grantGuardiansPause() external whenNotPaused {
        require(
            msg.sender == address(this),
            "TemporalGovernor: Only this contract can update grant guardian pause"
        );

        guardianPauseAllowed = true;
        lastPauseTime = 0;

        emit GuardianPauseGranted(block.timestamp);
    }

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L237

Vulnerability details

Impact

Proposals with value != 0 can't be executed because TemporalGovernor.sol doesn't have payable functions

Proof of Concept

    function executeProposal(bytes memory VAA) public whenNotPaused {
        _executeProposal(VAA, false);
    }

But proposal is executed with value: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L399-L402

            // Go make our call, and if it is not successful revert with the error bubbling up
            (bool success, bytes memory returnData) = target.call{value: value}(
                data
            );

Tools Used

Manual Review

Recommended Mitigation Steps

Add receive() external payable {} Or make executeProposal payable

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/Configs.sol#L55

Vulnerability details

Impact

At the time of deploy, deployer initializes token markets with initial amount to prevent exploit. At least USDC and WETH markets will be initialized during deploy. But initialMintAmount is hardcoded to 1e18 which is of for WETH (1,876 USD), but unrealistic for USDC (1,000,000,000,000 USD). Therefore deploy will fail.

Proof of Concept

Here initialMintAmount = 1 ether: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/Configs.sol#L55

    /// @notice initial mToken mint amount
    uint256 public constant initialMintAmount = 1 ether;

Here this amount is approved to MToken contract and supplied to mint MToken: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/mips/mip00.sol#L334-L379

            for (uint256 i = 0; i < cTokenConfigs.length; i++) {
                Configs.CTokenConfiguration memory config = cTokenConfigs[i];

                address cTokenAddress = addresses.getAddress(
                    config.addressesString
                );

               ...

                /// Approvals
                _pushCrossChainAction(
                    config.tokenAddress,
                    abi.encodeWithSignature(
                        "approve(address,uint256)",
                        cTokenAddress,
                        initialMintAmount
                    ),
                    "Approve underlying token to be spent by market"
                );

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

                ...
            }

Tools Used

Manual Review

Recommended Mitigation Steps

Specify initialMintAmount for every token separately in config

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/Addresses.sol#L50

Vulnerability details

Impact

Wormhole Bridge will have incorrect address, which disables whole TemporalGovernance contract and therefore administration of Moonbeam. But during deployment markets are initialized with initial token amounts to prevent exploits, therefore TemporalGovernance must own this initial tokens. But because of incorrect bridge address TemporalGovernance can't perform any action and these tokens are brick. Protocol needs redeployment and loses initial tokens.

Proof of Concept

To grab this report easily I divided it into 3 parts

1. Why WORMHOLE_CORE set incorrectly There is no config for Base Mainnet, however this comment states for used parameters: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/mips/mip00.sol#L55-L61

    /// --------------------------------------------------------------------------------------------------///
    /// Chain Name	       Wormhole Chain ID   Network ID	Address                                      |///
    ///  Ethereum (Goerli)   	  2	                5	    0x706abc4E45D419950511e474C7B9Ed348A4a716c   |///
    ///  Ethereum (Sepolia)	  10002          11155111	    0x4a8bc80Ed5a4067f1CCf107057b8270E0cC11A78   |///
    ///  Base	                 30    	        84531	    0xA31aa3FDb7aF7Db93d18DDA4e19F811342EDF780   |///
    ///  Moonbeam	             16	             1284 	    0xC8e2b0cD52Cf01b0Ce87d389Daa3d414d4cE29f3   |///
    /// --------------------------------------------------------------------------------------------------///

Why to treat this comment? Other parameters are correct except Base Network ID. But Base Network ID has reflection in code, described in #114. 0xA31aa3FDb7aF7Db93d18DDA4e19F811342EDF780 is address of Wormhole Token Bridge on Base Testnet according to docs and this address has no code in Base Mainnet

2. Why governor can't execute proposals with incorrect address of Wormhole Bridge Proposal execution will revert here because address wormholeBridge doesn't contain code: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L344-L350

    function _executeProposal(bytes memory VAA, bool overrideDelay) private {
        // This call accepts single VAAs and headless VAAs
        (
            IWormhole.VM memory vm,
            bool valid,
            string memory reason
        ) = wormholeBridge.parseAndVerifyVM(VAA);
        ...
    }

3. Impact of inability to execute proposals in TemporalGovernor.sol Here you can see that deployer should provide initial amounts of tokens to initialize markets:

    /// @notice the deployer should have both USDC, WETH and any other assets that will be started as
    /// listed to be able to deploy on base. This allows the deployer to be able to initialize the
    /// markets with a balance to avoid exploits
    function deploy(Addresses addresses, address) public {
    ...
    }

And here governor approves underlying token to MToken and mints initial mTokens. It means that governor has tokens on balance.

    function build(Addresses addresses) public {
        /// Unitroller configuration
        _pushCrossChainAction(
            addresses.getAddress("UNITROLLER"),
            abi.encodeWithSignature("_acceptAdmin()"),
            "Temporal governor accepts admin on Unitroller"
        );

        ...

        /// set mint unpaused for all of the deployed MTokens
        unchecked {
            for (uint256 i = 0; i < cTokenConfigs.length; i++) {
                Configs.CTokenConfiguration memory config = cTokenConfigs[i];

                address cTokenAddress = addresses.getAddress(
                    config.addressesString
                );

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

                /// Approvals
                _pushCrossChainAction(
                    config.tokenAddress,
                    abi.encodeWithSignature(
                        "approve(address,uint256)",
                        cTokenAddress,
                        initialMintAmount
                    ),
                    "Approve underlying token to be spent by market"
                );

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

                ...
            }
        }

        ...
    }

To conclude, there is no way to return the tokens from the TemporalGovernor intended for market initialization

Tools Used

Manual Review

Recommended Mitigation Steps

Change WORMHOLE_CORE to 0xbebdb6C8ddC678FfA9f8748f85C815C556Dd8ac6 according to docs

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/utils/ChainIds.sol#L7

Vulnerability details

Impact

Incorrect chainId of Base in deploy parameters results in incorrect deploy and subsequent redeployment

Proof of Concept

Contract ChainIds.sol is responsible for mapping chainId -> wormholeChainId which is used in contract Addresses to associate contract name with its address on specific chain. Addresses is the main contract which keeps track of all dependency addresses and passed into main deploy() and here addresses accessed via block.chainId: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/test/proposals/mips/mip00.sol#L77

    function deploy(Addresses addresses, address) public {
        ...
            trustedSenders[0].chainId = chainIdToWormHoleId[block.chainid];
            
        ...
            memory cTokenConfigs = getCTokenConfigurations(block.chainid);
    }

Here you can see that Network ID of Base set to 84531. But actual network id is 8453 from Base docs

contract ChainIds {
    uint256 public constant baseChainId = 84531;
    uint16 public constant baseWormholeChainId = 30; /// TODO update when actual base chain id is known
    
    uint256 public constant baseGoerliChainId = 84531;
    uint16 public constant baseGoerliWormholeChainId = 30;
    
    ...

    constructor() {
        ...
        chainIdToWormHoleId[baseChainId] = moonBeamWormholeChainId; /// base deployment is owned by moonbeam governance
        ...
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Change Base Network ID to 8453

Assessed type

Other

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");
    }

Assessed type

Invalid Validation