Strategy::checkPoolActivity() incorrect check leads to vulnerable price

Summary

Strategy::checkPoolActivity() reverts if the timestamp is 0. However, this will never happen. What actually happens is the timestamp being 1, when the cardinality has been increased and the tick timestamp was set to 1 for gas saving purposes.

When this tick is hit in the loop, it means there were not enough observations to reach the lookAgo timestamp and it should revert because the price could not be validated. However, if the timestamp is 1, it may not revert depending on the value of the tick delta calculated, and as the timestamp is 1, it will return true with an invalid price observation.

Root Cause

In Strategy:322, the timestamp check is incorrect.

Internal Pre-conditions

None.

External Pre-conditions

None.

Attack Path

1. Twap would not have enough credibility and rebalancing would be impossible.
2. Attacker or user adds cardinality to the uniswap pool, which sets the next ticks timestamp to 1.
3. Rebalance goes through with an invalid price with not enough validation, leading to depositing liquidity at a bad price and causing losses for users.

Impact

Loss of funds due to depositing liquidity at an unfavourable price.

PoC

The following function is used when cardinality is increased in the pool.

    function grow(
        Observation[65535] storage self,
        uint16 current,
        uint16 next
    ) internal returns (uint16) {
        require(current > 0, 'I');
        // no-op if the passed next value isn't greater than the current next value
        if (next <= current) return current;
        // store in each slot to prevent fresh SSTOREs in swaps
        // this data will not be used because the initialized boolean is still false
        for (uint16 i = current; i < next; i++) self[i].blockTimestamp = 1;
        return next;
    }

Mitigation

            if (timestamp == 1) {
                revert("timestamp 1");
            }