Flow

Summary

https://github.com/Cyfrin/2024-10-sablier/blob/8a2eac7a916080f2022527408b004578b21c51d0/src/abstracts/Batch.sol#L19

delegatecall is a relatively expensive operation. Using it in a loop can lead to high gas consumption, especially if the loop iterates over a large number of items. Gas costs could even exceed the block’s limit, potentially causing the transaction to fail.

Vulnerability Details

vulnerable code

Using delegatecall in the provided loop has several specific vulnerabilities to be mindful of, particularly due to the fact that delegatecall runs in the context of the caller contract’s storage.

Since delegatecall operates within the caller’s context, it can modify the caller’s state. If any of the calls[i] functions perform external calls, there’s a risk of reentrancy, where an external contract could call back into the original contract before the loop completes.An attacker could exploit this by re-entering the function before state variables are finalized, potentially allowing unauthorized actions or repeated function executions.

If calls[i] includes a function that transfers Ether, an attacker could create a contract that calls back into the function to re-execute the transfer, resulting in double-spending.

Because delegatecall uses the calling contract’s storage, any changes to storage within calls[i] affect the main contract. This could lead to unexpected or corrupted data if the functions in calls are not carefully manage

A call could inadvertently (or maliciously) modify important state variables or interfere with other delegatecall executions, resulting in data inconsistencies or contract malfunction.

Example: If calls[i] contains a function that changes shared state (e.g., count or other variables within the loop), it could alter the flow or result in corrupted data.

If calls[i] is populated with data that isn’t validated, an attacker could inject arbitrary function calls into the contract, which would then be executed within the caller’s context. Malicious code could exploit delegatecall to change important state variables, execute unauthorized actions, or drain funds. If the calls array contains untrusted data from an external source, an attacker could craft malicious function calls that interact with internal contract functions, potentially altering state or stealing assets.

Impact

If any delegatecall within the loop fails (!success), the entire transaction will revert due to revert Errors.BatchError(result);. When one call in the batch fails, all previous successful calls are reverted as well, potentially leading to inefficient transaction retries and increased gas costs. This makes it an all-or-nothing execution, which can be problematic if partial completion is acceptable. delegatecall in a loop as shown can lead to significant vulnerabilities, especially if each calls[i] is not carefully validated or controlled. delegatecall loop can be significant, potentially leading to financial losses, data corruption, or even a total compromise of the contract

Tools Used

Manual

Recommendations

Implement a nonReentrant modifier, like ReentrancyGuard, to prevent reentrancy issues. Alternatively, ensure state changes are finalized before any external calls.

Limit the storage exposure by ensuring that calls only includes trusted functions or implementing a safe design where variables involved in delegatecall are isolated from the main contract’s critical storage.