PoolTogether - erebus's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/ObservationLib.sol#L16

Vulnerability details

Impact

The developers are using a constant named MAX_CARDINALITY as the maximum size of the _observations parameter

uint16 constant MAX_CARDINALITY = 365; // 1 year

function binarySearch(
    Observation[MAX_CARDINALITY] storage _observations,
    uint24 _newestObservationIndex,
    uint24 _oldestObservationIndex,
    uint32 _target,
    uint16 _cardinality,
    uint32 _time
)

which is used as a buffer to store one year's observations and do search through the function binarySearch. However, it only defines 365 observations (not the Solidity's year, which is 60*60*24*365 >>> 365) for the whole year, meaning that it may not be enough (for sure) to store all of the records/observations.

Proof of Concept

Suppose that the rate of observations is twice a day. That means, for the day 182/183 it will start overwriting the initial ones (ring buffer) which leads to just searching for a "time-block" of 6 months instead of the year the devs wanted. From the code:

/**
 * @dev Sets max ring buffer length in the Account.observations Observation list.
 *         As users transfer/mint/burn tickets new Observation checkpoints are recorded.
 *         The current `MAX_CARDINALITY` guarantees a one year minimum, of accurate historical lookups.
...

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the 1 year from Solidity instead of hard-coding the value (check for gas too because loading every time such a big array would make it cost A LOT, which could incur in an OOG)

Assessed type

Error