Flow

Summary

The contract's modifier require checks may not provide sufficient error messages, leading to difficulty in debugging and understanding contract behavior when a check fails.

Finding Description

The contract utilizes modifiers such as notNull, notPaused, and others to enforce specific conditions before executing functions. These modifiers revert the transaction if their conditions are not met, utilizing error codes from the Errors library. However, if these error codes are not descriptive enough or if the logic in the error messages is incorrect, it may lead to confusion for developers and users trying to debug failed transactions.

For example, if a function is called with an invalid streamId, the revert will return a generic error message rather than specifying which condition failed (e.g., whether the stream ID is null or if the stream is paused). This lack of clarity breaks the security guarantee of transparency and may mislead users about the state of the contract.

Vulnerability Details

When a function that utilizes these modifiers is called, the transaction may revert without a clear reason if the error messages are not well-defined. This can potentially lead to:

• User Frustration: Users or developers may spend time troubleshooting issues without clear guidance on what went wrong.

• Difficulty in Auditing: Without clear error messages, external audits may overlook critical issues in the contract, affecting its integrity.

Example of Malicious Input

Consider a scenario where a user attempts to access a stream using an invalid streamId:

1. The user calls a function that checks for notNull(streamId).

2. The modifier triggers a revert with a potentially vague error from the Errors library.

3. The user is unable to discern the exact problem, potentially leading to repeated failed attempts and unnecessary transaction costs.

Impact

This issue has a medium impact because, while it does not lead to immediate security vulnerabilities (like reentrancy or arithmetic errors), it affects the usability and transparency of the contract. Developers relying on accurate error handling may find it challenging to debug their transactions, ultimately affecting the trustworthiness of the contract.

Proof of Concept

To illustrate the issue, here’s how the modifiers are currently implemented in the SablierFlowBase contract:

If _streams[streamId].isStream is false, the revert will occur, but it does not provide insight into whether the stream ID was never initialized or if it was set to null by another function.

Recommendations

To address this issue, the contract should provide clearer error messages in the Errors library. Here’s a suggestion on how to improve the error handling:

1. Enhance Error Definitions: Modify the Errors library to include more descriptive messages for each revert reason.

2. Update Modifier Implementation: Include more detailed error messages in the modifiers to give context to the reverts:

1. Create Specific Error Codes: Instead of using a single error code for various situations, differentiate between the types of null checks, such as:

   • Errors.SablierFlow_StreamDoesNotExist(streamId)

   • Errors.SablierFlow_StreamPaused(streamId)

This will provide users and developers with a clearer understanding of the nature of the issue they are encountering, thereby enhancing the overall security and user experience of the contract.