Moonwell - RED-LOTUS-REACH's results

Lines of code

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

Vulnerability details

Impact

Summary

TemporalGovernor.sol is missing the ability to transfer any native token while executing proposals.
In the original Compound sources, there is the payable specifier on both the governance and timelock contracts to allow for native tokens to be sent with calls to execute() [1][2].

As a result, if a governance proposal requires native tokens to be transferred while executing proposals this will not be possible, rendering necessary functionality of an effective governance system unavailable. There could be significant financial impact on the protocol if native tokens cannot be transferred within execution of crucial proposals.

Please see (https://www.comp.xyz/t/enable-transfer-eth-from-timelock/2689) for an example of a Compound proposal requiring native token trasnfer.

For example, Compound:

// Compound, GovernorAlpha.sol, line 193
function execute(uint proposalId) public payable {
  // ...payable...
  timelock.executeTransaction{value: proposal.values[i]}(...)
  // ...
}

// Compound Timelock.sol, line 81
function executeTransaction(...) public payable returns (bytes memory) {
  //...payable...
  (bool success, bytes memory returnData) = target.call{value: value}(callData);
  //...
}

In contrast, TemporalGovernor.sol:

// line 237, no payable
function executeProposal(bytes memory VAA) public whenNotPaused {
    _executeProposal(VAA, false);
}

The TemporalGovernor.sol also lacks a receive or fallback function to possibly send actual native token for proposals to be executed.

Links:

• [1]: https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorAlpha.sol#L193-L201
• [2]: https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Timelock.sol#L81-L106

Proof of Concept

pragma solidity 0.8.17;

import "forge-std/Test.sol";
import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
import {MockWormholeCore} from "@test/mock/MockWormholeCore.sol";
import {ITemporalGovernor, TemporalGovernor} from "@protocol/core/Governance/TemporalGovernor.sol";
 
contract TemporalGovernorNoValue is Test {
    TemporalGovernor governor;
    MockWormholeCore mockCore;

    address public wormholeCore;
    uint16 chainId = 42;

    function setUp() public {
        mockCore = new MockWormholeCore();
        wormholeCore = address(mockCore);

        TemporalGovernor.TrustedSender[] memory trustedSenders = new TemporalGovernor.TrustedSender[](1);
        trustedSenders[0] = ITemporalGovernor.TrustedSender({ chainId: chainId, addr: address(this) });

        governor = new TemporalGovernor(wormholeCore, 0 days, 0 days, trustedSenders);

        /////////////////////////////////////
        // Build the payload, set up Wormhole
        /////////////////////////////////////

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

        uint256[] memory values = new uint256[](1);
        values[0] = 0; // no values to send

        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSignature(
            "_setPriceOracle(address)",
            address(0x0)
        );

        bytes memory payload = abi.encode(
            address(governor), // intended recipient
            targets,
            values,
            payloads
        );

        mockCore.setStorage(
            true, // valid
            chainId, // _emitterChainId
            governor.addressToBytes(address(this)), // _emitterAddress
            "UNUSED", // reason
            payload // payload
        );

    }

    function test_fallbackOrReceieve() public {
        governor.queueProposal("");
        
        vm.expectRevert();
        address(governor).call{value: 1 ether}("");
    }
    
    function test_payable() public {
        // THIS WILL NOT COMPILE
        // uncomment to see
        // governor.executeProposal{value: 1 ether}("");
    }
}

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Add a receive or fallback function to the TemporalGovernor.sol contract or make the execute function payable.

Assessed type

Other