TemporalGovernor can't execute a proposal that sends ether since it can't receive ether

[code423n4]

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

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #268

[c4-judge]

alcueca marked the issue as satisfactory