Guardian could enforce the execution of VAA that is not meant for the TemporalGovernor via `fastTrackProposalExecution`

[code423n4]

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

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #308

[c4-judge]

alcueca marked the issue as satisfactory