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

Having no clear path to define totalLpPoolWeight and initially set totalLpPoolWeight to zero maybe problematic for the protocol #67

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/LpGauge.sol#L111
https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/InflationManager.sol#L581
https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/InflationManager.sol#L588

Vulnerability details

For totalLpPoolWeight to have some values, someone need to call InflationManager.executeLpPoolWeight. During the execution process, totalLpPoolWeight will need to go through totalLpPoolWeight = totalLpPoolWeight - currentUInts256[key] + pendingUInts256[key]; which can be problematic because the initial value of totalLpPoolWeight is zero that requires subtraction from currentUInts256[key] which I also have no information about the value but I assume that the value should not be absurdly high.

Since uint256 does not support negative value, the subtraction between these two variables would result in an underflow which will prompt error in solidity 0.8.0. However, the contract imports UnceckMath library, enable underflow possible. Therefore, the subtraction of totalLpPoolWeight - currentUInts256[key] can be restated as type(uint256).max - currentUInts256[key]. This produces an absurdly high totalLpPoolWeight which may affect currentRate of the pool causing it to likely be zero.

Still I am not sure if this is intended by the dev because I am not sure about the value of currentUInts256[key] and pendingUInts256[key]. If these two are high enough to match totalLpPoolWeight, it could bring currentRate to be in a reasonable level.

Proof Of Concepts

Pic 2 0

*totalLpPoolWeight should initially be zero.

@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
@danhper
Copy link
Collaborator

danhper commented Jun 6, 2022

totalLpPoolWeight is the sum of all currentUInts256[k], so this cannot underflow

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

Dup of #172

@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
@JeeberC4 JeeberC4 reopened this Jun 23, 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

4 participants