code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

Unchecked return value of low level call()/delegatecall() #385

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/delegatexyz/delegate-registry/blob/38576a79bd3781cc719bf7e6ff242cba4420ca8c/src/DelegationRegistry.sol#L239

Vulnerability details

The vulnerability related to an "Unchecked return value of low-level call()/delegatecall()" is a common and critical issue in Ethereum smart contracts. Let's break down this vulnerability and discuss its implications:

  1. Low-Level Calls in Solidity: In Solidity, developers can use low-level functions like call() and delegatecall() to interact with external contracts. These functions allow executing arbitrary code in another contract but have some significant differences:

    • call(): This function allows you to invoke functions in another contract but does not inherit the context (storage, state variables) of the called contract. It returns a boolean value indicating success or failure of the call and also provides access to the returned data.
    • delegatecall(): Similar to call(), this function allows you to invoke functions in another contract, but it does inherit the context of the calling contract, including storage and state variables. It also returns a boolean value for success or failure.
  2. Unchecked Return Values: The vulnerability arises when the return values of these low-level calls are not checked. Typically, after making a call, it's crucial to evaluate whether the call was successful and whether the returned data (if any) is valid or poses any security risks. However, if these checks are omitted, it can lead to various issues:

    • Reentrancy Attacks: If a contract blindly trusts the external contract's call without checking its return value, it might be susceptible to reentrancy attacks. In such an attack, a malicious contract can repeatedly call back into the vulnerable contract before it completes its execution, potentially altering the state in unintended ways.

    • Loss of Funds: If funds are transferred based on the success of a call without validation, an attacker might force a failed call and thereby prevent funds from being transferred correctly.

    • Data Corruption: If the called contract returns data that the calling contract relies on but does not validate it, it might lead to data corruption or manipulation.

  3. Mitigation: To address this vulnerability, developers should always perform the following checks after a low-level call:

    • Check the return value (true for success, false for failure).
    • If the call succeeded, validate the returned data (if any) to ensure it meets the expected format and constraints.

Here's a simplified example of how to mitigate this vulnerability:

(bool success, ) = externalContract.call{value: msg.value}(abi.encodeWithSignature("someFunction()"));
require(success, "External call failed");
// Perform further validation on the returned data if necessary

In summary, failing to check the return values of low-level calls in smart contracts can lead to serious vulnerabilities, including reentrancy attacks, loss of funds, and data corruption. It's essential to diligently validate the results of such calls to ensure the security and integrity of the contract.

Assessed type

Other

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity