Invalid Draw Range Vulnerability in getDisbursedBetween, the subsequent check for `_endDrawId < drawIds.first`.

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L173 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L166-L307

Vulnerability details

Impact

An attacker can bypass the check for a valid draw range and potentially manipulate the calculation of the disbursed balance, by providing an invalid draw range, an attacker could trick the contract into returning an incorrect disbursed balance or cause the contract to revert unexpectedly.

Proof of Concept

Error InvalidDrawRange is emitted in the getDisbursedBetween function of the DrawAccumulatorLib library when a draw range is not strictly increasing,the specific line of code where the error is emitted:

DrawAccumulatorLib.sol#L173

revert InvalidDrawRange(_startDrawId, _endDrawId);

This code line is triggered when the _startDrawId provided as input is greater than the _endDrawId, indicating that the draw range is not in ascending order. The InvalidDrawRange error is emitted to indicate this invalid condition.

In the getDisbursedBetween function, the vulnerability allows an attacker to retrieve the disbursed balance between a start and end draw ID, even if the end draw ID is not within the expected range. This can lead to incorrect disbursed balance calculations and potential exploitability.

Affected Code Block: DrawAccumulatorLib.sol#L166-L

function getDisbursedBetween(
    Accumulator storage _accumulator,
    uint16 _startDrawId,
    uint16 _endDrawId,
    SD59x18 _alpha
) internal view returns (uint256) {
    // ...

    // This check intentionally limits the `_endDrawId` to be no more than one draw before the latest observation.
    // This allows us to make assumptions on the value of `lastObservationDrawIdOccurringAtOrBeforeEnd`
    // and removes the need for an additional binary search to find it.
    if (_endDrawId < drawIds.second - 1) {
        revert InvalidDisbursedEndDrawId(_endDrawId);
    }

    // ...

    // if a "body" exists
    if (
        firstObservationDrawIdOccurringAtOrAfterStart > 0 &&
        firstObservationDrawIdOccurringAtOrAfterStart < lastObservationDrawIdOccurringAtOrBeforeEnd
    ) {
        Observation memory atOrAfterStart = _accumulator.observations[firstObservationDrawIdOccurringAtOrAfterStart];
        Observation memory atOrBeforeEnd = _accumulator.observations[lastObservationDrawIdOccurringAtOrBeforeEnd];
        uint amount = atOrBeforeEnd.disbursed - atOrAfterStart.disbursed;
        total += amount;
    }

    // ...

    return total;
}

The issue is with the validation of the _endDrawId parameter. The function expects the _endDrawId to be no more than one draw before the latest observation. However, this check is incorrect and allows an attacker to supply an arbitrary _endDrawId value that can result in incorrect disbursed balance calculations.

References:

• Solidity Documentation: https://solidity.readthedocs.io/
• OpenZeppelin Contracts: https://github.com/OpenZeppelin/openzeppelin-contracts

To illustrate the scenario, let's assume the library has been used to accumulate balances for draws 1 to 10, and the last observed draw is draw 10. Now, if someone tries to retrieve the disbursed balance between draw 1 and draw 5, it should work fine because the end draw ID (5) is within the observed range. However, if they try to retrieve the disbursed balance between draw 1 and draw 0 or any draw ID less than the last observed draw ID (10), the error "InvalidDisbursedEndDrawId" will be emitted.

The purpose of this error is to prevent historical disbursement queries that end more than one draw before the last observed draw. It ensures that the requested end draw ID is within the valid range of observed draws, avoiding incorrect or inconsistent results.

In summary, the "InvalidDisbursedEndDrawId" error is emitted when the end draw ID provided to the getDisbursedBetween function is invalid because it is too old and falls outside the range of observed draws.

Tools Used

Manual Review

Recommended Mitigation Steps

The code should be updated to validate that the _endDrawId is within the expected range of valid draw IDs. Here's an updated version of the affected code block:

function getDisbursedBetween(
    Accumulator storage _accumulator,
    uint16 _startDrawId,
    uint16 _endDrawId,
    SD59x18 _alpha
) internal view returns (uint256) {
    // ...

    // Add validation to ensure _endDrawId is within the valid range
    if (_endDrawId < _startDrawId) {
+       revert InvalidDrawRange(_startDrawId, _endDrawId);
    }

    // ...

    // if a "body" exists
    if (
        firstObservationDrawIdOccurringAtOrAfterStart > 0 &&
        firstObservationDrawIdOccurringAtOrAfterStart <= lastObservationDrawIdOccurringAtOrBeforeEnd
    ) {
        Observation memory atOrAfterStart = _accumulator.observations[firstObservationDrawIdOccurringAtOrAfterStart];
        Observation memory atOrBeforeEnd = _accumulator.observations[lastObservationDrawIdOccurringAtOrBeforeEnd];
        uint amount = atOrBeforeEnd.disbursed - atOrAfterStart.disbursed;
        total += amount;
    }

    // ...

    return total;
}

With this fix, the code now properly validates the range of draw IDs and prevents incorrect disbursed balance calculations.

Assessed type

Invalid Validation

[Picodes]

I don't understand. The check is here.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid