31497 - [SC - Low] executeBatch lacks payable so ethers can not be...

Submitted on May 20th 2024 at 16:11:09 UTC by @OxRizwan for Boost | Alchemix

Report ID: #31497

Report type: Smart Contract

Report severity: Low

Target: https://immunefi.com

Impacts:

• Smart contract unable to operate due to lack of token funds

• Logic errors

Description

Brief/Intro

executeBatch() lacks payable so ethers can not be a part of bath execution

Vulnerability Details

In TimelockExecutor.sol contract, executeBatch() is used to execute the ready operations containing the batch transactions.

    function executeBatch(
        address[] calldata targets,
        uint256[] calldata values,     @audit // ether value sent along with call
        bytes[] calldata payloads,
        bytes32 predecessor,
        bytes32 descriptionHash,
        uint256 chainId
    ) public virtual onlyRole(EXECUTOR_ROLE) {
        require(targets.length == values.length, "TimelockExecutor: length mismatch");
        require(targets.length == payloads.length, "TimelockExecutor: length mismatch");

        bytes32 id = hashOperationBatch(targets, values, payloads, predecessor, descriptionHash, chainId);

        _beforeCall(id, predecessor);
        for (uint256 i = 0; i < targets.length; ++i) {
            _execute(id, i, targets[i], values[i], payloads[i]);
        }
        _afterCall(id);
    }

This function takes values as a param and part of batch execution. The values are the ethers which are sent along with call. executeBatch() calls internal function execute() for the transactions execution which is implemented as below:

    function _execute(bytes32 id, uint256 index, address target, uint256 value, bytes calldata data) private {
        string memory errorMessage = "Governor: call reverted without message";
@>      (bool success, bytes memory returndata) = target.call{ value: value }(data);
        Address.verifyCallResult(success, returndata, errorMessage);
        require(success, "TimelockExecutor: underlying transaction reverted");

        emit CallExecuted(id, index, target, value, data);
    }

It can be seen at (@), the ether value is indeed a part of executeBatch() function as the value is not hardcoded to 0.

Now, the issue is that, executeBatch() will revert when msg.value > 0. The current implementation of the executeBatch() function within the smart contract lacks the payable keyword. This omission leads to a critical issue where any transaction that attempts to send ether (ETH) to this function will fail. Since the function is designed to allow the executor to execute arbitrary calls and potentially send ETH, the inability to accept ETH due to the missing payable specifier means that:

1. The contract does not behave as intended when interacting with functions or operations requiring ETH transfers.

2. Any attempt to send ETH to this function will revert and result in a failure of the intended operation.

3. ETH sent to this non-payable function will be stuck and effectively lost, leading to financial losses for the users.

Impact Details

executeBatch() will fail when value is greater than 0 so executeBatch() can not be succesfully executed due to this issue

References

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/governance/TimelockExecutor.sol#L275-L293

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/governance/TimelockExecutor.sol#L316

Recommendation to fix

Add payable to executeBatch() function.

Consider below changes:

    function executeBatch(
        address[] calldata targets,
        uint256[] calldata values,
        bytes[] calldata payloads,
        bytes32 predecessor,
        bytes32 descriptionHash,
        uint256 chainId
-    ) public virtual onlyRole(EXECUTOR_ROLE) {
+    ) public payable virtual onlyRole(EXECUTOR_ROLE) {
        require(targets.length == values.length, "TimelockExecutor: length mismatch");
        require(targets.length == payloads.length, "TimelockExecutor: length mismatch");

        bytes32 id = hashOperationBatch(targets, values, payloads, predecessor, descriptionHash, chainId);

        _beforeCall(id, predecessor);
        for (uint256 i = 0; i < targets.length; ++i) {
            _execute(id, i, targets[i], values[i], payloads[i]);
        }
        _afterCall(id);
    }

Proof of Concept

The issue can be easily understood with above description and recommendation to fix.

Additionally, the affected contract i.e TimelockExecutor.sol is actually referred from openzeppelin's TimelockController.sol where this issue is not present. If you see the executeBatch() of openzeppelin's TimelockController.sol then this function has payable keyword so ethers can be part of batch execution. This can be checked as below:

    function executeBatch(
        address[] calldata targets,
        uint256[] calldata values,
        bytes[] calldata payloads,
        bytes32 predecessor,
        bytes32 salt
@>  ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {
        if (targets.length != values.length || targets.length != payloads.length) {
            revert TimelockInvalidOperationLength(targets.length, payloads.length, values.length);
        }

        bytes32 id = hashOperationBatch(targets, values, payloads, predecessor, salt);

        _beforeCall(id, predecessor);
        for (uint256 i = 0; i < targets.length; ++i) {
            address target = targets[i];
            uint256 value = values[i];
            bytes calldata payload = payloads[i];
            _execute(target, value, payload);
            emit CallExecuted(id, i, target, value, payload);
        }
        _afterCall(id);
    }

Reference link: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d947fb056d6a7eb099013076ac5ea5a69e9fec06/contracts/governance/TimelockController.sol#L391