Moonwell - sces60107'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#L261

Vulnerability details

Impact

It is said that TemporalGovernor.fastTrackProposalExecution Allows the guardian to process a VAA when the TemporalGovernor is paused. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L261

    /// @notice Allow the guardian to process a VAA when the
    /// Temporal Governor is paused this is only for use during
    /// periods of emergency when the governance on moonbeam is
    /// compromised and we need to stop additional proposals from going through.
    /// @param VAA The signed Verified Action Approval to process

However, it can be executed even if the temporal governor is not paused.

Proof of Concept

TemporalGovernor.fastTrackProposalExecution Allows the guardian to process a VAA when the TemporalGovernor is paused. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L266

    /// @notice Allow the guardian to process a VAA when the
    /// Temporal Governor is paused this is only for use during
    /// periods of emergency when the governance on moonbeam is
    /// compromised and we need to stop additional proposals from going through.
    /// @param VAA The signed Verified Action Approval to process
    function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
        _executeProposal(VAA, true); /// override timestamp checks and execute
    }

But it doesn’t check the pause status of the contract. The guardian can execute it when the temporal governor is not paused.

Tools Used

Manual Review

Recommended Mitigation Steps

Add whenPaused on fastTrackProposalExecution

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

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

TemporalGovernor can execute proposals. But it cannot execute proposals with value != 0. The function doesn’t have a payable modifier. And there is no receive() in the contract.

Proof of Concept

There is no payable modifier on executeProposal and fastTrackProposalExecution. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L237 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L266

    function executeProposal(bytes memory VAA) public whenNotPaused {
        _executeProposal(VAA, false);
    }

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

And the TemporalGovernor doesn’t have a receive() function.

But _executeProposal should be able to make a call with value greater than zero. https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L400

    function _executeProposal(bytes memory VAA, bool overrideDelay) private {
        …

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add payable modifier on those functions or add receive() in TemporalGovernor

Assessed type

ETH-Transfer