`isTimeSafe` and `isTimeRangeSafe` not implemented in the functions `getBalanceAt` and `getTwabBetween`

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L278-L321 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L254-L276

Vulnerability details

Impact

The Natspec of both getBalanceAt and getTwabBetween functions indicates that they should implement the functions isTimeSafe and isTimeRangeSafe (respectively) to ensure that the queried timestamps are safe, but both functions don't implement them which can lead both functions to return wrong values which in the end will impact the protocol accounting as those function are used to fetch the users balances fro a given period.

Proof of Concept

I will start by explaining what the functions isTimeSafe and isTimeRangeSafe actually do. The isTimeRangeSafe is basically a double call to the function isTimeSafe for the start and end timestamps provided to it, so to understand what both functions are doing we should look at isTimeSafe :

function _isTimeSafe(
    uint32 PERIOD_LENGTH,
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _time
) private view returns (bool) {
    // @audit 1- check when there is no observations
    // If there are no observations, it's an unsafe range
    if (_accountDetails.cardinality == 0) {
      return false;
    }
    // If there is one observation, compare it's timestamp
    uint32 period = _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, _time);

    // @audit 1- check when there is a single observation
    if (_accountDetails.cardinality == 1) {
      return
        period != _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, _observations[0].timestamp)
          ? true
          : _time >= _observations[0].timestamp;
    }
    ObservationLib.Observation memory preOrAtObservation;
    ObservationLib.Observation memory nextOrNewestObservation;

    // @audit 2- check on the newest observation
    (, nextOrNewestObservation) = getNewestObservation(_observations, _accountDetails);

    if (_time >= nextOrNewestObservation.timestamp) {
      return true;
    }

    // @audit 3- check on the period of the oberservations after & before
    (preOrAtObservation, nextOrNewestObservation) = _getSurroundingOrAtObservations(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _time
    );

    uint32 preOrAtPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      preOrAtObservation.timestamp
    );
    uint32 postPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      nextOrNewestObservation.timestamp
    );

    // The observation after it falls in a new period
    return period >= preOrAtPeriod && period < postPeriod;
}

The functions working can be resumed into performing 3 checks :

1- A check in the case there is no observations or just a single one.

2- A check on the newest observation to see if the target time is after it.

3- A check to see if the target time is in a period between the old period and the next period.

We can see in the Natspec on the getBalanceAt function that it is supposed to use isTimeSafe to ensure timestamps are safe ;

 /**
    * @notice Looks up a users balance at a specific time in the past.
    * @dev If the time is not an exact match of an observation, the balance is extrapolated using the previous observation.
    * @dev Ensure timestamps are safe using isTimeSafe or by ensuring you're querying a multiple of the observation period intervals.
    * @param _observations The circular buffer of observations
    * @param _accountDetails The account details to query with
    * @param _targetTime The time to look up the balance at
    * @return balance The balance at the target time
*/

function getBalanceAt(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _targetTime
) internal view returns (uint256) {
    // @audit not using isTimeSafe
    ObservationLib.Observation memory prevOrAtObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _targetTime
    );
    return prevOrAtObservation.balance;
 }

And also in the Natspec of the getTwabBetween function, it is said that the function isTimeRangeSafe should be used to ensure the timestamps are safe :

/**
    * @notice Looks up a users TWAB for a time range.
    * @dev If the timestamps in the range are not exact matches of observations, the balance is extrapolated using the previous observation.
    * @dev Ensure timestamps are safe using isTimeRangeSafe.
    * @param _observations The circular buffer of observations
    * @param _accountDetails The account details to query with
    * @param _startTime The start of the time range
    * @param _endTime The end of the time range
    * @return twab The TWAB for the time range
*/

function getTwabBetween(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _startTime,
    uint32 _endTime
) internal view returns (uint256) {
    // @audit did not use isTimeRangeSafe

    ObservationLib.Observation memory startObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _startTime
    );

    ObservationLib.Observation memory endObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _endTime
    );

    if (startObservation.timestamp != _startTime) {
      startObservation = _calculateTemporaryObservation(startObservation, _startTime);
    }

    if (endObservation.timestamp != _endTime) {
      endObservation = _calculateTemporaryObservation(endObservation, _endTime);
    }

    // Difference in amount / time
    return
      (endObservation.cumulativeBalance - startObservation.cumulativeBalance) /
      (_endTime - _startTime);
}

But as we can see both code snippets above both isTimeSafe and isTimeRangeSafe were not used in either of the functions.

We can also notice that both function make calls to the _getPreviousOrAtObservation function, and by going through its logic we can easily see that it contains the first two checks implemented by the function isTimeSafe but it doesn't contain the last check :

function _getPreviousOrAtObservation(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _targetTime
) private view returns (ObservationLib.Observation memory prevOrAtObservation) {
    uint32 currentTime = uint32(block.timestamp);

    uint16 oldestTwabIndex;
    uint16 newestTwabIndex;

    // @audit 1- check when there is no observations
    // If there are no observations, return a zeroed observation
    if (_accountDetails.cardinality == 0) {
      return
        ObservationLib.Observation({ cumulativeBalance: 0, balance: 0, timestamp: PERIOD_OFFSET });
    }

    // @audit 2- check on the newest observation
    // Find the newest observation and check if the target time is AFTER it
    (newestTwabIndex, prevOrAtObservation) = getNewestObservation(_observations, _accountDetails);
    if (_targetTime >= prevOrAtObservation.timestamp) {
      return prevOrAtObservation;
    }

    // @audit 1- check when there is a single observation
    // If there is only 1 actual observation, either return that observation or a zeroed observation
    if (_accountDetails.cardinality == 1) {
      if (_targetTime >= prevOrAtObservation.timestamp) {
        return prevOrAtObservation;
      } else {
        return
          ObservationLib.Observation({
            cumulativeBalance: 0,
            balance: 0,
            timestamp: PERIOD_OFFSET
          });
      }
    }

    // @audit a check on the oldest observation also implemented in `_isTimeSafe`
    // Find the oldest Observation and check if the target time is BEFORE it
    (oldestTwabIndex, prevOrAtObservation) = getOldestObservation(_observations, _accountDetails);
    if (_targetTime < prevOrAtObservation.timestamp) {
      return
        ObservationLib.Observation({ cumulativeBalance: 0, balance: 0, timestamp: PERIOD_OFFSET });
    }

    ObservationLib.Observation memory afterOrAtObservation;
    // Otherwise, we perform a binarySearch to find the observation before or at the timestamp
    (prevOrAtObservation, afterOrAtObservation) = ObservationLib.binarySearch(
      _observations,
      newestTwabIndex,
      oldestTwabIndex,
      _targetTime,
      _accountDetails.cardinality,
      currentTime
    );

    // If the afterOrAt is at, we can skip a temporary Observation computation by returning it here
    if (afterOrAtObservation.timestamp == _targetTime) {
      return afterOrAtObservation;
    }

    return prevOrAtObservation;
}

Because the function _getPreviousOrAtObservation does not contain the last check on the periods, both functions getBalanceAt and getTwabBetween can return a wrong result, and because they are used by the protocol to fetch the balance of users (or vaults), this issue can cause problems to the good working of the system.

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this issue i recommend to add a check on the previous and next periods found in _getPreviousOrAtObservation, this will ensure that the function will behave as does the function isTimeSafe and will ensure that the queried timestamps are safe in both getBalanceAt and getTwabBetween.

Assessed type

Invalid Validation

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[Picodes]

@asselstine aren't the Natspec comments here as a reminder for the dev and not to say that the functions actually implement these checks?

This reports shows that getBalanceAt and getTwabBetween don't check when there are multiple observations that these observations are coherent with the requested time (check 3 in the report). It shows that this could lead to incorrect results for anyone querying these functions, but not how the accounting of the protocol or the internal functionalities are broken.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #464

[asselstine]

@Picodes I confirmed this one as I think the TwabController shouldn't be a leaky abstraction; it puts too much onus on the user to not shoot themselves in the foot.