Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Missing Reentrancy Protection in SablierFlow Despite CEI Pattern

0bingo76

Summary

While the SablierFlow contract implements the Checks-Effects-Interactions pattern, it lacks explicit reentrancy protection through OpenZeppelin's nonReentrant modifier. This could still allow malicious tokens to reenter the contract through multiple concurrent calls.

https://github.com/Cyfrin/2024-10-sablier/blob/8a2eac7a916080f2022527408b004578b21c51d0/src/SablierFlow.sol#L772

Vulnerability Details

The contract follows CEI but doesn't prevent multiple concurrent executions of the same function. A malicious token could use its transfer function to trigger multiple withdrawals before the first one completes.

Impact

The contract follows CEI but doesn't prevent multiple concurrent executions of the same function. A malicious token could use its `transfer` function to trigger multiple withdrawals before the first one completes. Even with CEI pattern in place, a malicious token could still reenter through these paths:

contract MaliciousToken {
SablierFlow sablier;
uint256 streamId;
uint256 callCount;
​
function transfer(address to, uint256 amount) external returns (bool) {
if (callCount == 0) {
callCount++;
// Even though state is updated, we can still call withdraw again
// because there's no reentrancy guard
sablier.withdraw(streamId, address(this), amount);
}
return true;
}
}
1. First call to `withdraw`:
- Passes all checks
- Updates state
- Calls `safeTransfer`
​
2. During `safeTransfer`, malicious token reenters:
- Makes second call to `withdraw`
- State is already updated, but function can still be called
- Can potentially drain more funds
​

While CEI pattern mitigates some risks, concurrent calls are still possible

Tools Used

manual code review

Recommendations

Add OpenZeppelin's ReentrancyGuard:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

And considered apply the fixes to this functions as well because they modify critical state variables, make external calls to potentially malicious tokens and they all handle token transfers

contract SablierFlow is ReentrancyGuard {
function withdraw(
uint256 streamId,
address to,
uint128 amount
) external nonReentrant returns (uint128, uint128) {
return \_withdraw(streamId, to, amount);
}
​
​
function deposit(
uint256 streamId,
uint128 amount
) external nonReentrant {
_deposit(streamId, amount);
}
​
function depositViaBroker(
uint256 streamId,
uint128 totalAmount,
Broker memory broker
) external nonReentrant {
_depositViaBroker(streamId, totalAmount, broker);
}
​
function refund(
uint256 streamId,
uint128 amount
) external nonReentrant {
_refund(streamId, amount);
}

}

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Appeal created

0bingo76 Submitter
8 months ago
inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.