Reserve does not correctly implement RingBuffer

[code423n4]

Handle

cmichel

Vulnerability details

The Reserve does not correctly use ring buffers to get the oldest / newest elements if the array is full (observations larger than cardinality) in which case it should wrap around.

getReserveAccumulatedBetween always picks reserveAccumulators[_cardinality - 1]. _checkpoint tries to write to reserveAccumulators[cardinality++] which will break once cardinality reaches MAX_CARDINALITY.

The TwabLib library has a correct oldestTwab/newestTwab implementation using the RingBufferLib that wraps around if needed.

Impact

Anyone can send 1 wei to the reserve and call checkpoint on it until the MAX_CARDINALITY is reached. Afterwards, trying to write any new checkpoints will fail as _checkpoint now tries to write to cardinality=MAX_CARDINALITY+1 which is out of bounds of the reserveAccumulators.

The reserve is broken and cannot withdraw funds anymore. The gas costs for such an attack are very high and would take ~7 years if writing every block, making it probably not worth fixing.

Recommended Mitigation Steps

Correctly implement the ring buffer usage like in TwabLib.

[PierrickGT]

PR: https://github.com/pooltogether/v4-core/pull/217

[GalloDaSballo]

The warden identified a griefing exploit that leverages ring buffers, the sponsor has mitigated by using a library that will wrap around instead of going out of bounds