`Pair.sol#_update()` will revert when `reserve0CumulativeLast` or `reserve1CumulativeLast` gets large enough

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Pair.sol#L207-L210

Vulnerability details

if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
    reserve0CumulativeLast += _reserve0 * timeElapsed;
    reserve1CumulativeLast += _reserve1 * timeElapsed;
}

As the solidity version used in the current implementation of Pair.sol is 0.8.13, and there are some breaking changes in Solidity v0.8.0, including:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics

When updating reserve0CumulativeLast and reserve1CumulativeLast in Pair.sol, overflow and underflow are desired as per the comment.

However, the intended overflow only works for solidity < 0.8.0 by default. If overflow and underflow are desired, then the math should be put into an unchecked block. Otherwise, the transaction will revert.

Impact

Since the overflow is desired in the original version, and it's broken because of using Solidity version >0.8. The Pair contract will break when the desired overflow happens.

Recommendation

Change to:

unchecked {
    if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
        reserve0CumulativeLast += _reserve0 * timeElapsed;
        reserve1CumulativeLast += _reserve1 * timeElapsed;
    }
}

[GalloDaSballo]

The warden has identified a bug that slipped through multiple audits, the cumulative checks are indeed meant to overflow to allow for continuous trades and add observations indefinitely.

That said, I went and checked some pairs from solidly and then simmed it, it seems like even in the most rosy of predictions:

1 Billion Tokens with 18 decimals, with a Million Trades per Second, over 1 thousand years will still be 30 digits away from overflowing

>>> 10e9 * 1e18 *  1e6 * seconds_per_week * 52 * 1000 
3.1449599999999998e+44

I believe the finding is totally valid and the unchecked block should have been there.

That said there seems to be little to no odds that these values will ever overflow

[GalloDaSballo]

After some consideration, because it will take over 1 thousand years for this to even remotely matter, I believe the finding to be valid, but the severity to be QA

[GalloDaSballo]

Low Severity