code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

_executeProposal does not check whether intendedRecipient is this contract. As a result, the owner may misoperate and execute messages other than those of this contract, resulting in a double-spend.

Proof of Concept

    function _queueProposal(bytes memory VAA) private {
        /// Checks

        // 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
        // @audit _queueProposal check intendedRecipient here
        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);
    }

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

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

        if (!overrideDelay) {
            require(
                queuedTransactions[vm.hash].queueTime != 0,
                "TemporalGovernor: tx not queued"
            );
            require(
                queuedTransactions[vm.hash].queueTime + proposalDelay <=
                    block.timestamp,
                "TemporalGovernor: timelock not finished"
            );
        } else if (queuedTransactions[vm.hash].queueTime == 0) {
            /// if queue time is 0 due to fast track execution, set it to current block timestamp
            queuedTransactions[vm.hash].queueTime = block.timestamp.toUint248();
        }

        // @audit _executeProposal not check intendedRecipient

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

        /// Interaction (s)

        _sanityCheckPayload(targets, values, calldatas);

        for (uint256 i = 0; i < targets.length; i++) {
            address target = targets[i];
            uint256 value = values[i];
            bytes memory data = calldatas[i];

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

            /// revert on failure with error message if any
            require(success, string(returnData));

            emit ExecutedTransaction(target, value, data);
        }
    }

As you can see from my code comments above, the _executeProposal does not check whether intendedRecipient is this contract. If the execution order is _queueProposal -> _executeProposal, this has no effect, but in addition to the normal delayed execution, the contract also provides a fast way to execute, allowing the owner to execute cross-chain messages without delay:

    function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
        _executeProposal(VAA, true); /// override timestamp checks and execute
    }

This means that the owner can execute cross-chain messages for this contract that are not the target contract. Although the owner is trusted, it is also possible to mistakenly execute messages on non-target contracts, and the user can also execute messages on target contracts, resulting in double-spend.

Tools Used

Manual review

Recommended Mitigation Steps

Check intendedRecipient in _executeProposal

Assessed type

Context

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #308

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory