Potential Security Risk in the 'execOnSubmodule' Function

Summary

I am reporting a potential security risk identified in the implementation of the 'execOnSubmodule' function within the contract.

Vulnerability Detail

The 'execOnSubmodule' function lacks proper verification to ensure that the provided 'callData_' parameter originates from a trusted source. This absence of verification could expose the contract to potential attacks, allowing malicious actors to manipulate the input data and thereby posing a security risk.

Impact

The potential security risk identified poses a threat to the integrity of the 'execOnSubmodule' function, potentially leading to unauthorized actions within the submodule.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/Submodules.sol#L120-127 Submodules.sol

function execOnSubmodule(
    SubKeycode subKeycode_,
    bytes memory callData_
) external permissioned returns (bytes memory) {
    Submodule submodule = _getSubmoduleIfInstalled(subKeycode_);
    // Lack of verification for the safety of 'callData_'
    (bool success, bytes memory returnData) = address(submodule).call(callData_);
    if (!success) revert Module_SubmoduleExecutionReverted(returnData);
    return returnData;
}

Tool used

Manual Review

Recommendation

function execOnSubmodule(
    SubKeycode subKeycode_,
    bytes memory callData_
) external permissioned returns (bytes memory) {
    Submodule submodule = _getSubmoduleIfInstalled(subKeycode_);

    // Verify that 'callData_' is not empty
    require(callData_.length > 0, "Empty call data");  // Line 7: Check for non-empty 'callData_'

    // Implement additional checks for the source of 'callData_'
    require(isTrustedSource(msg.sender), "Untrusted source of call data");

    // Perform the external call with the verified 'callData_'
    (bool success, bytes memory returnData) = address(submodule).call(callData_);

    if (!success) revert Module_SubmoduleExecutionReverted(returnData);

    return returnData;

It is recommended to implement additional validation checks for the input data in the 'execOnSubmodule' function. This verification should ensure that 'callData_' originates from a trusted source. Additionally, consideration can be given to implementing other security measures, such as signature verification, to add an extra layer of protection against potential manipulative attacks.

sherlock-audit / 2023-11-olympus-judging

Irissme - Potential Security Risk in the 'execOnSubmodule' Function #152