search

code-423n4 / 2021-05-fairside-findings

0 stars 0 forks source link

Changing `ERC20ConvictionScore.governanceThreshold` leads to temporarily broken state #39

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

Vulnerability Details

Changing the governanceThreshold breaks the governance credit score accounting as users who currently qualify for being a governor may not qualify anymore and this influences the quorum threshold. It can be changed using FSD.updateGovernanceThreshold.

Impact

Imagine, governance calls updateGovernanceThreshold with a higher value disqualifying current governors but their ERC20ConvictionScore.isGovernance[user] state is not yet updated. Someone creates a proposal using the old higher threshold.

Someone updates the user states now, for example, by transferring a single wei to them, their status is reset in _updateConvictionScore and the quorum threshold might be unreachable by the updated collective of governors. The proposal needs to be cancelled, all users' isGovernance state needs to be updated to arrive at the correct totalVotes again.

Recommended Mitigation Steps

I don't see a good solution in the current design. If possible make governanceThreshold a constant or manually update all users' state after a change such that the totalVotes are correct again.

fairside-core commented 3 years ago

The governanceThreshold restriction is not retro-actively applied in the _updateConvictionScore function, what is evaluated at all times is the minimum balance threshold. As such, I do not see this as a valid issue.

cemozerr commented 3 years ago

@fairside-core's comments, i.e. "what is evaluated at all times is the minimum balance threshold" seem to be correct. The governance threshold seems to be only evaluated if the user is not currently part of governance, and thus this issue is invalid.