sherlock-audit / 2022-11-nounsdao-judging

4 stars 0 forks source link

keccak123 - Uneven ratePerSecond from precision loss #51

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

keccak123

medium

Uneven ratePerSecond from precision loss

Summary

Sablier notes that the amount of tokens for the stream must be divisible by the time delta with no remainder. This is not necessary with Nouns streamer but the stream will not have an even flow over time. There will be an amount that is only available at the end of the stream. The amount only available at the end increases with the duration of the stream, which can reduce the overall value received by the receiver due to inflation (or lost yield) over time.

Vulnerability Detail

Sablier only allows a token amount for the stream that is divisible by the stream duration. This is not true of Nouns streamer. If we assume the maximum precision loss in ratePerSecond is 0.99 / 1e6, if the stream lasts for 1e12 seconds, a value of 1e6 tokens may be received only at the end of the stream. This may be a small amount of value for USDC (with decimals of 6, for 1e6 = $1). But if other tokens are added to this protocol in the future, a token such as WBTC with decimals of 8 and a peak price of over $60,000, 1e4 tokens could equate to 0.01 * 60000 = $600. Because the amount locked is proportional to the duration of the stream, the impact compounds to worse scenarios over time, because a greater wait period = greater tokens locked = greater loss of value from inflation.

Impact

Receiver loses value because some amount of tokens are locked until the end of the stream due to precision loss.

Code Snippet

There are comments acknowledging the precision loss but no protection for this scenario is implemented https://github.com/sherlock-audit/2022-11-nounsdao/blob/main/src/Stream.sol#L129-L136

Tool used

Manual Review

Recommendation

Add the same require(deposit % vars.duration == 0); code found in Sablier.

davidbrai commented 1 year ago

Not requiring the deposit to be mod 0 of the duration is a feature not a bug. We think the requirement sablier has makes it bad UX for stream creators.

"if the stream lasts for 1e12 seconds" -> 31,688 years, not something reasonable