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

votingPeriod in GuildVetoGovernor.sol is wrong #754

Closed
c4-bot-5 opened this issue Dec 27, 2023 · 8 comments
Closed

votingPeriod in GuildVetoGovernor.sol is wrong #754

c4-bot-5 opened this issue Dec 27, 2023 · 8 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-816 edited-by-warden 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-5
Copy link
Contributor

c4-bot-5 commented Dec 27, 2023

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L231
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L36

Vulnerability details

Impact

if we see the last 1 year data , roughly a block is mined in 12 seconds while in GuildVetoGovernor 13s/block is hardcoded

Proof of Concept

The voting period is an important factor in on-chain governance systems, and considering the 1 block / 13 secs users will only be able to vote for 155hours(6 day + 13 hours), while it's expected to be 7 days period and this can be a factor in some veto proposals where a quorum is not achieved till the very end
There are multiple instances where time is measured through 13s/block

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L230

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L36

Tools Used

manual review

Recommended Mitigation Steps

The voting period should be based on last year's data should be 12 secs/block instead of 13 secs/block

Assessed type

Governance

@c4-bot-5 c4-bot-5 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 27, 2023
c4-bot-10 added a commit that referenced this issue Dec 27, 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 29, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #816

@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 24, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@Trumpero Trumpero mentioned this issue Jan 31, 2024
@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
@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-c

@viraj124
Copy link

viraj124 commented Feb 2, 2024

hey @Trumpero
From the C4 judging criteria it's a valid medium if the function of the protocol or its availability could be impacted especially when governance is involved hence it's clear that the voting period is very much affected by this issue

Also, the parent issue is labeled as grade A, while this one is grade c even after explaining the core issue
so can you please re-evaluate?

@Trumpero
Copy link

Trumpero commented Feb 7, 2024

I believe this issue is of low severity because assets are not at risk and functionalities are not impacted, it simply represents careless documentation or natspec.

Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments).

When an issue is downgraded to QA, it will be included in the QA report of the warden for QA point evaluation. Based on my QA judging of this contest, you don't have enough QA points to reach grade-b, so all your QA issues are labeled as grade-C

@Tri-pathi
Copy link

@Trumpero does it qualify for grade b now ? i.e after including above one, #826 and #514. QA report contains 6 valid issues now

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-816 edited-by-warden 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

7 participants