Strategy::checkPoolActivity() does not look as far back as it should

Summary

Strategy::checkPoolActivity() is supposed to check tick delta until the tick more in the past than lookAgo. From the security doc:

checkPoolActivity does intentionally check 1 extra observation before lookAgo

"1 extra observation" and "before" indicate that it is intended that 1 more observation in the past is looked at, with a timestamp before lookAgo.

However, the opposite actually happens, due to the way observations are being calculated.

Root Cause

In Strategy:335, it will return 1 observation too early.

Internal Pre-conditions

None.

External Pre-conditions

None.

Attack Path

1. Protocol rebalances but the twap price validation is incomplete and may lead to losses when depositing in the pool at a bad price.

Impact

Loss of funds that will be arbitraged.

PoC

The way the observations work is:

1. The current tick of the pool is taken.
2. The tick cumulative of the most recent observation is taken.
3. The tick cumulative of the second most recent observation is taken.
4. These cumulative ticks are subtracted and divided by the delta timestamp, leading to the tick at the most recent observation.
5. The current tick of the pool is compared with the tick of the most recent observation in the first iteration. In the second, it's the most recent tick with the second most recent, so on and so forth.

However, when checking lookAgo vs timestamp, it is using the timestamp of the second most recent observation, but it evaluated the tick of the most recent observation. Thus, it will return 1 tick too early.

Note: recent and second most recent can be shifted in time to 3rd and 4th and so on.

Mitigation

The timestamp check should be made on the nextTimestamp, before it is updated to timestamp in the loop.