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

Reward calculation error results in incorrect payouts and stuck ether

wallebach

Summary

The formula used in _distributeRewards() to calculate rewards when a proposal is passed is incorrect. Voters may be rewarded less than they should be. Ether may be left in the contract with no way to get it back.

Vulnerability Details

According to the rules of the VotingBooth contract, the reward is distributed among the for voters if the proposal passes. for voters are the only recipients of the Ether reward. Each for voter receives an equal share of the reward.

The test testVotePassesAndMoneyIsSent() shows that the rules of the contract are being followed when the quorum has been reached and when all the voters voted in favour of the proposal. However, if we write a fuzzing test (see Recommendation #2) or change testVotePassesAndMoneyIsSent(), e.g. like this:

// VotingBoothTest.s.sol, lines 56-57
vm.prank(address(0x3));
- booth.vote(true);
+ booth.vote(false);

It will fail.

The error occurs because the reward distribution formula is not entirely correct. The expression:

uint256 rewardPerVoter = totalRewards / totalVotes;

Takes into account all the votes. However only for votes must count. A similar error is encountered during the last voter's reward calculation:

rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);

The error causes those who for-voted to receive less Ether than they should. For example, if 3 of 5 voted in favour, they get totalRewards / 5 ETH, although they should get totalRewards / 3 ETH. This also results in some of the ether remaining in the smart contract. See the example with comments below:

function outputActualVsExpectedRewards() public {
// uint256 constant ETH_REWARD = 10e18;
vm.prank(address(0x1));
booth.vote(true);
​
vm.prank(address(0x2));
booth.vote(true);
​
vm.prank(address(0x3));
booth.vote(false);
​
console.log(address(0x1).balance); // 3333333333333333333, but must be 5000000000000000000
console.log(address(0x2).balance); // 3333333333333333334, but must be 5000000000000000000
console.log(address(booth).balance); // 3333333333333333333, but must be zero
}

To correct the error totalVotes must be replaced by totalVotesFor (see Recommendation #1).

Once the fix is in place, it's also safe to remove the totalVotes variable because it's not being used anywhere else in the code.

Impact

High:

  • Voters might not be rewarded as expected by the rules.

  • It will not be possible to retrieve any ether left in the contract.

Tools Used

  • Manual check

  • Fuzzing test

Recommendations

  1. Formula for rewardPerVoter must be changed in two places:

// line 192
- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
...
// line 207
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+ rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, Math.Rounding.Ceil);

  1. Integrate fuzzing tests into the test suite. For example, the following fuzzing test can be used to easily find the vulnerability by using automatic substitution of different combinations of boolean values in vote1, vote2 and vote3 during each test run:

function testVotingWithFuzzing(bool vote1, bool vote2, bool vote3) public {
vm.prank(address(0x1));
booth.vote(vote1);
​
vm.prank(address(0x2));
booth.vote(vote2);
​
vm.prank(address(0x3));
booth.vote(vote3);
​
assert(!booth.isActive() && address(booth).balance == 0);
}
Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years 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.