code-423n4 / 2024-04-panoptic-findings

9 stars 4 forks source link

wrong implement of " twapFilter" in PanopticMath.sol. #421

Open c4-bot-10 opened 5 months ago

c4-bot-10 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L266

Vulnerability details

Impact

Detailed description of the impact of this finding.

as we are taking median of twapMeasurement array ,which has a size of 20 as indices go from (0 to 19). so the 10 element is array index 9 not 10.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. function twapFilter(IUniswapV3Pool univ3pool, uint32 twapWindow) external view returns (int24) { uint32[] memory secondsAgos = new uint32;

    int256[] memory twapMeasurement = new int256[](19);

    unchecked {
        // construct the time stots
        for (uint256 i = 0; i < 20; ++i) {
            secondsAgos[i] = uint32(((i + 1) * twapWindow) / 20);
        }

        // observe the tickCumulative at the 20 pre-defined time slots
        (int56[] memory tickCumulatives, ) = univ3pool.observe(secondsAgos);

        // compute the average tick per 30s window
        for (uint256 i = 0; i < 19; ++i) {
            twapMeasurement[i] = int24(
                (tickCumulatives[i] - tickCumulatives[i + 1]) / int56(uint56(twapWindow / 20))
            );
        }

        // sort the tick measurements
        int256[] memory sortedTicks = Math.sort(twapMeasurement);

        // Get the median value
    @>>>    return int24(sortedTicks[10]);
    }
}

Tools Used

Recommended Mitigation Steps

return int24(sortedTicks[9]);

Assessed type

Context

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #239

c4-judge commented 5 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 5 months ago

Picodes marked the issue as primary issue

dyedm1 commented 5 months ago

Technically the median would be 9 (so comments are wrong), but there are not really any meaningful consequences from using the 10th index instead. Not sure the Med sev on this is justified.

Picodes commented 5 months ago

Considering the lack of impact here, I do agree with the sponsor and think low severity is more justified under "state handling, function incorrect as to spec, issues with comments".

c4-judge commented 5 months ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 5 months ago

Picodes marked the issue as grade-a