-
- L-01.
SablierFlowBaseLacksEIP-165Compliance forEIP4906Interface Support - L-02. It is possible to avoid paying the
protocolFee - L-03. depletionTimeOf() can return 0 and a timestamp value when balance and debt relation didn't change
- L-04. Flow stream cannot be created for tokens that do not implement the
decimalsfunction
- L-01.
- High: 0
- Medium: 0
- Low: 4
Submitted by chista0x, zxriptor, 0xtheblackpanther, brene, baz1ka, liquidbuddha, ililhunterlili, spuriousdragon, inh3l, rzizah, cayde, ChainDefenders. Selected submission by: chista0x.
The SablierFlowBase contract does not adhere to the EIP4906 standard, as it fails to implement the required
supportsInterface(bytes4) function to confirm compatibility with the IERC4906 interface. This omission can cause
integration issues for contracts that rely on EIP-165 to verify interface support.
The ISablierFlowBase interface inherits from IERC4906, but the SablierFlowBase contract does not override the
supportsInterface() function.
interface ISablierFlowBase is
IERC4906, // 2 inherited components
IERC721Metadata, // 2 inherited components
IAdminable // 0 inherited components
The lack of a proper supportsInterface implementation can cause significant compatibility issues. Systems that check
for EIP-165 compliance may fail to interact correctly with the SablierFlowBase contract. This could lead to failures
in contract interactions, integrations, and potentially disrupt decentralized applications relying on SablierFlowBase.
Add the following test file to the test folder:
tests\Chista0xAudit.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22;
import { IERC4906 } from "@openzeppelin/contracts/interfaces/IERC4906.sol";
import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import { IERC721Metadata } from "@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol";
import { Base_Test } from "./Base.t.sol";
import { console } from "forge-std/src/console.sol";
contract Chista0xAudit is Base_Test {
function test_supportsInterface_IERC165() external view {
bool res = flow.supportsInterface(type(IERC165).interfaceId);
assertEq(res, true, "EIP165 interface not supported");
}
function test_supportsInterface_IERC721() external view {
bool res = flow.supportsInterface(type(IERC721).interfaceId);
assertEq(res, true, "IERC721 interface not supported");
}
function test_supportsInterface_IERC721Metadata() external view {
bool res = flow.supportsInterface(type(IERC721Metadata).interfaceId);
assertEq(res, true, "IERC721Metadata interface not supported");
}
function test_supportsInterface_IERC4906() external view {
bool res = flow.supportsInterface(type(IERC4906).interfaceId);
assertEq(res, true, "IERC4906 interface not supported");
}
}
Run the test with the command forge test --mc Chista0xAudit
Test output:
Ran 4 tests for tests/Chista0xAudit.t.sol:Chista0xAudit
[PASS] test_supportsInterface_IERC165() (gas: 9961)
[FAIL. Reason: IERC4906 interface not supported: false != true] test_supportsInterface_IERC4906() (gas: 9931)
[PASS] test_supportsInterface_IERC721() (gas: 9882)
[PASS] test_supportsInterface_IERC721Metadata() (gas: 9997)
Suite result: FAILED. 3 passed; 1 failed; 0 skipped; finished in 27.76ms (1.42ms CPU time)
Ran 1 test suite in 30.65ms (27.76ms CPU time): 3 tests passed, 1 failed, 0 skipped (4 total tests)
Failing tests:
Encountered 1 failing test in tests/Chista0xAudit.t.sol:Chista0xAudit
[FAIL. Reason: IERC4906 interface not supported: false != true] test_supportsInterface_IERC4906() (gas: 9931)
add supportsInterface function to the SablierFlowBase contract.
src\abstracts\SablierFlowBase.sol:
...
+ import { IERC4906 } from "@openzeppelin/contracts/interfaces/IERC4906.sol";
+ import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
...
abstract contract SablierFlowBase is
Adminable, // 1 inherited component
ISablierFlowBase, // 5 inherited component
ERC721 // 6 inherited components
{
...
+ function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721, IERC165) returns (bool) {
+ return interfaceId == type(IERC4906).interfaceId || super.supportsInterface(interfaceId);
+ }
...
}
Manual code review / Foundry tests
Submitted by ljj, 0xstalin, strapontin. Selected submission by: ljj.
It is possible to avoid paying the protocolFee via withdrawing small amounts of tokens. Although gas fees for
transactions would deter users from withdrawing small amounts, considering that the Sablier protocol is supporting any
ERC20 token that has less than 19 decimals, this becomes a valid attack vector for tokens with little decimals.
Sablier admin can implement protocol fees for a token with the setProtocolFee function found in the
SablierFlowBase.sol contract.
function setProtocolFee(IERC20 token, UD60x18 newProtocolFee) external override onlyAdmin {
// Check: the new protocol fee is not greater than the maximum allowed.
if (newProtocolFee > MAX_FEE) {
revert Errors.SablierFlowBase_ProtocolFeeTooHigh(newProtocolFee, MAX_FEE);
}
UD60x18 oldProtocolFee = protocolFee[token];
// Effects: set the new protocol fee.
protocolFee[token] = newProtocolFee;
// Log the change of the protocol fee.
emit ISablierFlowBase.SetProtocolFee({
admin: msg.sender,
token: token,
oldProtocolFee: oldProtocolFee,
newProtocolFee: newProtocolFee
});
// Refresh the NFT metadata for all streams.
emit BatchMetadataUpdate({ _fromTokenId: 1, _toTokenId: nextStreamId - 1 });
}This protocolFee is applied when a user calls the withdraw function which calls the _withdraw in the
SablierFlow.sol contract.
function _withdraw(
uint256 streamId,
address to,
uint128 amount
)
internal
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
// rest of the function
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;
}
}
unchecked {
// Effect: update the aggregate balance.
aggregateBalance[token] -= amount;
}
// Interaction: perform the ERC-20 transfer.
token.safeTransfer({ to: to, value: amount });
// rest of the function
}As seen from this function, the protocolFee that will be applied is calculated in the calculateAmountsFromFee function
in the Helpers.sol contract.
function calculateAmountsFromFee(
uint128 totalAmount,
UD60x18 fee
)
internal
view
returns (uint128 feeAmount, uint128 netAmount)
{
// Calculate the fee amount based on the fee percentage.
feeAmount = ud(totalAmount).mul(fee).intoUint128();
// Calculate the net amount after subtracting the fee from the total amount.
netAmount = totalAmount - feeAmount;
}Due to the use of UD60x18 math, in low
totalAmount inputs, the feeAmount will return 0. This can be verified by adding the following line in the function.
console.log("fee amount:", feeAmount);This creates an attack vector where users can withdraw small amounts of tokens (e.g. withdraw 9 tokens at a time when
fee is 10%) to avoid paying fees. Tokens with industry standard decimals (decimals >= 6) make this attack vector
unlikely as malicious actors would have to pay a lot of gas fees to avoid paying protocolFee. However, as specified in
the Scope of the audit:
Any ERC-20 token can be used with Flow as long as it adheres to the following assumptions:
- The total supply of any ERC-20 token remains below
$(2^{128} - 1)$ , i.e.,type(uint128).max.- The
transferandtransferFrommethods of any ERC-20 token strictly reduce the sender's balance by the transfer amount and increase the recipient's balance by the same amount. In other words, tokens that charge fees on transfers are not supported.- An address' ERC-20 balance can only change as a result of a
transfercall by the sender or atransferFromcall by an approved address. This excludes rebase tokens, interest-bearing tokens, and permissioned tokens where the admin can arbitrarily change balances.- The token contract does not allow callbacks (e.g., ERC-777 is not supported).
This means that Sablier Flow is expected to support ERC20 tokens with low decimals. Tokens with low decimals make this
attack vector very likely to happen as amount of calls needed to make to withdraw full amount while avoiding fees will
be low. This amount calculated with the following solidity calculation assuming the protocolFee is 10%.
uint256 calls = ((tokenAmount * (10 ** decimals) / 9)) + ((tokenAmount * (10 ** decimals)) % 9 > 0 ? 1 : 0);This calculation can also be written as the following math equation once again assuming the protocolFee is 10%, where
N is the amount of calls required, T is the amount of tokens and d is the amount of decimals.
As an example, using this calculation, we can find out that in order to withdraw 10 tokens with 0 decimals it would take
2 different calls to withdraw the full amount while avoiding fees, for 10 tokens and 2 decimals 112 different calls.
Making tokens with low decimals very susceptible to this attack as it would be profitable for malicious actors to split
their withdraw in multiple calls to avoid paying the protocolFee and pay less of that amount in gas fees.
Consider that as the protocolFee goes down, amount of tokens that can be withdraw in a single transaction goes up making
the attack cost less gas to perform. Even if no tokens with low decimals become widely used, this attack can very well be
worth to execute if gas prices keep going down and BTC (widely used token with most value for 1 wei) price goes up.
Add the following test contract in the tests file and run it to observe the vulnerability. In order to run this test,
add a mint function in the MockERC20.sol
pragma solidity >=0.8.22;
import { Test } from "forge-std/src/Test.sol";
import { console } from "forge-std/src/console.sol";
import "../src/SablierFlow.sol";
import "../src/interfaces/IFlowNFTDescriptor.sol";
import "./mocks/ERC20Mock.sol";
// Mock NFT descriptor to satisfy constructor requirements
contract MockNFTDescriptor is IFlowNFTDescriptor {
function tokenURI(IERC721Metadata, uint256) external pure override returns (string memory) {
return "test_uri";
}
}
contract CustomTest is Test {
SablierFlow private flow;
ERC20Mock private token;
ERC20Mock private wbtc;
MockNFTDescriptor private nftDescriptor;
address private sender = address(0x1);
address private recipient = address(0x2);
address private admin = address(0x3);
uint128 private amount = 100e18;
uint256 private streamId;
function setUp() public {
// Deploy mock tokens and NFT descriptor
token = new ERC20Mock("MockToken", "MTK", 0);
wbtc = new ERC20Mock("Wrapped Bitcoin", "WBTC", 8);
nftDescriptor = new MockNFTDescriptor();
// Mint initial tokens directly to the sender's address
token.mint(sender, amount);
wbtc.mint(sender, amount);
// Deploy the SablierFlow contract
flow = new SablierFlow(address(admin), nftDescriptor);
// Approve the SablierFlow contract to spend the sender's tokens
vm.startPrank(sender);
token.approve(address(flow), amount);
wbtc.approve(address(flow), amount);
vm.stopPrank();
}
function testCreateAndDepositAndWithdrawWithFee() public {
// implement the fee
UD60x18 newFee = UD60x18.wrap(0.1e18);
vm.prank(admin);
flow.setProtocolFee(token, newFee);
// caches
uint256 decimals = token.decimals();
uint256 tokenAmount = 10;
uint128 amountToDeposit = uint128(tokenAmount * (10 ** decimals));
// create a stream and deposit
vm.prank(sender);
streamId = flow.createAndDeposit(sender, recipient, ud21x18(1e18), token, true, amountToDeposit);
// advance time
vm.warp(block.timestamp + 10000);
// calculation
uint256 calls = ((tokenAmount * (10 ** decimals) / 9)) + ((tokenAmount * (10 ** decimals)) % 9 > 0 ? 1 : 0);
uint128 withdrawableAmount = flow.withdrawableAmountOf(streamId);
// recipent withdraw
vm.startPrank(recipient);
flow.withdraw(streamId, recipient, 9);
flow.withdraw(streamId, recipient, 1);
vm.stopPrank();
// assertions
assertEq(calls, 2);
assertEq(flow.protocolRevenue(token), 0);
assertEq(token.balanceOf(address(recipient)), withdrawableAmount);
}Likelihood: Low as tokens with such little decimals are not widely used at the moment
Impact: High as this vulnerability leads to direct loss of funds for the protocol
Manual review, foundry
Implement a standard base fee of 1 when protocolFee > ZERO but protocolFeeAmount returns 0. An example of this is
shown below.
function _withdraw(
uint256 streamId,
address to,
uint128 amount
)
internal
returns (uint128 withdrawnAmount, uint128 protocolFeeAmount)
{
// rest of the function
if (protocolFee > ZERO) {
// Calculate the protocol fee amount and the net withdraw amount.
(protocolFeeAmount, amount) = Helpers.calculateAmountsFromFee({ totalAmount: amount, fee: protocolFee });
if (protocolFeeAmount == 0) protocolFeeAmount = 1;
// Safe to use unchecked because addition cannot overflow.
unchecked {
// Effect: update the protocol revenue.
protocolRevenue[token] += protocolFeeAmount;
}
// rest of the function
}Alternatively, protocol already reverts when stream's token decimals are greater than 18, protocol must add another check to make sure that the token decimals can not be too little. The amount can be set according to what protocol deems is acceptable (such as 4). An example of this recommendation is shown below.
function _create(
address sender,
address recipient,
UD21x18 ratePerSecond,
IERC20 token,
bool transferable
)
internal
returns (uint256 streamId)
{
// Check: the sender is not the zero address.
if (sender == address(0)) {
revert Errors.SablierFlow_SenderZeroAddress();
}
uint8 tokenDecimals = IERC20Metadata(address(token)).decimals();
// Check: the token decimals are not greater than 18.
if (tokenDecimals > 18 || tokenDecimals < 4) {
revert Errors.SablierFlow_InvalidTokenDecimals(address(token));
}
// rest of the functionL-03. depletionTimeOf() can return 0 and a timestamp value when balance and debt relation didn't change
Submitted by neilalaois, cheatc0d33. Selected submission by: neilalaois.
The issue is somewhat similar to the issue reported in Cantina's audit report section 3.3.4* (copied below). It describes a problem that depletionTimeOf() returns 0 when it should still return the timestamp of when the totalDebt will exceed the balance by one token. But in this case I want to highlight the return difference of when balance is and is NOT 0.
Inconsistent returns from depletionTimeOf(), depending if balance is 0 or not, even though the time and rate doesn't change. This is because the function first checks if there is no balance and returns 0. But if stream has balance > 0, then it will only return 0 if the debt is above balance + 1 MVT (Minimum Value Transferable).
This creates a scenario where if the value is withdrawn at the time when stream balance = totalDebt the
depletionTimeOf() will shift from returning a timestamp slightly into the future to returning 0. This means that the
depletion isn't correctly configured of what it considers the "depletion time" if it is the timestamp of when the total
debt EXCEEDS the balance or when total debt is EQUAL to the balance.
An example of the problem is that the depletionTimeOf() returns:
- rps > 0; balance = 1; totalDebt = 1 -> output is a timestamp
- rps > 0; balance = 0; totalDebt = 0 -> output is 0
Even though in both cases the values are equal, one will provide a timestamp and will consider the stream not yet depleted, while in the other case it is considered depleted.
For a POC example using the project test case please look at "Tools Used" section.
Not completely accurate depletionTimeOf() value. Returns 0 if balance is 0, but returns timestamp if balance = totalDebt (but their return values should match).
Manual review + foundry tests
The test does this:
- Matches the totalDebt to stream balance using timestamp warp (prints them out for proof)
- Prints out the timestamp of now and the expected depletionTime which differ by 1 second
- Performs a withdrawal to recipient with the amount matching the balance. Post withdraw balance = 0.
- Prints out the timetstamps again, we see that "now" is still the same, but the depletion time is now 0. Even though the block timestamp hasn't changed and any rate parameters haven't changed either.
function test_DepletionTimeOf_balance_discrepancy() external givenNotNull givenNotPaused givenBalanceNotZero {
console.log("Rate per second : ", flow.getRatePerSecond(defaultStreamId).intoUint128());
vm.warp({ newTimestamp: block.timestamp + 47_408_000 });
console.log("total debt (6 decimals): ", flow.totalDebtOf(defaultStreamId));
console.log("balance (6 decimals) : ", flow.getBalance(defaultStreamId));
console.logString("---PRE withdraw---");
console.log("---now : ", block.timestamp);
console.log("---depletion time : ", flow.depletionTimeOf(defaultStreamId));
vm.stopPrank();
vm.prank(users.recipient);
console.logString("---Withdrawing amount = stream balance---");
flow.withdraw(defaultStreamId, users.recipient, flow.getBalance(defaultStreamId));
console.logString("---POST withdraw---");
console.log("---now : ", block.timestamp);
console.log("---depletion time : ", flow.depletionTimeOf(defaultStreamId));
}The script will print out:
Logs:
Rate per second : 1000000000000000
total debt (6 decimals): 50000000000
balance (6 decimals) : 50000000000
---PRE withdraw---
---now : 1777740800
---depletion time : 1777740801
---Withdrawing amount = stream balance---
---POST withdraw---
---now : 1777740800
---depletion time : 0This discrepancy is very minor edge case, that the function can return two different outputs at the same time without changing the rates. And that the depletion time doesn't have to be reached for it to be "depleted". Which is a slight conflict in the depletionTimeOf logic.
Probably the simplest solution is to simply remove the if(balance == 0) case and always follow the same logic path of
totalDebt has to EXCEED the balance. Otherwise alter the further down logic checks to switch to checking when the
totalDebt EQUALS the balance.
The @notice comment in ISablierFlow.sol inaccurately describes the depletionTimeOf()
/// @notice Returns the time at which the total debt exceeds stream balance. If the total debt is less than
/// or equal to stream balance, it returns 0.It should probably say that ...If the total debt is MORE than...
Cantina 3.3.4 report (for reference):
3.3.4 depletionTimeOf() returns 0 when still solvent at edge of depletion time
Severity: Low Risk
Context: SablierFlow.sol#L75
Description: The Natspec of the depletionTimeOf() states that it returns 0 when there is uncovered debt.
/// @notice Returns the time at which the stream will deplete its balance and start to accumulate uncovered , debt. If
/// there already is uncovered debt, it returns zero.;The solvencyPeriod is also calculated based on when the debt exceeds the balance by 1.
if (tokenDecimals == 18) {
solvencyAmount = (balance - snapshotDebt + 1);
} else {
uint128 scaleFactor = (10 ** (18 - tokenDecimals)).toUint128();
solvencyAmount = (balance - snapshotDebt + 1) * scaleFactor;
}
uint256 solvencyPeriod = solvencyAmount / _streams[streamId].ratePerSecond.unwrap();Therfore depletionTimeOf() should not return 0 when the totalDebt == balance, but rather the timestamp at which 1 more token will be streamed.
Recommendation: Change the depleteiontimeOf() as follows:
- if (snapshotDebt + _ongoingDebtOf(streamId) >= balance) {
+ if (snapshotDebt + _ongoingDebtOf(streamId) > balance) {Submitted by charlescheerful, x1485967, helium, inh3l, greed. Selected submission by: inh3l.
Protocol plans to support all possible ERC20 tokens, except for some criteria but will not be able to support tokens
that do not impelement the decimals function.
The protocol plans to work with any ERC20 token based on the information provided in the readme. Expect for those with the criteria highlighted below.
Any ERC-20 token can be used with Flow as long as it adheres to the following assumptions:
- The total supply of any ERC-20 token remains below
$(2^{128} - 1)$ , i.e.,type(uint128).max.- The
transferandtransferFrommethods of any ERC-20 token strictly reduce the sender's balance by the transfer amount and increase the recipient's balance by the same amount. In other words, tokens that charge fees on transfers are not supported.- An address' ERC-20 balance can only change as a result of a
transfercall by the sender or atransferFromcall by an approved address. This excludes rebase tokens, interest-bearing tokens, and permissioned tokens where the admin can arbitrarily change balances.- The token contract does not allow callbacks (e.g., ERC-777 is not supported).
However, the SablierFlow incorrectly assumes that the token to be streamed by the creator implements a decimal function.
This is because, upon stream creation, an attempt is made to get the token's
decimals
by calling the decimals function.
>>> uint8 tokenDecimals = IERC20Metadata(address(token)).decimals();
// Check: the token decimals are not greater than 18.
if (tokenDecimals > 18) {
revert Errors.SablierFlow_InvalidTokenDecimals(address(token));
}
However, not all tokens (including the standard ones) actually implement a decimals function. According to EIP-20 standard,
decimals
Returns the number of decimals the token uses - e.g. 8, means to divide the token amount by 100000000 to get its user representation.
OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
But by querying the decimal function, due to its non-existence in these tokens, the functions will fail causing that streams for these tokens cannot be created.
Flow streams for tokens like cloutContracts, DigixDAO, etc cannot be created if desired due to the absence of the decimal function breaking compatibility.
Manual review.
Recommend using a tryCatch block to query the decimals. If it fails, hardcode it to 18 for scaling.