safdie - Unchecked low-level call in `_call` function will cause unexpected behavior of the contract `SymmioPartyA.sol`

[sherlock-admin3]

safdie

Medium

Unchecked low-level call in _call function will cause unexpected behavior of the contract SymmioPartyA.sol

Summary

The _call function in the SymmioPartyA contract uses a low-level call to interact with the symmioAddress without verifying the success of the call. This can lead to unexpected behavior and potential security risks if the call fails or if symmioAddress is not a contract.

Root Cause

The root cause of this vulnerability is the use of a low-level call without proper checks to ensure the call’s success and to verify that symmioAddress is a valid contract address. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/multiAccount/SymmioPartyA.sol#L47-L49

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. An attacker could set symmioAddress to a non-contract address.
2. When _call is executed, the low-level call would fail silently.
3. The contract would behave unexpectedly, potentially leading to loss of funds or other unintended consequences.

Impact

1. Loss of funds if the contract relies on the success of the call for financial transactions.
2. Unexpected behavior of the contract, leading to potential exploits or denial of service.

PoC

pragma solidity >=0.8.18;

import "@openzeppelin/contracts/access/AccessControl.sol";

contract SymmioPartyA is AccessControl {
    bytes32 public constant MULTIACCOUNT_ROLE = keccak256("MULTIACCOUNT_ROLE");
    address public symmioAddress;

    constructor(address admin, address multiAccountAddress, address symmioAddress_) {
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
        _grantRole(MULTIACCOUNT_ROLE, multiAccountAddress);
        symmioAddress = symmioAddress_;
    }

    event SetSymmioAddress(address oldSymmioContractAddress, address newSymmioContractAddress);

    function setSymmioAddress(address symmioAddress_) external onlyRole(DEFAULT_ADMIN_ROLE) {
        emit SetSymmioAddress(symmioAddress, symmioAddress_);
        symmioAddress = symmioAddress_;
    }

    function _call(bytes memory _callData) external onlyRole(MULTIACCOUNT_ROLE) returns (bool _success, bytes memory _resultData) {
        (bool success, bytes memory result) = symmioAddress.call{ value: 0 }(_callData);
        require(success, "Call failed");
        return (success, result);
    }
}

Mitigation

To mitigate this vulnerability, ensure that the call is checked for success and that symmioAddress is a valid contract address before making the call.

function _call(bytes memory _callData) external onlyRole(MULTIACCOUNT_ROLE) returns (bool _success, bytes memory _resultData) {
    require(symmioAddress.code.length > 0, "Address is not a contract");
    (bool success, bytes memory result) = symmioAddress.call{ value: 0 }(_callData);
    require(success, "Call failed");
    return (success, result);
}