code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

When executing a call in TemporalGovernor the error is supposed to "bubble" up:

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

File: Governance/TemporalGovernor.sol

399:            // Go make our call, and if it is not successful revert with the error bubbling up
400:            (bool success, bytes memory returnData) = target.call{value: value}(
401:                data
402:            );
403:
404:            /// revert on failure with error message if any
405:            require(success, string(returnData));

However, string(returnData) doesn't do what you expect as reverts are passed abi encoded:

The first 4 bytes of the revert result is the signature, like Error(string) (0x08c379a), similar to a function selector. Then follows the abi encoded string of the error. Simply casting this to a string will result in a string decode error.

Further reading here: https://ethereum.stackexchange.com/questions/83528/how-can-i-get-the-revert-reason-of-a-call-in-solidity-so-that-i-can-use-it-in-th

Hence the reasons for reverts will be misformatted as seen in my PoC below.

Proof of Concept

Test in TemporalGovernorExec.t.sol:

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

        Revert rev = new Revert();

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

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

        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSelector(Revert.iRevert.selector); /// reverts

        /// 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);
        // fails with "�y� Ceci n'est pas une revert"
        vm.expectRevert("Ceci n'est pas une revert");
        governor.executeProposal("");
    }

Revert:

contract Revert {
    function iRevert() pure public {
        revert("Ceci n'est pas une revert");
    }
}

Tools Used

Manual audit

Recommended Mitigation Steps

Consider taking the implementation from uniswap (which originally is from https://ethereum.stackexchange.com/a/83577): https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/Multicall.sol#L16-L23

16:            if (!success) {
17:                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
18:                if (result.length < 68) revert();
19:                assembly {
20:                    result := add(result, 0x04)
21:                }
22:                revert(abi.decode(result, (string)));
23:            }

Assessed type

Error

0xSorryNotSorry commented 1 year ago

While technically valid, the issue doesn't risk the funds.

The submission should have been inside the QA report.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 1 year ago

Valid QA

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)