no type safety on ```target``` and ```value``` parameters in ```_call``` function

[mehdi-defiesta]

Describe the bug There is no type safety on data and target parameters in call function in OperatorV1_1.sol. Any operator is able to send the entire balance of the contract to address(0) (or any non existing address) leading to funds loss. Operator could send funds to their EOA address as well.

code snippet:

    // @audit no type safety on data parameter => vulnerabilities if the input is not properly validated
    function _call(address target, uint256 value, bytes memory data) internal {
        (bool success, bytes memory result) = target.call{value : value}(data);
        if (!success) {
            assembly {
                revert(add(result, 32), mload(result))
            }
        }
    }

Configuration

• Severity: Low

Impact Any operator is able to send the entire balance of the contract to address(0) (or any non existing address) leading to funds loss. Operator could send funds to their EOA address as well.

Recommendation We must add an address(0) safety check and implement an ìsContract function in order to avoid sending ETH to the wrong target. Moreover, It seems important to add a gas parameter to the call function in the case a sequencing function is abnormally gas consuming.

function _call(address target, uint256 value, bytes memory data) internal {
        require(target != address(0), "Target address cannot be zero");
        require(isContract(target), "Target must be a contract");

        (bool success, bytes memory result) = target.call{value: value, gas: 5000}(data);
        if (!success) {
            // Bubble up the revert reason
            if (result.length > 0) {
                assembly {
                    let returndata_size := mload(result)
                    revert(add(32, result), returndata_size)
                }
            } else {
                revert("Call failed without a revert reason");
            }
        }

        emit CallResult(success, result);
    }

    function isContract(address account) internal view returns (bool) {
        uint256 size;
        assembly {
            size := extcodesize(account)
        }
        return size > 0;
    }

[Zena-park]

Your opinion is very reasonable. Thank you

The vulnerability exists to have been caused by adding an undefined function(powerful function). I will remove functions (execute, executeBatch) in the operator.
@mehdi-defiesta What do you think? It will be better to upgrade the contract when a specifically needed function arises.

[mehdi-defiesta]

I agree. I guess we should either remove the execute and executeBatch function or add more safety checks. Severity is Low though because we should assume operators are trusted.

[Zena-park]

@mehdi-defiesta I deleted them.

[parth-15]

@mehdi-defiesta @Zena-park I don't agree with recommendation of adding fixed gas like 5000 or any specific arbitrary number. The gas consumed may differ on different calldata and execution path it takes. Also, there may be the possibility that even if the function can run with 5000 gas in this evm version, next evm upgrade may change the gas for specific opcode which can break the execution. It's not suggested to use transfer and send due to this specific reason.

[mehdi-defiesta]

5000 was arbitrary. Since we do not know the exact function that is going to be passed into the call, we cannot express ourselves on how much max gas the caller should use. That's why it's better to not keep the function for the moment.

[Zena-park]

I'm trying to delete the execute and executeBatch functions. This is because it is a function that does not need to be used in the Operator contract yet. @parth-15 Are you opposed to deleting it?

[parth-15]

@Zena-park I am not opposed to removing it. I was opposing the fix of forwarding arbitrary gas . But if you remove the function, it's all okay.