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

If proposal passes then `totalRewards` will be divided by `totalVotes` instead of `totalVotesFor` and transfer their calculated part to them who voted for only , remaining stuck forever

0x11singh99

Summary

When user votes for and against till the quorum reached and proposal passes. Then fund should be divided into the voters who voted for. But a flawed logic in _distributeRewards() function wrongly divided the fund by all voters and transferred only to them who voted for. Incorrect rewardPerVoter will be sent to those who voted for. And other remaining reward part of those voters who voted against will be stuck in the contract.

Vulnerability Details

There is no way the get the against voter fund if totalVotesFor are greater than totalVotesAgainst

In _distributeRewards function when totalVotesAgainst are smaller than totalVotesFor the else block is going to execute which is calculating rewardPerVoter for all voters which is incorrect we need to calculate reward for the voters who voted to totalVotesFor. So that all reward can be transferred.

/src/VotingBooth.sol#L192-L209

​
uint256 rewardPerVoter = totalRewards / totalVotes;//@audit-issue high divided by totalVotes instead of totalVotesFor
​
for (uint256 i; i < totalVotesFor; ++i) {//@audit only run for who voted for
// 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);//@audit transferring money to who voted for
​

POC

I added some lines and a new test in the given test file VotingBoothTest.t.sol.
To run the test use command
forge test --mt testVotePassesAndAgainstVotersPartRewardStuck -vv
Five voters need to vote in total 9 voters to reach the quorum and to pass proposal for votes will be 4 and against votes are only 1 so proposal will pass. But the part of against voters fund is stuck in current scenario 20% of total reward because 20% voter voted against. The reward was divided into 5 parts instead of 4 (who voted for) due to wrong logic and 1 part of that is stuck. We can see using assert the fund is there but we can't withdraw that. Because once the proposal passes contract becomes useless voting becomes inactive. After that even if we try to vote it will revert.

​
function setUp() public virtual {
// 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));
+ voters.push(address(0x6));
+ voters.push(address(0x7));
+ voters.push(address(0x8));
+ voters.push(address(0x9));
​
// 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));
}
​
// required to receive refund if proposal fails
receive() external payable {}
​
+ function testVotePassesAndAgainstVotersPartRewardStuck() public {
+ vm.prank(address(0x1));
+ booth.vote(true);
+
+ vm.prank(address(0x2));
+ booth.vote(true);
​
+ vm.prank(address(0x3));
+ booth.vote(true);
​
+ vm.prank(address(0x4));
+ booth.vote(true);
+ vm.prank(address(0x5));
+ booth.vote(false);//@audit one voter voted against
​
+ assert(address(booth).balance == ETH_REWARD / 5);//@audit this check will pass, Since reward was divided into 5 parts instead of 4. according to all voters but transferred only to who voted for. The rewrad stuck there is the part of voters who voted against. Here 1 voter voted against out of 5 voters so his respective 20% part stuck in contract. Now there is no way to withdraw it.
​
+ vm.exceptRevert();
+ vm.prank(address(0x6));
+ booth.vote(true); //@audit will revert with error "DP: voting has been completed on this proposal" now we can't vote anymore and 20% of ETH Reward stuck there forever
}
​

Impact

The user who voted to For their funds are not correct and also the reward divided by all voters number so remaining funds of against voters will be stuck forever in contract.

Tools Used

Foundry

Recommendations

Use totalVotesFor in place of totalVotes in the else block to divide totalRewards so voters who voted for there funds are accurately calculated.

change this below in _distributeRewards function of VotingBooth.sol. So award will only be divided into who voted for when proposal passes and no Eth will be stuck in contract.

- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
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.