getMaxSecondsAgo() makes an invalid assumption that the observation at index 0 is always initialized

Summary

getMaxSecondsAgo() assumes that the observation at index 0 is always initialized. This may not be true if the pool was just created. It should check initialized before using the observationTimestamp.

Vulnerability Detail

getMaxSecondsAgo() makes an incorrect assumption that the observation at index 0 is always initialized. Here is a more detailed explanation: The key parts of getMaxSecondsAgo() are:

   (uint32 observationTimestamp, , , bool initialized) = pool.observations(
     (observationIndex + 1) % observationCardinality
    );

   // ...

   if (!initialized) {
     (observationTimestamp, , , ) = pool.observations(0); 
    }

It first gets the timestamp of the observation at index (observationIndex + 1) % observationCardinality. If that observation is not initialized, it falls back to getting the timestamp at index 0, assuming it is initialized. However, this is not a valid assumption if the pool was just created. When a pool is first created, the observation at index 0 will not be initialized until the first price update. So in this case, getMaxSecondsAgo() would revert with "OLD" since it relies on the observation at index 0 being initialized when it may not be. This could cause issues any time you call getMaxSecondsAgo() on a newly created pool - it will revert unexpectedly.

Impact

It could result in reading uninitialized storage, which could return arbitrary data. This could cause getMaxSecondsAgo() to return an incorrect, invalid result. The severity depends on how getMaxSecondsAgo() is used, but reading uninitialized storage is generally considered a high severity issue. At minimum it would be a medium severity bug that could lead to unexpected behavior.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/libraries/Oracle.sol#L223-L231

Tool used

Manual Review

Recommendation

getMaxSecondsAgo() should first check if the observation at index 0 is initialized before using its timestamp. A suggestive example:

   (uint32 observationTimestamp0, , , bool initialized0) = pool.observations(0);

   if (!initialized0) {
     // No observations initialized yet
      return 0; 
    }

   // Rest of function remains the same...

   if (!initialized) {
     (observationTimestamp, , , ) = pool.observations(0);
   }

This first checks index 0, and returns 0 if it is uninitialized. Otherwise, it continues the logic as before.

sherlock-audit / 2023-10-aloe-judging

feelereth - getMaxSecondsAgo() makes an invalid assumption that the observation at index 0 is always initialized #120