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

Minus before addition -> underflow risk (But reverted due to solidity 0.8) #172

Open
code423n4 opened this issue Jun 3, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/InflationManager.sol#L574

Vulnerability details

Impact

totalKeeperPoolWeight = totalKeeperPoolWeight - currentUInts256[key] + pendingUInts256[key];

If currentUInts256[key] > totalKeeperPoolWeight it is clearly reverted due to underflow.

Proof of Concept

totalKeeperPoolWeight = totalKeeperPoolWeight - currentUInts256[key] + pendingUInts256[key];

Obviously minus currentUInts256[key] before addition pendingUInts256[key]. If currentUInts256[key] > totalKeeperPoolWeight it is clearly reverted due to underflow.

But if you plus before minus, it never get underflow even if currentUInts256[key] > totalKeeperPoolWeight

But still underflow if currentUInts256[key] > totalKeeperPoolWeight + pendingUInts256[key]

Tools Used

Scan code by eye

Recommended Mitigation Steps

totalKeeperPoolWeight = totalKeeperPoolWeight + pendingUInts256[key] - currentUInts256[key];

pendingUInts256[key] will be added to totalKeeperPoolWeight first, greater value resist underflow error.

@code423n4 code423n4 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 Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@chase-manning
Copy link
Collaborator

totalKeeperPoolWeight will always be greater than currentUInts256[key]

@chase-manning chase-manning added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

I believe the finding to have validity, however, in lack of a POC, with the sponsor disputing, I can't help but downgrade to QA.

Please provide a POC showing how the misconfiguration can actually happen to ensure your findings can withstand basic scrutiny

@GalloDaSballo GalloDaSballo added 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 Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants