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

'VotingBooth:_distributeRewards' Does not pay out the correct amount to 'for' voters under certain conditions

coffee

Summary

The 'VotingBooth:_distributeRewards' function does not reward the correct amount to 'for' voters under certain conditions where the 'totalAllowedVoters' is either 5, 7, or 9 and there is at least 1 'against' vote.

Vulnerability Details

When voting ends and if the 'for' votes win, _distributeRewards is called. The rewardPerVoter is calculated by taking the (totalRewards / totalVoters). In cases where there are 5, 7, or 9 totalAllowedVoters and there is at least 1 'against' vote, this will result in an incorrect payout and leave money left over in the contract.

For example, if there are 5 totalAllowedVoters and the first two voters vote 'true' and the third votes 'false' this will end the vote. _distributeRewards will be called and the rewardPerVoter will be calculated to 0.33 -> (1 ETH / 3 totalVoters). This will pay out 0.33 to the two 'for' voters and leave 0.33 in the contract. This should payout 0.5 to each of the 'for' voters.

Impact

The below fuzz test fails because the contract balance is not zero after the vote has ended. There is a balance left over.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import {VotingBooth} from "../../src/VotingBooth.sol";
import {Test, console} from "forge-std/Test.sol";
import {StdInvariant} from "forge-std/StdInvariant.sol";
​
contract OpenInvariantsTest is Test {
// eth reward
uint256 constant ETH_REWARD = 10e18;
​
// allowed voters
address[] voters;
​
// contracts required for test
VotingBooth booth;
​
function setUp() public {
// deal this contract the proposal reward
deal(address(this), ETH_REWARD);
​
// setup the allowed list of voters
voters.push(address(0x1));
voters.push(address(0x2));
voters.push(address(0x3));
voters.push(address(0x4));
voters.push(address(0x5));
​
// constrain fuzz test senders to the set of allowed voting addresses
for (uint256 i; i < voters.length; ++i) {
targetSender(voters[i]);
}
​
// setup contract to be tested
booth = new VotingBooth{value: ETH_REWARD}(voters);
​
// verify setup
//
// proposal has rewards
assert(address(booth).balance == ETH_REWARD);
// proposal is active
assert(booth.isActive());
// proposal has correct number of allowed voters
assert(booth.getTotalAllowedVoters() == voters.length);
// this contract is the creator
assert(booth.getCreator() == address(this));
​
targetContract(address(booth));
}
​
receive() external payable {}
​
function invariant_protocolBalanceShouldBeZeroIfVoteEnded() public view {
if (!booth.isActive()) {
console.log("Contract Balance: ", address(booth).balance);
assert(address(booth).balance == 0);
}
}

Tools Used

--Foundry

Recommendations

It is recommended to change the logic in the 'VotingBooth:_distributeRewards' function to calculate the rewardPerVoter using the totalVotesFor instead of totalVotes when the vote passes 'for'.

else {
- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
​
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);
+ rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, 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.