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

1 stars 0 forks source link

No Receive functiion in TemporalGovernor contract #333

Closed code423n4 closed 11 months ago

code423n4 commented 12 months ago

Lines of code

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

Vulnerability details

Impact

The function _executeProposal in TemporalGovernor contract will fail, if there is a value to send with the call to the targets

_executeProposal function could send native token out along with a call to the targets encoded in vm.payload, but the current implementation has no recieve function in place to receive any native tokens

This will make the function unusable when a value is to be sent along with the call, as all subsequent calls with not enough native token balance in the contract will revert this calls

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

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

I Recommend adding a receive function to receive native tokens, the receive function can then be restricted to accept tokens from only trusted addresses

Assessed type

call/delegatecall

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

c4-judge commented 11 months ago

alcueca marked the issue as partial-50

c4-judge commented 11 months ago

alcueca changed the severity to 2 (Med Risk)