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

Rewards are not zero if the proposal passes and there is at least one false vote due to the wrong logic when calculating `rewardPerVoter`

bube

Summary

In the VotingBooth::_distributeRewards function the rewardPerVoter is not calculated correctly and the for voters can receive less rewards than they should receive.

Vulnerability Details

After the proposal is passed the rewards should be distributed among the for voters and after that the contract balance should be zero. The VotingBooth::vote function calls VotingBooth::_distributeRewards function that checks if the proposal pass or not and calculates the rewardPerVoter, this is the reward that the for voter should receive. But in this calculation the totalRewards are divided by totalVotes instead of totalVotesFor. In the calculation for the last voter is used also totalVotes. The totalVotes are all voters that vote with for and against. Therefore, the for voter will receive less rewards than it is intended if there are against voters and the proposal is passed. The against voters do not receive rewards, therefore there are rewards in the contract balance after the distribution. If all voters are for voters the rewards are distributed correctly. The following code shows the places with wrong calculation:

function _distributeRewards() private {
// get number of voters for & against
uint256 totalVotesFor = s_votersFor.length;
uint256 totalVotesAgainst = s_votersAgainst.length;
uint256 totalVotes = totalVotesFor + totalVotesAgainst;
​
// rewards to distribute or refund. This is guaranteed to be
// greater or equal to the minimum funding amount by a check
// in the constructor, and there is intentionally by design
// no way to decrease or increase this amount. Any findings
// related to not being able to increase/decrease the total
// reward amount are invalid
uint256 totalRewards = address(this).balance;
​
// if the proposal was defeated refund reward back to the creator
// for the proposal to be successful it must have had more `For` votes
// than `Against` votes
if (totalVotesAgainst >= totalVotesFor) {
// proposal creator is trusted to create a proposal from an address
// that can receive ETH. See comment before declaration of `s_creator`
_sendEth(s_creator, totalRewards);
}
// otherwise the proposal passed so distribute rewards to the `For` voters
else {
//@audit rewardPerVoter should be calculated for the number of votersFor, not all voters
@> uint256 rewardPerVoter = totalRewards / totalVotes;
​
for (uint256 i; i < totalVotesFor; ++i) {
// proposal creator is trusted when creating allowed list of voters,
// findings related to gas griefing attacks or sending eth
// to an address reverting thereby stopping the reward payouts are
// invalid. Yes pull is to be preferred to push but this
// has not been implemented in this simplified version to
// reduce complexity & help you focus on finding the
// harder to find bug
​
// if at the last voter round up to avoid leaving dust; this means that
// the last voter can get 1 wei more than the rest - this is not
// a valid finding, it is simply how we deal with imperfect division
if (i == totalVotesFor - 1) {
@> rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}
}
​
​

Impact

If we have 9 allowed voters, the quorum is reached at the 5th voter. So, let's consider we have 5 votes: 2 false and 3 true. The proposal passes and the for voters should receive rewards and the balance of the contract should be zero after that. But the calculation of the rewardPerVoter in the VotingBooth::_distributeRewards function includes also the against voters and their rewards remains in the contract after the distribution. And the for voters receive less rewards than they should receive.
In the test file VotingBoothTest.t.sol the test case that demonstrates the rewards distribution among voters uses only true votes. But the described problem arises when one or more of the votes are false and the proposal passes. The following test function testVotePassesAllRewardsDistributed demonstrates this problem. The function can be added to the test file and executed by Foundry command: forge test --match-test "testVotePassesAllRewardsDistributed" -vvv.

Unit Test
function testVotePassesAllRewardsDistributed() public {
vm.prank(address(0x1));
booth.vote(false);
​
vm.prank(address(0x2));
booth.vote(true);
​
vm.prank(address(0x3));
booth.vote(true);
​
vm.prank(address(0x4));
booth.vote(false);
​
vm.prank(address(0x5));
booth.vote(true);
​
// show the rewards in contract after distribution
console.log('rewards in contract after distribution', address(booth).balance);
// show the rewards that every voter is received
console.log("0x1", address(0x1).balance);
console.log("0x2", address(0x2).balance);
console.log("0x3", address(0x3).balance);
console.log("0x4", address(0x4).balance);
console.log("0x5", address(0x5).balance);
// check that the voting is inactive and the balance of the contract is 0
assert(!booth.isActive() && address(booth).balance == 0);
}
​
​

This test shows that the rewards in the contract after the distribution among the for voters are: rewards in contract after distribution 4000000000000000000. But the intended behavior of the protocol is that if the proposol passes and the rewards are distributed among the for voters, the rewards in the contract after that should be zero. But the assertion address(booth).balance == 0 in the showed test fails. That is because the wrong logic in the calculation of rewardPerVoter. In this calculation the rewards are divided by the number of all voters instead of the number of the for voters. Only the for voters receive rewards. Therefore the part of the rewards that is calculated for the rest voters (against voters) remains in the contract. In that way the for voters receive less reward than they actually should receive.

I found this issue by manual review and unit test, but the task was to write fuzz test. I'm not sure if my fuzz test is written in the correct way (it is my first attempt to write fuzz test). It found the issue. And I write this explanation, because this is 'learning' audit and I want to increase my knowledges in testing. I believe that the unit test is enough for the proof of this issue.
Here is my fuzz test:

Fuzz Test
​
/**
* forge-config: default.fuzz.runs = 1024
* forge-config: default.fuzz.max-test-rejects = 500
*/
function testFuzzVote(uint8[9] calldata votes, uint256 allowedVotersLength) public {
allowedVotersLength = bound(allowedVotersLength, 3, 9);
vm.assume(allowedVotersLength % 2 == 1);
uint256 votersFor;
uint256 votersAgainst;
uint256 startingAmount = address(booth).balance;
deal(address(this), ETH_REWARD);
​
// add addresses for voters, uses the addreses from voters[]
for (uint256 i; i < allowedVotersLength; i++) {
vt.push(voters[i]);
console.log(vt[i]);
}
​
// setup contract to be tested
booth = new VotingBooth{value: ETH_REWARD}(vt);
​
// Cast votes based on the fuzzed input
for (uint256 i = 0; i < vt.length; i++) {
// Skip if voting is already completed
if (!booth.isActive()) {
break;
}
​
// Prank the voter to simulate voting
vm.prank(vt[i]);
// Cast the vote, converting the uint8 to a bool
booth.vote(votes[i] % 2 == 1);
console.log(votes[i]);
if (votes[i] % 2 == 1)
votersFor++;
else
votersAgainst++;
}
​
// Check that voting has ended
bool active = booth.isActive();
if (!active) {
// Attempt to vote again should fail
for (uint256 i = 0; i < vt.length; i++) {
vm.prank(vt[i]);
vm.expectRevert("DP: voting has been completed on this proposal");
booth.vote(votes[i] % 2 == 1);
}
​
//rewards should match starting balance if the against votes are more or equal to the for votes
if (votersAgainst >= votersFor) {
assertEq(address(this).balance, startingAmount);
assert(address(this).balance >= startingAmount);
}
​
// Check that the contract's balance is zero after rewards distribution
assertEq(address(booth).balance, 0, "Contract balance should be zero after rewards distribution");
​
} else {
// the balance of the contract should be greather than 0 if the proposal is active
assert(address(booth).balance > 0);
}
}
​

This test can be added to the test file of the project and executed through the command: forge test --match-test "testFuzzVote" -vvvv.
The test function checks several states of the protocol:

  1. If the voting ends after the quorum is reached.

  2. If rewards match starting balance if the against votes are more or equal to the for voters

  3. If the contract's balance is zero after rewards distribution.

  4. If the balance of the contract is greather than 0 when the proposal is active.

And the test fails at the assertion that the contract's balance is zero after rewards distribution.

Tools Used

Manual Review, Foundry

Recommendations

Replace the totalVotes with totalVotesFor in calculation rewardPerVoter in the VotingBooth::_distributeRewards function.

function _distributeRewards() private {
.
.
.
// otherwise the proposal passed so distribute rewards to the `For` voters
else {
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
- uint256 rewardPerVoter = totalRewards / totalVotes;
​
for (uint256 i; i < totalVotesFor; ++i) {
// proposal creator is trusted when creating allowed list of voters,
// findings related to gas griefing attacks or sending eth
// to an address reverting thereby stopping the reward payouts are
// invalid. Yes pull is to be preferred to push but this
// has not been implemented in this simplified version to
// reduce complexity & help you focus on finding the
// harder to find bug
​
// if at the last voter round up to avoid leaving dust; this means that
// the last voter can get 1 wei more than the rest - this is not
// a valid finding, it is simply how we deal with imperfect division
if (i == totalVotesFor - 1) {
+ rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, Math.Rounding.Ceil);
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}
}
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.