Sablier

Description

The withdraw, withdrawMax, and withdrawMaxAndTransfer functions in the SablierV2Lockup contract are designed to handle withdrawals and transfers of funds and NFTs. It might be assumed that there is no need for a reentrancy guard on these functions because, once the funds are withdrawn, there are no funds left to re-enter and withdraw. However, this assumption is not necessarily safe. Reentrancy attacks can exploit the gap between state changes, and without a guard, an attacker could potentially reenter the function before the state is fully updated.

1: Withdraw Function:

2: withdrawMax Function

3: withdrawMaxAndTransfer

Proof of concept

An attacker initiates a call to withdraw, withdrawMax, or withdrawMaxAndTransfer, which triggers the withdrawal of funds.

1: The attacker calls one of the functions.

2: Before the state variables (withdrawn amount, isDepleted flag) are fully updated, the attacker reenters the function.

3: The reentrant call exploits the gap between state changes, potentially leading to multiple withdrawals or incorrect state updates.

Impact

1: Without a reentrancy guard, an attacker could reenter the withdraw, withdrawMax, and withdrawMaxAndTransfer functions before the state variables are fully updated. This could lead to inconsistencies in the contract's state, such as incorrect withdrawn amounts or isDepleted flags.

2: An attacker could exploit the timing of state updates to manipulate the contract's state, potentially leading to unauthorized withdrawals or other malicious activities.

Tools Used

Manual review

Recommendations

Implementing a reentrancy guard, such as the nonReentrant modifier from OpenZeppelin's ReentrancyGuard, can prevent reentrant calls and ensure that state updates are atomic and consistent.

Here is the updated code with Reentrancy Guard: