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

Potential infinite loop #172

Closed
c4-bot-7 opened this issue Dec 19, 2023 · 5 comments
Closed

Potential infinite loop #172

c4-bot-7 opened this issue Dec 19, 2023 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-152 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-7
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L532-L534

Vulnerability details

Impact

The loop contains a misplaced increment operation i++, which could lead to an infinite loop if userGaugeWeight is zero, as i would never be incremented.

Proof of Concept

The related code snippet is part of a loop that iterates over a list of gauges to calculate weights and perform certain updates. However, there is a potential issue with the position of the i++ increment operation within the loop. It is placed inside an unchecked block and within a conditional statement that checks if userGaugeWeight is not equal to zero. If userGaugeWeight is zero for any gauge, the increment will be skipped, potentially causing an infinite loop as the index i will not be updated.

        for (
            uint256 i = 0;
            i < size && (userFreeWeight + userFreed) < weight;

        ) {
            address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }

                unchecked {
                    ++i;
                }
            }
        }

Tools Used

Recommended Mitigation Steps

        for (
            uint256 i = 0;
            i < size && (userFreeWeight + userFreed) < weight;

        ) {
            address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }

-               unchecked {
-                   ++i;
-               }
            }
+           unchecked {
+               ++i;
+           }

        }

Assessed type

Loop

@c4-bot-7 c4-bot-7 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 19, 2023
c4-bot-7 added a commit that referenced this issue Dec 19, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Dec 30, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #152

@c4-judge
Copy link
Contributor

Trumpero 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 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 28, 2024
@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed grade-b labels Jan 31, 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 duplicate-152 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants