Moonwell Cross-Chain Governance - Moonwell

It is important to highlight the _grantGuardian internal function in ConfigurablePauseGuardian contract as follows:

- src/xWELL/ConfigurablePauseGuardian.sol [Line: 93]

    function _grantGuardian(address newPauseGuardian) internal {
        address previousPauseGuardian = newPauseGuardian;
        pauseGuardian = newPauseGuardian;

        /// if a new guardian is granted, the contract is automatically unpaused
        _setPauseTime(0);

        emit PauseGuardianUpdated(previousPauseGuardian, newPauseGuardian);
    }

We can observe that the event PauseGuardianUpdated is emitted with the same values for oldPauseGuardian and newPauseGuardian. This happens because the value attributed for address previousPauseGuardian is newPauseGuardian instead of pauseGuardian.

The MultichainGovernor contract is also impacted because it inherits from ConfigurablePauseGuardian.

It is recommended to modify the _grantGuardian function in ConfigurablePauseGuardian as follows:

    function _grantGuardian(address newPauseGuardian) internal {
        address previousPauseGuardian = pauseGuardian;
        pauseGuardian = newPauseGuardian;

        /// if a new guardian is granted, the contract is automatically unpaused
        _setPauseTime(0);

        emit PauseGuardianUpdated(previousPauseGuardian, newPauseGuardian);
    }

Remediation Plan

SOLVED: The Moonwell team has solved this issue as recommended in the provided code snippet.

The smart contracts under analysis have owners with privileged rights to perform administrative operations and need to be trusted not to act maliciously.

- src/Governance/MultichainGovernor/MultichainVoteCollection.sol [Line: 371]

	    function setGasLimit(uint96 newGasLimit) external onlyOwner {

- src/Governance/MultichainGovernor/MultichainVoteCollection.sol [Line: 382]

	    function setNewStakedWell(address newStakedWell) external onlyOwner {

It is recommended to mitigate centralization issues by implementing multi-signature mechanisms. This prevents a single entity from performing administrative and protected tasks unilaterally.

Remediation Plan

ACKNOWLEDGED: The Moonwell team has acknowledged the issue and informed that potential centralization concerns are mitigated because in this case, the owner of the contract is the Temporal Governor.

In the MultichainGovernor contract, proposals can be executed if they were previously succeeded, with arbitrary calldatas, values and targets contract addresses provided by the creator when calling the propose function, accordingly to the current implemented logic.

- src/Governance/MultichainGovernor.sol [Line: 721]

    function execute(
        uint256 proposalId
    ) external payable override whenNotPaused {
        /// Checks
        require(
            state(proposalId) == ProposalState.Succeeded,
            "MultichainGovernor: proposal can only be executed if it is Succeeded"
        );

        uint256 totalValue = 0;

        Proposal storage proposal = proposals[proposalId];

        for (uint256 i = 0; i < proposal.targets.length; ) {
            totalValue += proposal.values[i];
            unchecked {
                i++;
            }
        }

        require(totalValue == msg.value, "MultichainGovernor: invalid value");

        /// Effects

        proposal.executed = true;

        /// remove the proposal that is about to be executed from all proposals,
        /// and remove from inactive proposals from user list
        _syncTotalLiveProposals();

        /// Interactions

        unchecked {
            for (uint256 i = 0; i < proposal.targets.length; i++) {
                proposal.targets[i].functionCallWithValue(
                    proposal.calldatas[i],
                    proposal.values[i],
                    "MultichainGovernor: execute call failed"
                );
            }
        }

        emit ProposalExecuted(proposalId);
    }

There are no verifications in place that validate whether the values provided by the creator when calling the propose function are legitimate. Likewise, there are no such verifications on the execute function, what could lead to malformed calls being performed to targets.

Moving forward, the call to targets[i].functionCallWithValue will revert in cases the constructed calldatas and values are not valid for that specific target. This could lead to a scenario where the proposal status is set to, ⁣ butsucceeded can never be set to executed because of the failed calls.

It is recommended to perform validation on user-provided data (inputs) when the function propose is called. A whitelist mechanism, similar to the one applied to the break glass functionality would be a good starting point.

By whitelisting possible calldatas and targets, preferably, using mappings, it is possible to ensure that only valid and authorized arguments are passed when calling functionCallWithValue, making it less likely to revert.

Alternatively, enhancing the proposal state management, for example, adding a bool executionFailed element to the Proposal struct or adding ExecutionFailed to the ProposalState enum, both in the IMultichainGovernor could improve the handling of such scenarios. Moving in this direction, it would also be possible to modify the cancel function to handle these scenarios appropriately.

Remediation Plan

ACKNOWLEDGED: The Moonwell team accepted the risk in benefit of the contract being as future-proof as possible, as restricting that calldata only interacts with an allowed list of contracts, or must be executable/would not revert on propose, would be too restrictive and inflexible. The team mentioned it's an intended behavior because the additional contracts and calls the governor might need to make in the future are yet unknown.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state:

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in Official Solidity Documentation.

Remediation Plan

ACKNOWLEDGED: The Moonwell team has acknowledged this finding and has opted not to perform modifications as an style decision.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and adds them to a special data structure known as “topics” instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all of the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 15]

	    event StartBlockSet(uint256 proposalId, uint256 startBlock);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 18]

	    event VoteCast(

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 26]

	    event ProposalCreated(

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 38]

	    event ProposalCanceled(uint256 id);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 41]

	    event ProposalQueued(uint256 id, uint256 eta);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 44]

	    event ProposalExecuted(uint256 id);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 47]

	    event BreakGlassExecuted(

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 54]

	    event QuroumVotesChanged(uint256 oldValue, uint256 newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 57]

	    event ProposalThresholdChanged(uint256 oldValue, uint256 newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 60]

	    event VotingPeriodChanged(uint256 oldValue, uint256 newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 63]

	    event BreakGlassGuardianChanged(address oldValue, address newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 66]

	    event GovernanceReturnAddressChanged(address oldValue, address newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 69]

	    event CrossChainVoteCollectionPeriodChanged(

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 75]

	    event UserMaxProposalsChanged(uint256 oldValue, uint256 newValue);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 83]

	    event CrossChainVoteCollected(

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 94]

	    event CalldataApprovalUpdated(bytes data, bool approved);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 99]

	    event ProposalRebroadcasted(uint256 proposalId, bytes data);

- src/Governance/MultichainGovernor/IMultichainGovernor.sol [Line: 102]

	    event NewStakedWellSet(address newStakedWell, bool toUseTimestamps);

- src/Governance/MultichainGovernor/IMultichainVoteCollection.sol [Line: 17]

	    event ProposalCreated(

- src/Governance/MultichainGovernor/IMultichainVoteCollection.sol [Line: 29]

	    event VotesEmitted(

- src/Governance/MultichainGovernor/IMultichainVoteCollection.sol [Line: 41]

	    event VoteCast(

- src/Governance/MultichainGovernor/IMultichainVoteCollection.sol [Line: 49]

	    event NewStakedWellSet(address newStakedWell);

It is recommended to add the indexed keyword when declaring events, considering the following example:

    event Indexed(
        address indexed from,
        bytes32 indexed id,
        uint indexed value
    );

Remediation Plan

ACKNOWLEDGED: The Moonwell team has acknowledged the finding.