First Flight #6: Voting Booth
First Flight #6
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect distribute logic potentially locks ETH in contract

rocknet

Summary

Incorrect denominator used when determining the rewards per voter will lock ETH in the contract, unless all voters vote "For".

Vulnerability Details

The logic used to determine the reward amount to send to each "For" voter is flawed. The contract is dividing by all voters, rather than the number of "For" voters, as intended. This results in some ETH value (in proportion to the "Against" voters) being locked in the contract.

rewardPerVoter should not be divided by the total votes, as it is here:

uint256 rewardPerVoter = totalRewards / totalVotes;

and here:

if (i == totalVotesFor - 1) {
rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
}

Rather, they should be divded by totalVotesFor. This also aligns with the comments and README.

Impact

The only way the balance is spent from the VotingBooth contract is via _distributeRewards(). After _distributeRewards() is called, if there is any ETH remaining in the contract balance, it's locked. Depending on the reward level, this could be a large loss of value to the creator, and be extension, to those that voted "For".

Tools Used

Initially I created an invariant fuzz test, like so:

function invariant_NotActiveBalanceZero() public {
if (!booth.isActive()) {
console.log("balance: %s", address(booth).balance);
}
assertEq(!booth.isActive(), address(booth).balance == 0);
}

Not shown is the setup function which used targetSender() to constrain the voter msg.sender to the list of voters. This test showed that the balance of the contract was not 0 when voting was complete, as was intended, when the vote passed, but some portion of the quorum voted against.

I then moved to a unit test / PoC to show that the remaining balance was the portion of "Against" votes. For example, in a quorum of 5, if two voted "Against", the amount of reward left in the contract was 40%. (2/5 = 0.4). The following test passes, but it's exhibiting a flaw, and showing the specifics of the flawed results.

function testValueStrandedByVotesAgainst() public {
vm.prank(address(0x1));
booth.vote(false);
​
vm.prank(address(0x2));
booth.vote(false);
​
vm.prank(address(0x3));
booth.vote(true);
​
vm.prank(address(0x4));
booth.vote(true);
​
vm.prank(address(0x5));
booth.vote(true);
​
console.log("balance: %s", address(booth).balance);
assert(!booth.isActive() && address(booth).balance == 4 ether);
}

Recommendations

Write fuzz tests for all invariants to catch logic issues. Consider expanding on unit tests for more edge cases (current tests had all true or all false). Finding issues with fuzz tests can help direct better unit tests.

Change the logic for calculating rewardPerVoter to divide by totalVotesFor. Further, consider changing the name of the variable to align with the intention, e.g. rewardPerForVoter. In this instance, the name of the variable aligned with the flawed logic and not the intent. If the logic is changed, the variable name should be considered for update.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

VotingBooth._distributeRewards(): Incorrect computation of rewardPerVoter

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.