Summary

The getTwap function will loop through previous rounds and compute cumulative sum until a round at least lookback seconds ago is reached. But the roundId in Chainlink Aggregator is NOT incremental and Not all roundIds are valid. As a result, the function getTwap function may not be able to work properly.

Vulnerability Details

As per the doc of chainlink(https://docs.chain.link/data-feeds/historical-data), the roundId is computed by phaseId and aggregatorRoundId:

phaseId is incremented each time the underlying aggregator implementation is updated. It is used as a key to find the aggregator address. aggregatorRoundId is the aggregator roundId. The id starts at 1. The computed roundId is not incremental and not all roundIds are valid.
For example:

1. phaseId is 1 and the last aggregatorRoundId is 199, the computed roundId is 18446744073709551815

2. phaseId is 2 and the first aggregatorRoundId is 1, the computed roundId is 36893488147419103233

All roundId between 18446744073709551815 and 36893488147419103233 are invalid.

Currently, the function getTwap reduces roundId by 1 in each loop:

It is possible that the new roundId is invalid. If the roundId is invalid, the getTwap will return 0 to indicate that there is a failure.

Impact

The getTwap function will not work properly due to using wrong roundId.

Tools Used

Manual Review

Recommendations

Consider reducing the phaseId and aggregatorRoundId in the loop and using these two variable to calculate the roundId.