Summary
Imperfect division in rewardPerVoter calculation. That makes each voters, in more case, to receive 1 less wei of reward, which would be left in the contract forever. And that breaks the Invariant of contract ending balance being 0.
Vulnerability Details
uint256 rewardPerVoter = totalRewards / totalVotes;
is rounding down for each voters' reward. And the last vote only (potentially) receives 1 more wei by rounding up, but the leftover can be more than that depending on the number of voters who voted for, and the total reward.
Impact
The Invariant of contract ending balance being 0, is broken.
Proof of Concept
using a fuzz test, found a case that:
totalRewards = 1e18 + 2
number of voters = 9
5 voters vote For, 0 votes Against
rewardPerVoter = 2e17 for first 4 voters
and rewardPerVoter =2e17 + 1 for 5th voter
but there is 1 wei left in contract after distribution
function testFuzzVotePassesAndMoneyIsNotAllSent(uint256 reward, uint160 numberOfVoters) public {
reward = bound(reward, 1 ether, type(uint256).max);
numberOfVoters = uint160(bound(numberOfVoters, 3, 9));
numberOfVoters = numberOfVoters / 2 * 2 + 1;
voters = new address[](numberOfVoters);
for (uint160 i; i < numberOfVoters; i++) {
voters[i] = address(i + 1);
}
deal(address(this), reward);
booth = new VotingBooth{value: reward}(voters);
assert(address(booth).balance == reward);
assert(booth.isActive());
assert(booth.getTotalAllowedVoters() == voters.length);
assert(booth.getCreator() == address(this));
for (uint160 i; i < numberOfVoters / 2 + 1; i++) {
vm.prank(voters[i]);
booth.vote(true);
}
assert(!booth.isActive());
assertEq(address(booth).balance, 0);
}
Tools Used
Foundry, Fuzz test
Recommendations
(A) for the last voter, transfer whatever left in the contract balance:
if (i == totalVotesFor - 1) {
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+ rewardPerVoter = address(this).balance;
}
(B) transfer leftover to proposal creator
+ _sendEth(s_creator, address(this).balance);