_updateTwav() and _getTwav() will revert when cumulativePrice overflows

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Twav/Twav.sol#L28
https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Twav/Twav.sol#L40

Vulnerability details

Impact

Contract will break when cumulativeValuation overflows.

PoC

Cumulative prices are designed to work with overflows/underflows because in the end the difference is important.

In _updateTwav() when _prevCumulativeValuation + (_valuation *_timeElapsed) overflows the contract will not work anymore.

twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative @audit overflow breaks the contract

Same problem in _getTwav()

  _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);@audit same overflow breaks the contract

}

Similar issues

code-423n4/2022-04-phuture-findings#62

Recommended

Add unchecked keyword in every line you add / subtract cumulative prices.

[HardlyDifficult]

Without a better POC of the issue occurring it's hard to justify this is High risk. e.g. maybe it could be forced by spamming updateTWAV, but it's not clear if that would require extremely large values or an unrealistic number of transactions.

Related to #178, that one includes unchecking the price in the recommendation but the rest of the description focuses on timestamp overflows while this one looks at price overflows.