code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

`TemporalGovernor` can send value but has no `receive` #309

Closed code423n4 closed 11 months ago

code423n4 commented 12 months ago

Lines of code

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

Vulnerability details

Description

Cross chain proposals can contain instructions to send native value. TemporalGovernor then sends this value when doing the call to the target contract:

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

The issue is however that TemporalGovernor has no way to receive value so it will revert since there will not be any value in the contract.

Impact

If a proposal needed requires native value, TemporalGovernor will not be able to execute it, thus impeding on the functionality of the contract. Unless extraordinary methods (SELFDESTRUCT/SENDALL) are taken to fund the contract.

Proof of Concept

Test in TemporalGovernorExec.t.sol:

    // to show it doesn't fail on transferring to `this`
    receive() external payable {} 

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

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

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

        bytes[] memory payloads = new bytes[](1);

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

        // EvmError: OutOfFund
        vm.expectRevert();
        governor.executeProposal("");

        // revert no fallback
        vm.deal(address(this),1000);
        vm.expectRevert();
        payable(address(governor)).transfer(1000);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a receive function.

Assessed type

ETH-Transfer

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as duplicate of #268

c4-judge commented 11 months ago

alcueca marked the issue as satisfactory