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.

Sign up for GitHub

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

Jump to bottom

QA Report #81

Open
code423n4 opened this issue Mar 3, 2022 · 3 comments
Open

QA Report #81

code423n4 opened this issue Mar 3, 2022 · 3 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/skalenetwork/ima-c4-audit/blob/main/contracts/schain/CommunityLocker.sol#L209-L231

Vulnerability details

Impact

The setGasPrice() function is used to configure the mainnetGasPrice and gasPriceTimestamp on the relevant Schain. While the function would typically perform proper BLS signature verification, this has been commented out and hence any user can call this function to configure the values to any arbitrary amount.

Proof of Concept

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider performing the necessary signature verification on this function.
@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 Mar 3, 2022
code423n4 added a commit that referenced this issue Mar 3, 2022
@code423n4
leastwood issue #81
35dc022
@cstrangedk cstrangedk added duplicate This issue or pull request already exists sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Mar 4, 2022
@cstrangedk
Copy link
Collaborator

Duplicate of #21 and #80

There is no computation that currently uses mainnetGas price, if you search all contracts you will find no other reference to use this variable. This is a TODO stub, which will be finished and implemented when the Oracle is complete - at which the BLS threshold signature from the nodes will be required to be verified to set the MainnetGasPrice.

@GalloDaSballo
Copy link
Collaborator

Per the sponsor reply, and the fact that the function isn't used for computation, I will downgrade to non-critical

@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 Apr 27, 2022
@JeeberC4 JeeberC4 removed the duplicate This issue or pull request already exists label Jun 1, 2022
@JeeberC4 JeeberC4 reopened this Jun 1, 2022
@JeeberC4
Copy link

JeeberC4 commented Jun 2, 2022

Creating QA Report for warden as judge downgraded issue. Preserving original title: setGasPrice() Can be Called by Anyone

@JeeberC4 JeeberC4 changed the title setGasPrice() Can be Called by Anyone QA Report Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cstrangedk @GalloDaSballo @code423n4 @JeeberC4