Vote Duplication in Sweep Wars

[aleph-v]

The sweep wars contract records a user's voting power based on the ratio at the beginning of their vote. When votes are removed the contract calculates the user's current voting power based on their current locking ratio and then either removes those votes if the vote is for for a collection or adds them if the vote is against a collection. The consequence of this is if a user can either increase or reduce their voting power after voting some remainder will be left when they withdraw at the end because the voting power ratio at the end of the locking period is not equal to what it was at the beginning. This allows vote duplication via the following steps: 1) Deposit at the lowest possible lock length (2 weeks) 2) Vote in the opposite of the preferred direction (with 1/12 ie about 8.6% of total votes) 3) Extend lock by depositing again to the longest lock length (24 weeks) 4) Wait for lock to end and withdraw causing the correction to overshoot because the user's current voting power is 100% of what was deposited (by aprox 91.4% with current constants) 5) Deposit and vote again and you will have added 191% of your voting power to your preferred collection.

• Severity Estimate: Critical [Impact: High, Likelihood: High]
• Remediation Recommendation: Store real user votes not their total amount of floor voted.

[tomwade]

I can confirm that this issue is replicable through a test suite

[tomwade]

Proposed Solution:

• User deposits a second amount (setting a different epoch lock period)
• This will call the war contract to change the power applied to a collection(s) for the user

This problem appears to be arising because the initial vote is done at a lower power, then an initial deposit will increase the power. When a withdraw is called, the votes at negated at the new power (so a 20% negative vote becomes an 80% positive vote). By updating the power at the point of power change, then this will avoid this incorrect negation.

[aleph-v]

I think it's simpler to store the user votes in the mapping itself instead of the floor token amounts which were voted with. This means when revoke is called you can negate any effect they had. If a user wants to vote multiple times you can call your free voting power function and check their global votes cast is not greater than their current balance's votes.

[tomwade]

I'll have a bit more of an in-depth review of both approaches, as off the top of my head I'm a little concerned about restaking giving different powers for that approach. For example, relocking for a shorter period that would reduce the power but that wouldn't be reflected in the vote.

This may not be an issue though. I'll review and implement, as I think the best solution here will also be mapped onto New Collection Wars as raised in issue #49 .

[aleph-v]

This is maybe out of scope for the issue (and audit) but I think y'all could simplify the codebase a lot by combining the epoc manager, staking contract, and voting contracts into one contract. You do a lot of sync callbacks between each one because it the state affects eachother but instead making them a single structure could simply your abstractions.