Flow

Summary

State variables are modified before ensuring successful token transfers, leading to potential loss of funds if the transfer fails.

Finding Description

In the collectProtocolRevenue function, the contract resets the protocolRevenue[token] to zero before calling safeTransfer to send the revenue to the specified address. If the safeTransfer call fails (for instance, if the recipient is a contract that reverts the transaction), the state will already reflect that the revenue has been reset, causing the loss of the revenue amount that was intended to be transferred. This breaks the security guarantee of fund integrity, as it allows for scenarios where the contract's state does not accurately reflect the funds it should have.

A malicious actor could exploit this by interacting with the collectProtocolRevenue function in such a way that triggers the transfer failure, resulting in funds being erroneously deducted from the state.

Vulnerability Details

• Function: collectProtocolRevenue(IERC20 token, address to)

• Affected Lines:

  protocolRevenue[token] = 0; // State change occurs here

  token.safeTransfer({ to: to, value: revenue }); // Potential failure point

Impact

This issue can lead to a loss of protocol revenue without any recourse, directly impacting the contract's financial integrity. If the revenue is lost without being transferred, it could undermine user trust and the contract's operational viability.

Proof of Concept

A potential proof of concept involves a malicious token contract that reverts transfers. If a user calls the collectProtocolRevenue function while the recipient is a contract that intentionally reverts transfers, the state variable will be set to zero, and the intended recipient will not receive any funds:

1. User calls collectProtocolRevenue(token, maliciousContractAddress).

2. The contract sets protocolRevenue[token] = 0;.

3. The safeTransfer to maliciousContractAddress fails, reverting the transaction.

4. The contract now incorrectly reflects that there is no protocol revenue available for that token, causing a loss of funds.

Recommendations

To fix this issue, the state should only be modified after confirming that the transfer has succeeded. This can be accomplished by checking the success of the safeTransfer before modifying the state variable. Here’s a revised code snippet:

This modification ensures that the protocol revenue is only reset after a successful transfer, thereby maintaining the integrity of the contract’s state and preventing potential fund loss.

File Location

SablierFlowBase.sol