Flow

Summary

The collectProtocolRevenue and recover functions are vulnerable to reentrancy attacks, allowing a malicious actor to manipulate state variables and potentially drain funds from the contract.

Finding Description

The current implementation of collectProtocolRevenue and recover performs state changes before transferring tokens. This creates a window where a malicious contract could call these functions repeatedly before the state is finalized, leading to a reentrancy attack.

Security Guarantees Affected

This vulnerability breaks the guarantee of atomicity, allowing for unexpected side effects during fund transfers. An attacker could exploit this by:

1. Creating a malicious contract that calls collectProtocolRevenue or recover.

2. Using the fallback function to re-enter the vulnerable function, causing state corruption or loss of funds.

Vulnerability Details

In the collectProtocolRevenue function, the protocol revenue is reset to zero before the token transfer occurs. If the token transfer fails, the revenue is already lost. An attacker can exploit this by forcing a transfer failure and then re-entering the function to collect the revenue again, resulting in double withdrawal.

Impact

The impact is severe as it allows an attacker to withdraw funds multiple times, effectively draining the contract's balance. This can lead to significant financial losses for the protocol and its users.

Proof of Concept

Recommendations

To mitigate the reentrancy risk, you should implement a reentrancy guard using a mutex pattern or similar approach. Here’s a snippet of the fixed code with a basic reentrancy guard:

Suggested Code Snippet

Additional Notes

• Ensure that any function that interacts with external contracts is protected against reentrancy by applying the nonReentrant modifier.

• Consider using OpenZeppelin's ReentrancyGuard for a more robust implementation.

File Location

SablierFlowBase.sol