function `_queueProposal` not checking if the required time is passed to allow proposal to set to the queue list

[code423n4]

Lines of code

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

Vulnerability details

Impact

in the _queueProposal function there is no check for if the requested time is passed to allow queue the proposal. in this case any proposal after creating can be added to the queue list.

Proof of Concept

the TemporalGovernor.sol contract have a variable that is used to return the amount of time a proposal need to wait before going into the process(execute or queue):

 /// @notice returns the amount of time a proposal must wait before being processed.
    uint256 public immutable proposalDelay;

but this variable is not used to check if the time is passed to process a proposal in queue function:

function _queueProposal(bytes memory VAA) private {
        //@audit we don't check if the proposal delay is passed or not
        // This call accepts single VAAs and headless VAAs
        (
            IWormhole.VM memory vm,
            bool valid,
            string memory reason
        ) = wormholeBridge.parseAndVerifyVM(VAA);

        // Ensure VAA parsing verification succeeded.
        require(valid, reason);

        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[])
        );

        _sanityCheckPayload(targets, values, calldatas);

        // Very important to check to make sure that the VAA we're processing is specifically designed
        // to be sent to this contract
        require(
            intendedRecipient == address(this),
            "TemporalGovernor: Incorrect destination"
        );

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

        /// Check that the VAA hasn't already been processed (replay protection)
        require(
            queuedTransactions[vm.hash].queueTime == 0,
            "TemporalGovernor: Message already queued"
        );

        /// Effect

        // Add the VAA to queued messages so that it can't be replayed
        queuedTransactions[vm.hash].queueTime = block.timestamp.toUint248();

        emit QueuedTransaction(intendedRecipient, targets, values, calldatas);
    }

in this case every proposal that is created can be set to the queue list by users (the function is PERMISSIONLESS) before the requested time has passed or not:

 /// @notice We explicitly don't care who is relaying this, as long
    /// as the VAA is only processed once AND, critically, intended for this contract.
    /// @param VAA The signed Verified Action Approval to process
    /// @dev callable only when unpaused
    function queueProposal(bytes memory VAA) external whenNotPaused {
        _queueProposal(VAA);
    }

Tools Used

manual review

Recommended Mitigation Steps

recommend to add check if the block.timeStamp is more than or equal to the proposalDelay

Assessed type

Governance

[0xSorryNotSorry]

    * @dev `verifyVM` serves to validate an arbitrary vm against a valid Guardian set
    *  - it aims to make sure the VM is for a known guardianSet
    *  - it aims to ensure the guardianSet is not expired
    *  - it aims to ensure the VM has reached quorum
    *  - it aims to verify the signatures provided against the guardianSet
    */

As per the Wormhole documentation for parseAndVerifyVM function above, the proposal will not pass to be queued due to not being reached to quorum by the quardians and it will not pass the require statement below;

// Ensure VAA parsing verification succeeded.
require(valid, reason);

Invalid assumption.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[alcueca]

On Timelocks, the delay is required on execution, not on queuing AND execution.

Think about it, if you require a delay on queuing, when does the delay time start counting?

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid