Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throttle rate is applied incorrectly. #89

Open
howlbot-integration bot opened this issue Aug 20, 2024 · 7 comments
Open

Throttle rate is applied incorrectly. #89

howlbot-integration bot opened this issue Aug 20, 2024 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_52_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-07-reserve/tree/main/contracts/p1/RToken.sol#L356-L359
https://github.com/code-423n4/2024-07-reserve/tree/main/contracts/p1/RToken.sol#L370-L375
https://github.com/code-423n4/2024-07-reserve/tree/main/contracts/p1/RToken.sol#L387-L391

Vulnerability details

Impact

Throttle rate will be applied incorrectly.
For instance, the RToken can be issued more than issuance throttle settings or can be redeemed less than redemption throttle settings.

Proof of Concept

RToken updates the available amount and last update time of throttle limit for issueance and redemption in issueTo, redeemTo and redeemCustom functions but not update them in mint, melt and dissolve functions.
For instance, mint function doesn't issue tokens to users but increase the totalSupply of RToken.
Since the calculation of available amount depends on totalSupply, this causes the incorrect applying of throttle rate.

Vulnerability Detail

RToken update and check the throttle in the issueTo function as follows.

    function issueTo(address recipient, uint256 amount) public notIssuancePausedOrFrozen {
        --- SKIP ---
        uint256 supply = totalSupply();

        // Revert if issuance exceeds either supply throttle
        issuanceThrottle.useAvailable(supply, int256(amount)); // reverts on over-issuance
        redemptionThrottle.useAvailable(supply, -int256(amount)); // shouldn't revert
        --- SKIP ---
    }

Then the totalSupply is used in Throttle.sol#useAvailable function as follows.

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

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

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

        // Update throttle.timestamp if available amount changed or at limit
        if (available != throttle.lastAvailable || available == limit) {
            throttle.lastTimestamp = uint48(block.timestamp);
        }

        // Update throttle.lastAvailable
        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);
        }
        throttle.lastAvailable = available;
    }

totalSupply is used to calculate limit in L46 and then limit is used to calculate available in L49.
currentlyAvailable function of L49 is the following.

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

As a result, we can see that the increasement of available amount depends on totalSupply and the duration of it (L74-L75).
However, mint function increase the totalSupply but doesn't update the issuanceThrottle.lastTimestamp.

From this, the following scenario is available.

  1. Assume that issuanceThrottle.params.amtRate is zero or small, so we can ignore it.
    And assume that issuanceThrottle.params.pctRate = 10%.
  2. Assume that totalSupply of RToken is 1000 at t = 0.
    Then the hourly limit of issuance is 1000 * 10% = 100.
  3. There is no issuance of RToken for an hour.
  4. At t = 3599, 1000 of RToken is minted by backingManager. Then totalSupply is updated to 2000 now.
  5. At t = 3600, A user can calls issueTo to issue RToken with amount = 200.
  6. Since totalSupply = 1000 from t = 0 to t = 3599 and totalSupply = 2000 from t = 3599 to t = 3600, the hourly limit of issuance should be (1000 * 10% * 3599 + 2000 * 10% * 1) / 3600 = 100.
    However, the user issued 200 tokens in 1 hour which exceeds 100.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify RToken.sol#mint function as follows.

    function mint(uint192 baskets) external {
        require(_msgSender() == address(backingManager), "not backing manager");
        _scaleUp(address(backingManager), baskets, totalSupply());
++      issuanceThrottle.useAvailable(totalSupply(), 0);
++      redemptionThrottle.useAvailable(totalSupply(), 0);
    }

Assessed type

Math

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_52_group AI based duplicate group recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Aug 20, 2024
howlbot-integration bot added a commit that referenced this issue Aug 20, 2024
@tbrent tbrent added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Aug 21, 2024
@tbrent
Copy link

tbrent commented Aug 21, 2024

Confirming the issue, but think severity is not HIGH. No funds are at risk.

@akshatmittal akshatmittal added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Aug 26, 2024
@akshatmittal
Copy link

Going back at this.

The throttle is supposed to apply to external interactions not internal ones, and the throttle works correctly in all of those cases.

You can obviously see people making arguments in both directions on when should and should not the throttle apply, which bring it down to the goals of the throttle in the first place.

@thereksfour
Copy link

Agree with sponsor, using throttle for internal operations is likely to result in incorrect workflow and introduce new issues. It is fine for throttle to be used to just restrict external operations.

@c4-judge
Copy link
Contributor

thereksfour marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Aug 28, 2024
@c4-judge
Copy link
Contributor

thereksfour marked the issue as unsatisfactory:
Invalid

@c4-judge
Copy link
Contributor

c4-judge commented Sep 4, 2024

thereksfour changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 4, 2024
@c4-judge c4-judge reopened this Sep 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Sep 4, 2024

thereksfour marked the issue as grade-b

@c4-judge c4-judge added grade-b and removed unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Sep 4, 2024
@C4-Staff C4-Staff added the Q-07 label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_52_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants