Moonwell - 0xComfyCat's results

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L266 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L352-L389

Vulnerability details

Overview

The Validated Action Attestation (VAA) includes an intendedRecipient field that is employed to ascertain the designated receiver of a message. Considering that the Wormhole message is multicasted, it is advisable to authenticate the intendedRecipient at the receiving end.

Impact

In the context of the TemporalGovernor, the validation is performed during _queueProposal, thus ensuring that any queued proposal is specifically intended for the TemporalGovernor. It is therefore deemed acceptable to forgo this verification during execution in _executeProposal. However, if the intendedRecipient is not verified, the Guardian could potentially enforce the execution of a VAA that is not meant for the TemporalGovernor contract, by means of fastTrackProposalExecution.

Proof of Concept

The following Forge test demonstrates that TemporalGovernor's fastTrackProposalExecution does not reject a VAA that incorporates an invalid intendedRecipient.

    function testTemporalGovernorIntendedRecipientCheck() public {
        // Setup
        address mockWormholeCore = address(1234);
        ITemporalGovernor.TrustedSender[] memory trustedSenders = new ITemporalGovernor.TrustedSender[](1);
        trustedSenders[0] = ITemporalGovernor.TrustedSender(1, address(this));
        TemporalGovernor gov = new TemporalGovernor(mockWormholeCore, 1 days, 0, trustedSenders);

        IWormhole.VM memory mockVM;
        mockVM.emitterChainId = 1;
        mockVM.emitterAddress = bytes32(bytes20(address(this))) >> 96;
        address[] memory targets = new address[](1);
        uint256[] memory values = new uint256[](1);
        bytes[] memory datas = new bytes[](1);

        // intendedRecipient is not TemporalGovernor
        mockVM.payload = abi.encode(address(123), targets, values, datas);
        vm.mockCall(mockWormholeCore, abi.encodeWithSignature("parseAndVerifyVM(bytes)"), abi.encode(mockVM, true, ""));

        // queueProposal rejects
        vm.expectRevert("TemporalGovernor: Incorrect destination");
        gov.queueProposal("");

        // fastTrackProposalExecution accept and execute it
        vm.expectCall(address(0), "", 1);
        gov.fastTrackProposalExecution("");
    }

Recommended Mitigation Steps

Incorporate an intendedRecipient validation into the internal function _executeProposal.

        // Ensure the emitterAddress of this VAA is a trusted address
        require(
            trustedSenders[vm.emitterChainId].contains(vm.emitterAddress), /// allow multiple per chainid
            "TemporalGovernor: Invalid Emitter Address"
        );

        require(
            !queuedTransactions[vm.hash].executed,
            "TemporalGovernor: tx already executed"
        );

        queuedTransactions[vm.hash].executed = true;
        
        address intendedRecipient;
        address[] memory targets; /// contracts to call
        uint256[] memory values; /// native token amount to send
        bytes[] memory calldatas; /// calldata to send
        (intendedRecipient , targets, values, calldatas) = abi.decode(
            vm.payload,
            (address, address[], uint256[], bytes[])
        );

        // @audit - To prevent fastracking wrong intendedRecipient, we check it again here
        require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L27-L424 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L382-L388

Vulnerability details

Impact

It is conceivable that a proposal might incorporate a call that transmits ether. During _executeProposal, the values array from the payload is decoded and an attempt to send it is made accordingly.

        address[] memory targets; /// contracts to call
        uint256[] memory values; /// native token amount to send
        bytes[] memory calldatas; /// calldata to send
        (, targets, values, calldatas) = abi.decode(
            vm.payload,
            (address, address[], uint256[], bytes[])
        );

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

While the call's implementation is correct, executing such a proposal is not feasible. The contract lacks any means to receive ether, as there are no payable functions or receive functions. Hence, it is not possible for anyone to fund the TemporalGovernor with ether, except via selfdestruct, which is not advisable.

Proof of Concept

The following Forge tests demonstrate that:

1. TemporalGovernor is unable to receive ether via conventional methods.
2. TemporalGovernor is incapable of executing a proposal that includes a call to send ether.

    function testTemporalGovernorCannotExecuteSendEther() public {
        // Setup
        address mockWormholeCore = address(1234);
        ITemporalGovernor.TrustedSender[] memory trustedSenders = new ITemporalGovernor.TrustedSender[](1);
        trustedSenders[0] = ITemporalGovernor.TrustedSender(1, address(this));
        TemporalGovernor gov = new TemporalGovernor(mockWormholeCore, 1 days, 0, trustedSenders);

        // Can't send ether to TemporalGovernor
        deal(address(this), 1 ether);
        vm.expectRevert();
        payable(address(gov)).transfer(1 ether);

        // Mock proposal that attempt to sends ether
        IWormhole.VM memory mockVM;
        mockVM.emitterChainId = 1;
        mockVM.emitterAddress = bytes32(bytes20(address(this))) >> 96;
        address[] memory targets = new address[](1);
        uint256[] memory values = new uint256[](1);
        values[0] = 1 ether;
        bytes[] memory datas = new bytes[](1);
        mockVM.payload = abi.encode(address(gov), targets, values, datas);
        vm.mockCall(mockWormholeCore, abi.encodeWithSignature("parseAndVerifyVM(bytes)"), abi.encode(mockVM, true, ""));

        // Can queue but can't execute
        gov.queueProposal("");
        vm.expectRevert();
        gov.executeProposal("");
    }

Tools Used

• Manual inspection
• Visual Studio Code
• Foundry

Recommended Mitigation Steps

There are two possible mitigation strategies, contingent upon whether or not the TemporalGovernor needs to send ether:

1. If the transmission of ether is necessary, consider adding a receive fallback function or a payable function to accept ether.
2. If the transmission of ether is not required, eliminate the ability to send ether by completely disregarding the values field in the payload.

Assessed type

Payable

Table of Contents

Low risk findings

1. [L-01] Indefinite delay risk in proposals incorporating _reduceReserve call
2. [L-02] WETHRouter mint and redeem would revert when transfer is paused
3. [L-03] Lack of token decimal validation in mToken initialization

QA findings

1. [Q-01] Shorter contract lifespan due to storing timestamps in uint32
2. [Q-02] Unused variable borrowerIndex in repayBorrowFresh

[L-01] Indefinite delay risk in proposals incorporating _reduceReserve call

The _reduceReserve function, which initiates the transfer of tokens out of the mToken contract, inherently necessitates a sufficient balance within the mToken contract to facilitate the transfer.

        // Fail gracefully if protocol has insufficient underlying cash
        if (getCashPrior() < reduceAmount) {
            return fail(Error.TOKEN_INSUFFICIENT_CASH, FailureInfo.REDUCE_RESERVES_CASH_NOT_AVAILABLE);
        }

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

Potential exists for an ill-intentioned actor to manipulate the cash reserve through borrowing, consequently preventing the execution of the proposal. It is prudent, therefore, to avoid combining the _reduceReserve function with other high-stakes calls such as lowering the collateral factor in the same proposal.

[L-02] WETHRouter mint and redeem would revert when transfer is paused

The mint and redeem functions of the WETHRouter involve the transfer of mTokens as part of the wrapping process. Consequently, these functions may cease to function if the transfer operation is paused. It is, therefore, recommended that the DApp frontend be designed to handle this particular case seamlessly, enabling the user to mint/redeem as WETH via mToken even in the event of a transfer pause.

    function mint(address recipient) external payable {
        weth.deposit{value: msg.value}();

        require(mToken.mint(msg.value) == 0, "WETHRouter: mint failed");

        // @audit - Revert if transfer is paused
        IERC20(address(mToken)).safeTransfer(
            recipient,
            mToken.balanceOf(address(this))
        );
    }
    
    function redeem(uint256 mTokenRedeemAmount, address recipient) external {
	    // @audit - Revert if transfer is paused
        IERC20(address(mToken)).safeTransferFrom(
            msg.sender,
            address(this),
            mTokenRedeemAmount
        );
        ...
    }

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/router/WETHRouter.sol#L36-L39 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/router/WETHRouter.sol#L46-L50

[L-03] Lack of token decimal validation in mToken initialization

The design of ChainlinkOracle is such that it does not accommodate the support of underlying tokens that have more than 18 decimals. This necessary verification appears to be absent in the initialization process of mToken.

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

[Q-01] Shorter contract lifespan due to storing timestamps in uint32

With uint32, contract would last until 2106

[Q-02] Unused variable borrowerIndex in repayBorrowFresh

        /* We remember the original borrowerIndex for verification purposes */
        vars.borrowerIndex = accountBorrows[borrower].interestIndex;

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