Summary
The calculation to determine the rewardPerVoter uses the totalVotes which can cause the value to be lower than intended and leaves dust in the contract.
Vulnerability Details
When the proposal has passed, the calculation to determine how much of the totalRewards to distribute to each of the s_votersFor uses the totalVotes. If there were any s_votersAgainst then the result will be less than it should. Additionally, since the calculation to "avoid leaving dust" will also not produce the correct value.
This can be demonstrated by adding some stateful fuzz testing to the current VotingBoothTest.t.sol test.
+import {StdInvariant} from "forge-std/StdInvariant.sol";
-contract VotingBoothTest is Test {
+contract VotingBoothTest is StdInvariant, Test {
// eth reward
uint256 constant ETH_REWARD = 10e18;
// allowed voters
address[] voters;
+ address[] votersFor;
// contracts required for test
VotingBooth booth;
function setUp() public virtual {
// ... snip ...
assert(booth.getTotalAllowedVoters() == voters.length);
// this contract is the creator
assert(booth.getCreator() == address(this));
+
+ targetContract(address(booth));
+ for (uint i = 0; i < voters.length; i++) {
+ targetSender(voters[i]);
+ }
+ // include non-voter addresses for fuzz testing
+ targetSender(address(0x6));
+ targetSender(address(0x7));
+ targetSender(address(0x8));
}
+
+ function statefulFuzz_vote() public {
+ assertEq(booth.getCreator(), address(this), "Creator is wrong!");
+ assertEq(booth.getTotalAllowedVoters(), voters.length, "Total allowed voters is wrong");
+ if (booth.isActive()) {
+ assertEq(ETH_REWARD, address(booth).balance, "Reward drained");
+ } else {
+ assertEq(0, address(booth).balance, "Reward not sent");
+ if (address(this).balance == ETH_REWARD) {
+ // vote was defeated, all votes must not have a balance.
+ for (uint i = 0; i < voters.length; i++) {
+ assertEq(voters[i].balance, 0);
+ }
+ } else {
+ // we can learn who voted "for" the proposal by checking their balance
+ for (uint i = 0; i < voters.length; i++) {
+ if (voters[i].balance > 0) {
+ votersFor.push(voters[i]);
+ }
+ }
+ uint256 rewardPerVoter = ETH_REWARD / votersFor.length;
+ for (uint i = 0; i < votersFor.length; i++) {
+ assertLe(rewardPerVoter, votersFor[i].balance);
+ }
+ }
+ }
+ }
}
In addition, the foundry.toml will need the following values added:
+[invariant]
+runs = 1000
+depth = 10
+fail_on_revert = false
Alternatively, adding the another unit test with a more mixed vote could also demonstrate the problem.
+
+ function testVotesForGetRewardedCorrectly() public {
+ vm.prank(address(0x2));
+ booth.vote(true);
+ vm.prank(address(0x3));
+ booth.vote(false);
+ vm.prank(address(0x4));
+ booth.vote(true);
+
+ assert(!booth.isActive());
+ assertEq(0, address(booth).balance, "VotingBooth should not have balance");
+ assertEq((ETH_REWARD/2), address(address(0x2)).balance, "Voter(2) for should have at least half the pool");
+ assertGe((ETH_REWARD/2), address(address(0x4)).balance, "Voter(4) for should have at least half the pool");
+ assertEq(0, address(address(0x3)).balance, "VoterAgainst should not have balance");
+ }
Impact
Not all rewards will be distributed to those who voted if not all voters voted for the proposal.
Tools Used
Foundry Fuzz Testing & Manual Review
Recommendations
It is recommended that VotingBooth::_distributeRewards() logic to send rewards to the addresses that voted for the proposal should be altered to use the count of totalVotesFor instead of totalVotes. This will ensure that all rewards are distributed.
function _distributeRewards() private {
- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor;
for (uint256 i; i < totalVotesFor; ++i) {
// proposal creator is trusted when creating allowed list of voters,
// 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);
}
}
