Accumulation of error in method averagePriceForPeriod

[code423n4]

Handle

a_delamo

Vulnerability details

Impact

In order to calculate the average price during the last 24h, we use the method averagePriceForPeriod from the library libPrice. To calculate this, is doing the average of each hour and then the average of all the previous results. But for doing these operations, we are not using the fixed-point library (PRBMathSD59x18.sol), so with each operation, we are accumulating errors and it can affect the final result.

function averagePrice(PriceInstant memory price)
        internal
        pure
        returns (uint256)
    {
        // todo double check safety of this.
        // average price == 0 is not neccesarily the
        // same as no trades in average
        if (price.trades == 0) {
            return 0;
        }
        return price.cumulativePrice / price.trades;
    }

    /**
     * @notice Calculates average price over a time period of 24 hours
     * @dev Ignores hours where the number of trades is zero
     * @param prices Array of PriceInstant instances in the 24 hour period
     * @return Average price in the time period (non-weighted)
     */
    function averagePriceForPeriod(PriceInstant[24] memory prices)
        internal
        pure
        returns (uint256)
    {
        uint256[] memory averagePrices = new uint256[](24);

        uint256 j = 0;
        for (uint256 i = 0; i < 24; i++) {
            PriceInstant memory currPrice = prices[i];

            // don't include periods that have no trades
            if (currPrice.trades == 0) {
                continue;
            } else {
                averagePrices[j] = averagePrice(currPrice);
                j++;
            }
        }

        return LibMath.meanN(averagePrices, j);
    }

/**
     * @notice Get sum of an (unsigned) array, for the first n elements
     * @param arr Array to get the sum of
     * @param n The number of (first) elements you want to sum up
     * @return Sum of first n elements
     */
    function sumN(uint256[] memory arr, uint256 n) internal pure returns (uint256) {
        uint256 total = 0;

        for (uint256 i = 0; i < n; i++) {
            total += arr[i];
        }

        return total;
    }

    /**
     * @notice Get the mean of an (unsigned) array
     * @param arr Array of uint256's
     * @return The mean of the array's elements
     */
    function mean(uint256[] memory arr) internal pure returns (uint256) {
        uint256 n = arr.length;

        return sum(arr) / n;
    }

    /**
     * @notice Get the mean of the first n elements of an (unsigned) array
     * @dev Used for zero-initialised arrays where you only want to calculate
     *      the mean of the first n (populated) elements; rest are 0
     * @param arr Array to get the mean of
     * @param len Divisor/number of elements to get the mean of
     * @return Average of first n elements
     */
    function meanN(uint256[] memory arr, uint256 len) internal pure returns (uint256) {
        return sumN(arr, len) / len;
    }

[raymogg]

In this specific case, the use of WAD maths is actually not wanted. This is due to the fact that trades is not a WAD value itself, so we actually want the divide to reduce decimal places if needed to compute the true average.

If we used the WAD maths library, the output would not be as expected due to this. We could convert the number of trades used to compute the average into a WAD, however the output would be the same as using built in division.

[cemozerr]

Marking this issue as invalid as @raymogg's explanation makes sense.