Users can manipulate observation creation

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L352-L391

Vulnerability details

Impact

Users can alter the record of their balances.

Proof of Concept

If users delegateBalance has changed, observations will be recorded. If there is a need for new observation, new observation will be created otherwise will be overwritten.

And new observation creation will be decided in the _getNextObservationIndex() function.

if (currentPeriod == 0 || currentPeriod > newestObservationPeriod) {
      return (
        uint16(RingBufferLib.wrap(_accountDetails.nextObservationIndex, MAX_CARDINALITY)),
        newestObservation,
        true
      );
    }

newestObservationPeriod is the last observations period. currentPeriod is a period that calculated with uint32 currentTime = uint32(block.timestamp).

uint32 currentPeriod = _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, currentTime);
    uint32 newestObservationPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      newestObservation.timestamp
    );

function _getTimestampPeriod(
    uint32 PERIOD_LENGTH,
    uint32 PERIOD_OFFSET,
    uint32 _timestamp
  ) private pure returns (uint32 period) {
    if (_timestamp <= PERIOD_OFFSET) {
      return 0;
    }
    // Shrink by 1 to ensure periods end on a multiple of PERIOD_LENGTH.
    // Increase by 1 to start periods at # 1.
    return ((_timestamp - PERIOD_OFFSET - 1) / PERIOD_LENGTH) + 1;
  }

The problem is, If someone deposits a small amount frequently, currentPeriod and newestObservationPeriod will always be the same and new observation won't be created. Attackers can keep doing this until closeDraw and manipulate their balances.

According to the docs : If a draw were to start and end within a period a user would be able to alter the record of their balance for that draw by overwriting an Observation.

It is important to note that due to Observation overwriting, average balances for a period are not finalized until a period ends. Therefore if a draw ends but a period has not, a user would be able to manipulate their average balance for that final period of time after the draw ends. This would result in an inaccurate record of their balance held during the draw.

Tools Used

Manual Review

Recommended Mitigation Steps

Assessed type

Other

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[Picodes]

If someone deposits a small amount frequently, currentPeriod and newestObservationPeriod will always be the same and new observation won't be created-> this only works for small deposit within the same PERIOD_LENGTH, otherwise timestamp roundings will differ.

This reports shows how by leveraging the fact that new observations are not created if the previous observations falls within the same period, an attacker could modify its average balance for the final period if a draw ends within a period.

[asselstine]

Added safe boundary checks: https://github.com/GenerationSoftware/pt-v5-twab-controller/pull/5