Throttle::useAvailable should not update throttle.lastTimestamp if limit * delta < ONE_HOUR

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/libraries/Throttle.sol#L62
https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/libraries/Throttle.sol#L73

Vulnerability details

Description

Function Throttle::currentlyAvailable calculates the current amount available for consumption. Even though the output is right, it has a problem when it is used inside Throttle::useAvailable

    function currentlyAvailable(Throttle storage throttle, uint256 limit)
        internal
        view
        returns (uint256 available)
    {
        uint48 delta = uint48(block.timestamp) - throttle.lastTimestamp; // {seconds} // @audit It uses throttle.lastTimestamp 
        available = throttle.lastAvailable + (limit * delta) / ONE_HOUR; // @audit rounding error if limit * delta < ONE_HOUR
        if (available > limit) available = limit;
    }

Considering available = throttle.lastAvailable + (limit * delta) / ONE_HOUR;, we can see there is a possible rounding error if limit * delta < ONE_HOUR, then available = throttle.lastAvailable. This rounding error is inevitable, but we must notice that delta = uint48(block.timestamp) - throttle.lastTimestamp;.

If then we consider that, inside Throttle::useAvailable the last line is throttle.lastTimestamp = uint48(block.timestamp);, we can force this rounding, DOSing the Throttle until its amount available become 0.

Impact

DOS of Throttle.lastAvailable update if limit * delta < ONE_HOUR. We can DOS this update forever if we are allowed to sent amount = 0

POC

Let's suppose we have a Throttle with next values in Ethereum network:

• amtRate: 250
• pctRate: 1000
• lastTimestamp: 0
• lastAvailable: 250

And that supply is 280_000 * 1e18

If this was the case, the limit = (supply * params.pctRate) / FIX_ONE_256 = 280_000 * 1e18 * 1000 / 1e18 = 280

Let's suppose that 1 block has passed, then block.timestamp = 12. If we call currentlyAvailable(throttle, limit):

1. delta = block.timestamp - throttle.lastTimestamp = 12 - 0 = 12
2. available = throttle.lastAvailable + (limit * delta) / ONE_HOUR = 1000 + (280 * 12) / 3600 = 1000 + 3360 / 3600 = 1000.

Considering amount = 1, the amount available would be updated but throttle.lastTimestamp = uint48(block.timestamp) = 12 not.

If in next block we try to do the same call, the only change of state would be lastTimestamp = uint48(block.timestamp) = 24. Meaning we can DOS lastTimestamp for 250 blocks. Which is equal to 50 minutes.

Mitigation steps

Modify Throttle::currentlyAvailable and Throttle::useAvailable in order to avoid this error. In particular I would suggest next refactor:

    function currentlyAvailable(Throttle storage throttle, uint256 limit)
        internal
        view
        returns (uint256 available, bool correspondsUpdatingLastTimestamp)
    {
        uint48 delta = uint48(block.timestamp) - throttle.lastTimestamp; // {seconds}

        uint256 accruedAmount = (limit * delta) / ONE_HOUR; 
        if(accruedAmount != 0){
            available = throttle.lastAvailable
            correspondsUpdatingLastTimestamp = true;
        }else{
            available = throttle.lastAvailable + accruedAmount;
        }

        if (available > limit) available = limit;
    }

    function useAvailable(
        Throttle storage throttle,
        uint256 supply,
        int256 amount
    ) internal {
        // untestable: amtRate will always be greater > 0 due to previous validations
        if (throttle.params.amtRate == 0 && throttle.params.pctRate == 0) return;

        // Calculate hourly limit
        uint256 limit = hourlyLimit(throttle, supply); // {qRTok}

        // Calculate available amount before supply change
        (uint256 available, bool correspondsUpdatingLastTimestamp) = currentlyAvailable(throttle, limit);

        // Calculate available amount after supply change
        if (amount > 0) {
            require(uint256(amount) <= available, "supply change throttled");
            available -= uint256(amount);
            // untestable: the final else statement, amount will never be 0
        } else if (amount < 0) {
            available += uint256(-amount);
        }

        // Update cached values 
        throttle.lastAvailable = available;
        if(correspondsUpdatingLastTimestamp){
            throttle.lastTimestamp = uint48(block.timestamp);
        }
        
    }

Assessed type

Other

[0xean]

Had a little trouble following this one, but if I am understanding this correctly, it most likely should be QA due to the nature of the rounding error and this having significantly less impact as the numbers scale.

For example, the pctRate provided isn't in the expected range of values per the docs

Default value: `2.5e16` = 2.5% per hour
Mainnet reasonable range: 1e15 to 1e18 (0.1% per hour to 100% per hour)

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[tbrent]

Mitigated in reserve-protocol/protocol#857

[c4-judge]

0xean changed the severity to QA (Quality Assurance)