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

Improperly Skewed Governance Mechanism #232

Open
code423n4 opened this issue May 24, 2022 · 2 comments
Open

Improperly Skewed Governance Mechanism #232

code423n4 opened this issue May 24, 2022 · 2 comments
Labels
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) 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-aura/blob/main/contracts/AuraLocker.sol#L594-L609
https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraLocker.sol#L611-L618

Vulnerability details

ALR-02H: Improperly Skewed Governance Mechanism

File Lines Type
AuraLocker.sol L594-L609, L611-L618 Governance Susceptibility

Description

The balance checkpointing system exposed by the contract for governance purposes is flawed as it does not maintain voting balances properly. In detail, the total supply of votes is tracked as the sum of all locked balances, however, the total voting power of an individual only tracks delegated balances. As a result, governance percentage thresholds will be significantly affected and potentially unmet.

Impact

The governance module may be unusable due to the significant discrepancy between "circulating" voting power supply and the actual voting power of each individual summed up.

Solution (Recommended Mitigation Steps)

We advise the total voting supply to properly track the delegated balances only as otherwise, any system relying on proportionate checkpointed balances will fail to function properly.

PoC

Issue is deducible by inspecting the relevant lines referenced in the issue and making note of the calculations within the getPastVotes individual voting power function as well as the getPastTotalSupply cumulative voting power function.

Tools

Manual inspection of the codebase.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels May 24, 2022
code423n4 added a commit that referenced this issue May 24, 2022
@0xMaharishi 0xMaharishi added invalid This doesn't seem right sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels May 27, 2022
@0xMaharishi
Copy link

This is intended behaviour. There is no incentive for users not to delegate their votes. And even if there were, not delegating is the equivalent to having voting power but not voting. Therefore this is not a relevant issue

@dmvt
Copy link
Collaborator

dmvt commented Jun 20, 2022

I'm going to leave this one in play and downgrade the severity. The warden's report is accurate, however, if the required percentages of voting cannot be met, the DAO would simply have to go on a campaign to get people to delegate their votes. This would be annoying but not critically destructive. That said, medium severity makes sense because a bad actor could potentially gather voting power and intentionally disrupt things by not delegating it. I'd recommend implementing the fix suggested by the warden.

@dmvt dmvt added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed invalid This doesn't seem right 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) 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