Summary

When generating the SVGs there is a logic of abbreviating the amounts deposited via SablierV2NFTDescriptor#abbreviateAmount(), which creates an abbreviated representation of the provided amount, rounded down and prefixed with ">= ", but this logic has not taken into consideration that when pricy tokens like WBTC and WETH are to be integrated, ~90% of the times most especially for the latter we can conclude that the amount that would be deposited could be as low as 0.1WBTC maybe lower, the current implementation of SablierV2NFTDescriptor#abbreviateAmount() however would finalize the SVG of this attempt as >= 1 WBTC which showcases how this would be heavily wrong as the SVG is going to include data that indicates that the amount deposited for the NFT is more than 900% what it really is, considering the main purpose of Sablier is to be a permisionless token distributor this then means that the receiver would indeed think the amount attached with the stream is acccurate as is written in the SVG and have the senders completely decieve them on acting based on false data.

Vulnerability Details

First see https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2NFTDescriptor.sol#L47-L117

Now would be key to note that the docs for this function clearly indicates that the tokenURI is expected to have the direct inlinements of the URI describing any stream NFT and then return The URI of the ERC721-compliant metadata.

This function queries the abbreviateAmount() function before when generating the SVG to know the right amount of tokens attached to the stream NFT, now see abbreviateAmount()'s implementation here: https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2NFTDescriptor.sol#L133-L168

As hinted by the |> tag, we can see that there is currently a logic applied for when "dummy" values of assets are passed, however this logic easily breaks protocol's compatibility with assets like WBTC/ WETH, see Proof of Concept below.

Proof of Concept

• A stream is to be created being attached with WBTC as an asset.

• WBTC is quite pricey, so we assume we are to attach $5000 worth of WBTC to this stream.

• As at the time of writing this report, the price of one WBTC is $66,892.01.

• The above means that the amount the user is going to pass would be 0.07475 e8 WBTC.

• e8 cause the WBTC token has 8 decimals, as can be confirmed here: https://www.contractreader.io/contract/mainnet/0x2260fac5e5542a773aa44fbcfedf7c193bc2c599

• Now when the attempt at generating the SVGs reach abbreviateAmount(), the safeAssetDecimals() is being queried so as to cache the value of the token's decimals to later on use as a divisor.

• The truncatedAmount = decimals == 0 ? amount : amount / 10 ** decimals; calculation in abbreviateAmount(), would have our 0.07475 WBTC rounded down to 0, since 0.07475e8 = 7,475e6, and since the denominator in this case is 10**8, the token has 8 decimals, i.e denominator > numerator is true , so solidity rounds down the value to 0and now the truncatedAmount = 0 .

• The above causes then causes the if (truncatedAmount < 1) check in abbreviateAmount() to be triggerred, which then erroneuosly assumes this is a dummy value, and heavily inflates the truncatedAmount to a whopping 1 WBTC which is > 1300% the original value.

The same series of scenario can be made for WETH, if we are to deposit $1000 worth of WETH considering the current price of WETH is ~ $3300, the truncatedAmount always equates to zero, due to 0.3e18 WETH / 10**18 always rounding down to 0.

Having it at the back of our mind that the following have been hinted as the actors in the protocol: https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/README.md#L63-L73

This then easily allows the Sender to game the Recipient by showing him that in our case the amount of assets attached to the stream is ~$70,000 where in real sense it's actually only $5000.

Impact

Protocol is incompatible with pricey tokens, contrary to protocol's claim of supporting any ERC 20 token in the contest's readMe.

The above statement is true, albeit users can go ahead and try integrating this asset with protocol, however this would completely break the information passed as the SVG & metadata for the stream's NFT and would easily being an avenue of not being able to validate the real amount deposited for a stream NFT by honest users.

Tools Used

Manual review

Recommendations

Reimplement the current logic of directly using the deposited amount and then directly diving it by the token's decimals when generating the SVG, that's to say if we multiply the deposited amount first by say 1e4 before dividing it by the token's decimals we can ensure that even pricey tokens would be rightly evaluated, a pseudo fix to affected snippets would be the below: https://github.com/Cyfrin/2024-05-Sablier/blob/43d7e752a68bba2a1d73d3d6466c3059079ed0c6/v2-core/src/SablierV2NFTDescriptor.sol#L138-L142

Would be key to note that this instance of querying NFTSVG.generateSVG() should now expect an inflated value and logic can also be applied to get the value back to normal.