Summary
In the SablierFlow contract, the _withdraw function calculates and deducts a protocol fee from the withdrawal amount when the protocol fee is greater than zero.
However, the aggregateBalance mapping, which tracks the total tokens held by the contract on behalf of users, is only decremented by the net withdrawal amount and does not account for the protocol fee protocolFeeAmount. This results in a discrepancy where aggregateBalance[token] overstates the actual amount owed to users by the accumulated protocol fees.
Over time, with numerous withdrawals involving protocol fees, this inconsistency will grow, leading to incorrect surplus calculations in the recover function and the inability to accurately track user balances.
Vulnerability Details
function _withdraw(
uint256 streamId,
address to,
uint128 amount
)
internal
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
if (amount == 0) {
revert Errors.SablierFlow_WithdrawAmountZero(streamId);
}
if (to == address(0)) {
revert Errors.SablierFlow_WithdrawToZeroAddress(streamId);
}
if (to != _ownerOf(streamId) && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierFlow_WithdrawalAddressNotRecipient({ streamId: streamId, caller: msg.sender, to: to });
}
uint8 tokenDecimals = _streams[streamId].tokenDecimals;
uint256 totalDebtScaled = _ongoingDebtScaledOf(streamId) + _streams[streamId].snapshotDebtScaled;
uint256 totalDebt = Helpers.descaleAmount(totalDebtScaled, tokenDecimals);
uint128 balance = _streams[streamId].balance;
uint128 withdrawableAmount;
if (balance < totalDebt) {
withdrawableAmount = balance;
} else {
withdrawableAmount = totalDebt.toUint128();
}
if (amount > withdrawableAmount) {
revert Errors.SablierFlow_Overdraw(streamId, amount, withdrawableAmount);
}
uint256 amountScaled = Helpers.scaleAmount(amount, tokenDecimals);
unchecked {
if (amountScaled <= _streams[streamId].snapshotDebtScaled) {
_streams[streamId].snapshotDebtScaled -= amountScaled;
}
else {
_streams[streamId].snapshotDebtScaled = totalDebtScaled - amountScaled;
_streams[streamId].snapshotTime = uint40(block.timestamp);
}
_streams[streamId].balance -= amount;
}
IERC20 token = _streams[streamId].token;
UD60x18 protocolFee = protocolFee[token];
if (protocolFee > ZERO) {
(protocolFeeAmount, amount) = Helpers.calculateAmountsFromFee({ totalAmount: amount, fee: protocolFee });
unchecked {
protocolRevenue[token] += protocolFeeAmount;
}
}
unchecked {
aggregateBalance[token] -= amount;
}
token.safeTransfer({ to: to, value: amount });
assert(totalDebt - _totalDebtOf(streamId) == balance - _streams[streamId].balance);
emit ISablierFlow.WithdrawFromFlowStream({
streamId: streamId,
to: to,
token: token,
caller: msg.sender,
withdrawAmount: amount,
protocolFeeAmount: protocolFeeAmount
});
return (amount, protocolFeeAmount);
}
Here, the aggregateBalance[token] is decremented only by amount, which is the net withdrawal amount after deducting the protocol fee. The protocolFeeAmount is added to protocolRevenue[token] but not subtracted from aggregateBalance[token]. As a result, aggregateBalance[token] overstates the actual user balances by the accumulated protocolRevenue[token].
Steps:-
The protocol has a "non-zero" fee configured for withdrawals. Multiple users perform withdrawals that incur protocol fees.
Users now perform withdrawals from their streams, incurring protocol fees.
The aggregateBalance[token] is decremented only by the net withdrawal amount, not including the protocolFeeAmount.
Over time, protocolRevenue[token] accumulates, but aggregateBalance[token] does not reflect this accumulation.
The aggregateBalance[token] becomes overstated by the total protocolRevenue[token].
When the admin calls recover, the surplus is miscalculated:
uint256 surplus = token.balanceOf(address(this)) - aggregateBalance[token];
Since aggregateBalance[token] is overstated, surplus may be zero or less than the actual surplus.
The discrepancy arises because the protocol fee remains within the contract's token balance but is not accounted for in aggregateBalance[token].
The aggregateBalance[token] is intended to represent the total of user-owned tokens within the contract.
Failing to adjust aggregateBalance[token] by the protocolFeeAmount results in an inaccurate representation of user balances versus the actual token holdings.
Impact
The mismatch between aggregateBalance[token] and the actual token balance can lead to incorrect surplus calculations. The admin may be unable to recover surplus tokens legitimately belonging to the protocol, affecting the protocol's revenue.
Tools Used
Manual Review
Recommendations
Update the _withdraw function to decrement aggregateBalance[token] by the total amount withdrawn, including the protocol fee:
/// @dev See the documentation for the user-facing functions that call this internal function.
function _withdraw(
uint256 streamId,
address to,
uint128 amount
)
internal
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
// Check: the withdraw amount is not zero.
if (amount == 0) {
revert Errors.SablierFlow_WithdrawAmountZero(streamId);
}
// Check: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierFlow_WithdrawToZeroAddress(streamId);
}
// Check: `msg.sender` is neither the stream's recipient nor an approved third party, the withdrawal address
// must be the recipient.
if (to != _ownerOf(streamId) && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierFlow_WithdrawalAddressNotRecipient({ streamId: streamId, caller: msg.sender, to: to });
}
uint8 tokenDecimals = _streams[streamId].tokenDecimals;
// Calculate the total debt.
uint256 totalDebtScaled = _ongoingDebtScaledOf(streamId) + _streams[streamId].snapshotDebtScaled;
uint256 totalDebt = Helpers.descaleAmount(totalDebtScaled, tokenDecimals);
// Calculate the withdrawable amount.
uint128 balance = _streams[streamId].balance;
uint128 withdrawableAmount;
if (balance < totalDebt) {
// If the stream balance is less than the total debt, the withdrawable amount is the balance.
withdrawableAmount = balance;
} else {
// Otherwise, the withdrawable amount is the total debt.
withdrawableAmount = totalDebt.toUint128();
}
// Check: the withdraw amount is not greater than the withdrawable amount.
if (amount > withdrawableAmount) {
revert Errors.SablierFlow_Overdraw(streamId, amount, withdrawableAmount);
}
// Calculate the amount scaled.
uint256 amountScaled = Helpers.scaleAmount(amount, tokenDecimals);
// Safe to use unchecked, `amount` cannot be greater than the balance or total debt at this point.
unchecked {
// If the amount is less than the snapshot debt, reduce it from the snapshot debt and leave the snapshot
// time unchanged.
if (amountScaled <= _streams[streamId].snapshotDebtScaled) {
_streams[streamId].snapshotDebtScaled -= amountScaled;
}
// Else reduce the amount from the ongoing debt by setting snapshot time to `block.timestamp` and set the
// snapshot debt to the remaining total debt.
else {
_streams[streamId].snapshotDebtScaled = totalDebtScaled - amountScaled;
// Effect: update the stream time.
_streams[streamId].snapshotTime = uint40(block.timestamp);
}
// Effect: update the stream balance.
_streams[streamId].balance -= amount;
}
// Load the variables in memory.
IERC20 token = _streams[streamId].token;
UD60x18 protocolFee = protocolFee[token];
if (protocolFee > ZERO) {
// Calculate the protocol fee amount and the net withdraw amount.
(protocolFeeAmount, amount) = Helpers.calculateAmountsFromFee({ totalAmount: amount, fee: protocolFee });
// Safe to use unchecked because addition cannot overflow.
unchecked {
// Effect: update the protocol revenue.
protocolRevenue[token] += protocolFeeAmount;
}
+ // Effect: update the aggregate balance for the fee.
+ aggregateBalance[token] -= protocolFeeAmount; /// @audit - when `protocolFee > ZERO`
}
unchecked {
// Effect: update the aggregate balance.
aggregateBalance[token] -= amount;
}
// Interaction: perform the ERC-20 transfer.
token.safeTransfer({ to: to, value: amount });
// Protocol Invariant: the difference in total debt should be equal to the difference in the stream balance.
assert(totalDebt - _totalDebtOf(streamId) == balance - _streams[streamId].balance);
// Log the withdrawal.
emit ISablierFlow.WithdrawFromFlowStream({
streamId: streamId,
to: to,
token: token,
caller: msg.sender,
withdrawAmount: amount,
protocolFeeAmount: protocolFeeAmount
});
return (amount, protocolFeeAmount);
}
