`call` in `TemporalGovernor` doesn't check return value

[code423n4]

Lines of code

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

Vulnerability details

Description

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

File: core/Governance/TemporalGovernor.sol

400:            (bool success, bytes memory returnData) = target.call{value: value}(
401:                data
402:            );

According to mip00.sol, which shows the setup for the contracts, TemporalGovernor is going to be admin of the Compound contracts:

• Comptroller/Unitroller
• MErc20s

The issue is that admin calls on these contracts don't revert, they return a failure code, _setCollateralFactor as an example:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Comptroller.sol#L707-L740

File: core/Comptroller.sol

707:    function _setCollateralFactor(MToken mToken, uint newCollateralFactorMantissa) external returns (uint) {
708:        // Check caller is admin
709:        if (msg.sender != admin) {
710:            return fail(Error.UNAUTHORIZED, FailureInfo.SET_COLLATERAL_FACTOR_OWNER_CHECK);
711:        }
712:

            // @audit below are all checks that can fail silently

713:        // Verify market is listed
714:        Market storage market = markets[address(mToken)];
715:        if (!market.isListed) {
716:            return fail(Error.MARKET_NOT_LISTED, FailureInfo.SET_COLLATERAL_FACTOR_NO_EXISTS);
717:        }
718:
719:        Exp memory newCollateralFactorExp = Exp({mantissa: newCollateralFactorMantissa});
720:
721:        // Check collateral factor <= 0.9
722:        Exp memory highLimit = Exp({mantissa: collateralFactorMaxMantissa});
723:        if (lessThanExp(highLimit, newCollateralFactorExp)) {
724:            return fail(Error.INVALID_COLLATERAL_FACTOR, FailureInfo.SET_COLLATERAL_FACTOR_VALIDATION);
725:        }
726:
727:        // If collateral factor != 0, fail if price == 0
728:        if (newCollateralFactorMantissa != 0 && oracle.getUnderlyingPrice(mToken) == 0) {
729:            return fail(Error.PRICE_ERROR, FailureInfo.SET_COLLATERAL_FACTOR_WITHOUT_PRICE);
730:        }

            // ... execution

739:        return uint(Error.NO_ERROR);
740:    }

If a call from TemporalGovernor has any errors it might fail silently as the return code is not checked.

Impact

Important governance tasks can fail silently. Since anyone can execute them and the event emitted will signal success it might take time before governance realize that the intended changes were not made.

Some hints to the failure are given, the Failure event emitted by the failure check and the lack of events emitted on state changes on appropriate contracts and one could investigate the internal call chain of the transaction that performed the executeProposal.

All these require great extra care that might not be given since anyone can call executeProposal

Proof of Concept

Test in TemporalGovernorExec.t.sol:

    function testExecuteProposalFailureCodeOnComptroller() public {
        vm.warp(1); /// queue at 0 to enable testing of message already executed path

        Mocktroller mocktroller = new Mocktroller();

        address[] memory targets = new address[](1);
        targets[0] = address(mocktroller);

        uint256[] memory values = new uint256[](1);

        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSelector(Mocktroller.fails.selector); 

        /// to be unbundled by the temporal governor
        bytes memory payload = abi.encode(
            address(governor),
            targets,
            values,
            payloads
        );

        mockCore.setStorage(
            true,
            trustedChainid,
            governor.addressToBytes(admin),
            "reeeeeee",
            payload
        );

        governor.queueProposal("");

        bytes32 hash = keccak256(abi.encodePacked(""));
        {
            (bool executed, uint248 queueTime) = governor.queuedTransactions(
                hash
            );

            assertEq(queueTime, block.timestamp);
            assertFalse(executed);
        }

        vm.warp(block.timestamp + proposalDelay);

        // succeeds since there is no way to check for failure code
        governor.executeProposal("");
    }

Mocktroller:

contract Mocktroller {

    function fails() public view returns(uint256) {
        return 4; // failure code not 0
    }
}

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a expectedReturnValue to the payload passed with the VAA. If passed, this can then be checked in _executeProposal against the return value of the call

Assessed type

call/delegatecall

[0xSorryNotSorry]

This seems a bit inflated since it's the nature of proposal life cycles;

1. Propose -> Reach Quorum
2. Confirm the payload being as per the needs & proposal
3. Queue
4. Execute and expect Success, else it's not as executed.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[alcueca]

Still, to check the return value on the proposal so that a failed proposal reverts is valid QA. Much better than the proposal failing silently.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a