code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/VoterRegistration.sol#L53-L56 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L259

Vulnerability details

Impact

One can avoid to be revoked their votes

Proof of Concept

Let's assume there is a running votes. If an user will be revoked their votes by VoteRegistration::revokeVotesFrom, they can call Governance::vote with high gas fee to avoid for their votes to be taken away by front-running the revokeVotesFrom.

The Governance::vote will transfer the votes from the user to the governance contract. Therefore, after the vote, the user does not have any votes in their address. So, the VoteRegistration::revokeVotesFrom will revert and no votes are revoked.

To be really sure, the user keep their votes always in the Governance contract by voting and reclaim only before the next vote. This way, the user will have the voting power, but they will be always safe from any revokes.

// policies/VoterRegistration::revokeVotesFrom

 53     function revokeVotesFrom(address wallet_, uint256 amount_) external onlyRole("voter_admin") {
 54         // Revoke the votes in the VOTES module
 55         VOTES.burnFrom(wallet_, amount_);
 56     }

// policies/Governance::vote
 259         VOTES.transferFrom(msg.sender, address(this), userVotes);

Tools Used

none

Recommended Mitigation Steps

If the votes should be transferred to the governance after the vote, the revoke function should also decrease those votes.

fullyallocated commented 2 years ago

Technically correct but insignificant, the Voter Registration is just for testing purposes and real votes won't utilize this mechanic

0xean commented 1 year ago

going to mark this as a rough dupe of #275 - I don't think the "front running" makes this special in any way and really comes down to the same root issue. Votes that are revoked still count.