sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
307 stars 43 forks source link

Change check operator in `SablierV2LockupLinear._calculateStreamedAmount()` #958

Closed andreivladbrg closed 3 months ago

andreivladbrg commented 3 months ago

From Egis audit:

Context: SablierV2LockupLinear.sol

Description: If cliffTime == blockTimestamp or startTime == blockTimestamp, the calculation for streamedAmountwill be executed, which in the end will return 0. This is unnecessary as the function will just waste more gas to return the same value in the end.

Recommendation: Make the two checks >= instead of > like they are done in SablierV2LockupDynamic


IMO, we should use >= only for start time and not for cliff time because when current time equals cliff time, the calculation function returns a value > 0, @smol-ninja wdyt?

if (cliffTime > blockTimestamp || startTime >= blockTimestamp) {
     return 0;
}
smol-ninja commented 3 months ago

when current time equals cliff time, the calculation function returns a value > 0

I am curious as why it should return non-zero value when cliffTime equals blockTimestamp.

andreivladbrg commented 3 months ago

am curious as why it should return non-zero value when cliffTime equals blockTimestamp

it is not about wether it should or not

it is about that the function calculates a value > 0

the finding says: "the calculation for streamedAmountwill be executed, which in the end will return 0. This is unnecessary as the function will just waste more gas to return the same value in the end."


for example, if a stream is created with a deposit amount of 10,000 assets and for a duration of 10,000 seconds, and the cliff time is set exactly at 25% of the stream. with the operator >= for cliff time, when 2500 seconds pass from the beginning of the stream, i.e. 25%: startTime < blockTimestamp == cliffTime the calculateStreamedAmount will return 0 instead of 2500 assets. and ultimately the "cliff amount" is 2501. the UI would need to pass a cliff time of 25% -1 second to achieve the current flow

smol-ninja commented 3 months ago

Good observation and agree with that you said.