Summary

HelperscheckAndCalculateBrokerFee is designed to verify that the broker fee does not exceed a specified maximum and to calculate the broker and deposit amounts. However, protocol current have an assumption that casting to uint128 is safe which is wrong, as the current implementation would lead to overflow issues as proven in this report, showcasing how the attempt at creating streams for honest and valid users to be bricked.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/libraries/Helpers.sol#L80-L108

This function is used to check the broker fee is not greater than maxBrokerFee, and then also calculates the broker fee amount and the deposit amount from the total amount, problem here is that protocol wrongly assumes the unsafe casting to uint128 is safe since the maximum feee is hardcoded, this though line is actually wrong and could cause for protocol to be incompatible with some to be integrated assets, and this could even occur for asset that might be currently integrated but this attempt at calculating the fee would always revert due to an overflow.

Keep in mind that this helper function is actually heavily used across protocol, as shown by this search command: https://github.com/search?q=repo%3ACyfrin%2F2024-05-Sablier+checkAndCalculateBrokerFee+NOT+language%3AMarkdown+NOT+language%3AShell&type=code. this funciton is always queried whenever attempting to create streams via SablierV2LockupTranched#L233 SablierV2LockupLinear#L240 & SablierV2LockupDynamic#L319.

To prove how this could indeed overflow let's go through what sort of assets are supported in the protocol, from the contest's readMe the below conclusion can be made:
The protocol supports any ERC20 providing the token is not rebasing and it's total supply is assured to always be below type(uint128).max. which is 340282366920938463463374607431768211455 and that it has the normal transfer logic, i.e the amount specified in the transfer is what is being transferred.

Since for a token to be supported it's total supply must be below type(uint128).max 340282366920938463463374607431768211455.

Proof of Concept

Consider the next scenarios:

• ERC20 ABC token is to be integrated

• Let's divide the current accepted max value by 3.40282366920938463463374607431768211455 million , which makes the total supply less than 3 million times (_exponentially) the current accepted max and equal to 100000000000000000000000000000000. 1e32

• Now let's assume the current broker fee applied is 10% of the accepted max, i.e brokerFee = 0.01e18.

• Since it's an ERC20, we can of course conclude that the price of this token is going to be quite minute since it's total supply is quite large (this is very common in the crypto world with tokens like PEPE and other examples),

• To make this more realistic, let's even assume only 10% of the token's current total supply is in circulation and the remaining 90% are vested to be released in the next ten years, this makes our circulating total supply 1e32 - 1 = 1e31 and the current Market cap is 10 million dollars, meaning it takes

• Of course we can't have all the tokens to one person for a normal popular token, but for simplicity sake, lets assume that holders of this token are only 10,000 and all users are equal and own the same amount of tokens, this makes each user have a holding $1000 worth of tokens 1e27 tokens.

• Now, due to gas costs, obviously no user would try passing in the total amount when creating the streams for as low as $10, so let's assume a token holder wants to deposit $100 worth of their tokens, i.e 1e26 tokens in order to create the stream.

• When calculating the fee, before the casting to uint128 we have the calculation: (totalAmount).mul(brokerFee), in our case it's going to be 1e26 * 0.01e18:

[
1e26 \times 0.01e18 = 10^{42}
]

• But the maximum value of uint128 as previously hinted is way less than this ~3.4e39, which proves the overflow.

Impact

As hinted in the Proof of Concept we reduced the currently accepted max by over 3 million divisibly and still showcase how the overflow is going to occur when users are attempting to create streams for their assets.

Would be key to note that the more the total supply the easier this overflows and bricks the attempt at creating streams.

Considering the HelperscheckAndCalculateBrokerFee funciton is always queried whenever attempting to create streams via SablierV2LockupTranched#L233 SablierV2LockupLinear#L240 & SablierV2LockupDynamic#L319, any error would lead to a break in the whole attempt of creating the stream.

Tools Used

Manual review

Recommendations

Change the way the broker fee is being calculated when creating streams as in very real instances as shown in report, this attempt overflows.