TWA value might be incorrect because of wrong update in first lookback period

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Twa.sol#L21

Vulnerability details

Impact

In function getTwa(), it calculates the time-weighted average price by taking average sum between last twa value and new price value

function getTwa(IPool.TwaState storage self) internal view returns (int256) {
    if (block.timestamp == self.lastTimestamp) {
        return self.twa;
    }

    int256 _timeDiff = int256(block.timestamp - self.lastTimestamp) * PRBMathSD59x18.SCALE;
    int256 _lookback = int256(int16(self.lookback)) * PRBMathSD59x18.SCALE;

    if (_timeDiff >= _lookback) {
        return self.value;
    }
    // @audit last price did not last for (_lookback - _timeDiff) for first lookback period
    return PRBMath.mulDivSigned(self.twa, _lookback - _timeDiff, _lookback) + PRBMath.mulDivSigned(self.value, _timeDiff, _lookback);
}

Even in case last price did not last for _lookback - _timeDiff for first lookback period, it still assumes that price last for that long, it will result in slight wrong value calculation in getTwa(). For example, since first price will have a huge proportion when calculating time-weighted price, for first lookback period, price cannot change too much. In addition, this lookback period is at least one hour and can be longer, TWA is not correct and it will still have slight effect after first period.

Proof of Concept

Consider the scenario with lookback = 3600

1. First price value value = 100 and updated at t = 10. So twa = 100
2. New price is updated at t = 15 with price = 200, the new twa value is

   timeDiff = 5
   twa = (100 * 3595 + 200 * 5) / 3600 = 100.13

3. New price is updated at t = 20 with price = 205, the new twa value is

   timeDiff = 5
   twa = (100.13 * 3595 + 205 * 5) / 3600 = 100.27

   While the correct value should be

   (100 * 5 + 200 * 5) / 10 = 150

   Because price = 100 has last 5 seconds from t = 10 -> 15 and price = 200 has last 5 seconds from t = 15 -> 20.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider updating how twa price is calculated correctly

[gte620v]

Disagree. If we just define the TWA such that the price at the first time instant is also the price for the lookback period before the pool was started, then the TWA is valid.

[c4-sponsor]

gte620v marked the issue as sponsor disputed

[kirk-baird]

I consider this issue to be a Low severity issue.

I appreciate the wardens sentiment on whether the time before the start should be included in the TWA and attributed to the initial price or if it should be ignored. However, both designs are valid.

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-b

[kirk-baird]

Adding #81 as a duplicate I consider this to be a grade-a QA report as there are two unique issues not included in other reports which are of Low severity.

[c4-judge]

kirk-baird marked the issue as grade-a