code-423n4 / 2023-06-reserve-findings

1 stars 0 forks source link

RToken.setIssuanceThrottleParams and RToken.setRedemptionThrottleParams doesn't update lastTimestamp for the limiter #30

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L444-L459

Vulnerability details

Impact

RToken.setIssuanceThrottleParams and RToken.setRedemptionThrottleParams doesn't update lastTimestamp for the limiter. As result, next call to Throttle.useAvailable will calculate available amount incorrectly.

Proof of Concept

RToken has issuance throttle and redemption throttle, which should restrict amount that can be used by user on hourly basis. https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/libraries/Throttle.sol#L67-L75

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

As you can see, available amount depends on throttle.lastTimestamp, when available amount was calculated for the last time.

Params for the throttle can be changed by governance. In this case, calculations will use new amtRate and pctRate. But because, throttle.lastTimestamp is not updated, that means that this new values will affect older time as well, which is incorrect.

Tools Used

VsCode

Recommended Mitigation Steps

You need to update throttle.lastTimestamp when change params. Maybe call useAvailable in order to do that.

Assessed type

Error

c4-sponsor commented 1 year ago

tbrent marked the issue as disagree with severity

c4-sponsor commented 1 year ago

tbrent marked the issue as sponsor confirmed

tbrent commented 1 year ago

Agree with warden that this is an issue, but we think it is low severity since the newly set caps will be applied to restrict the throttle.

We expect to make a change to accumulate the throttle in set*ThrottleParams().

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)