Summary
Due to the scaling and descaling of token amounts in the same call, some tokens may be stuck in the contract.
Vulnerability Details
There is a possibility for users to withdraw the max amount of tokens via withdrawMax. Let's take a look on this function:
function withdrawMax(
uint256 streamId,
address to
)
external
override
noDelegateCall
notNull(streamId)
updateMetadata(streamId)
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
uint128 coveredDebt = _coveredDebtOf(streamId);
(withdrawnAmount, protocolFeeAmount) = _withdraw(streamId, to, coveredDebt);
}
First of all it calculates a coveredDedt amount in _coveredDebtOf(streamId). And descale the amount:
function _coveredDebtOf(uint256 streamId) internal view returns (uint128) {
...
@> uint256 totalDebt = _totalDebtOf(streamId);
if (balance < totalDebt) {
return balance;
}
return totalDebt.toUint128();
}
function _totalDebtOf(uint256 streamId) internal view returns (uint256) {
uint256 totalDebtScaled = _ongoingDebtScaledOf(streamId) + _streams[streamId].snapshotDebtScaled;
@> return Helpers.descaleAmount({ amount: totalDebtScaled, decimals: _streams[streamId].tokenDecimals });
}
With the next call to _withdraw() descaled value of coveredDebt (as an amount argument) scaled again:
function _withdraw(
uint256 streamId,
address to,
uint128 amount
)
internal
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
...
@> uint256 amountScaled = Helpers.scaleAmount(amount, tokenDecimals);
...
return (amount, protocolFeeAmount);
}
Now take a look on scaling functions:
function descaleAmount(uint256 amount, uint8 decimals) internal pure returns (uint256) {
if (decimals == 18) {
return amount;
}
unchecked {
uint256 scaleFactor = 10 ** (18 - decimals);
@> return amount / scaleFactor;
}
}
function scaleAmount(uint256 amount, uint8 decimals) internal pure returns (uint256) {
if (decimals == 18) {
return amount;
}
unchecked {
uint256 scaleFactor = 10 ** (18 - decimals);
@> return amount * scaleFactor;
}
}
The token is the same, so are the decimals.
So it basically does a division and then a multiplication with the same amount value.
Eventually the presision is lost and some tokens are stuck on the contract.
Event with admin recovery function they will remain on the contract as the prime value of it stored in the aggregateBalance which is deducted on recovery process:
function recover(IERC20 token, address to) external override onlyAdmin {
@> uint256 surplus = token.balanceOf(address(this)) - aggregateBalance[token];
if (surplus == 0) {
revert Errors.SablierFlowBase_SurplusZero(address(token));
}
token.safeTransfer(to, surplus);
emit Recover(msg.sender, token, to, surplus);
}
Impact
The presision is lost and some tokens are stuck on the contract.
Tools Used
Manual review
Recommendations
Consider adding a bool argument to the _withdraw() function and refining it to work with scaled amounts to avoid additional scaling.
