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

The `VotingBooth::_distributeRewards` function fees distribution can lead to money stuck forever in the contract

DenSosnovskyi

Description: The VotingBooth::_distributeRewards function always distribute the rewarding fees by dividing the funding by all voters count, which leads to money losing forever if at least one "against" voter has voted. The more "against" voters vote and the bigger funding is, the more money will be lost in case "for" voters win. It could be not a problem if the creator could withdraw the money, but there no such a function in the contract. In case "for" voters expect to receive all funding, their expectations will be wrong as well.

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 {
@> uint256 rewardPerVoter = totalRewards / totalVotes;
​
for (uint256 i; i < totalVotesFor; ++i) {

Impact: Creator can unintentionally lose his/her money

Proof of Concept:

If "against" and "for" voters vote, and "for" voters win, the creator will lose part of the money stuck in the contract forever.

Code

Place the following into VotingBoothTest.t.sol

function testCreatorLosePartOfMoneyWhenForAndAgainstVoteAndForVotersWin() public {
uint256 ETH_REWARD = 3e18;
​
deal(address(this), ETH_REWARD);
address[] memory voters = new address[](5);
​
voters[0] = address(0x1);
voters[1] = address(0x2);
voters[2] = address(0x3);
voters[3] = address(0x4);
voters[4] = address(0x5);
​
booth = new VotingBooth{value: ETH_REWARD}(voters);
​
vm.prank(address(0x1));
booth.vote(false);
vm.prank(address(0x2));
booth.vote(true);
vm.prank(address(0x3));
booth.vote(true);
​
// contract money cannot be withdrawn after it is not active
assert(!booth.isActive());
// the contract close 1 ETH forever
assert(address(booth).balance == 1e18);
// the first "for" voter receives 1 ETH
assert(address(0x2).balance == 1e18);
// the second "for" voter receives 1 ETH
assert(address(0x3).balance == 1e18);
}

Recommended Mitigation: There are two alternatives.

  1. If the creator is 100% trusted, the withdraw function with only creator access can be created so that the creator could take back all money from contract after the voting was closed. But it is still not recommended, because there is always a threat of the creator being compromised.

  2. Distribute all funding among the "for" voters.

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 {
- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
​
for (uint256 i; i < totalVotesFor; ++i) {
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.