Summary
Incorrect calculation of rewardPerVoter inside _distributeRewards() results in lower rewards to be awarded to for voters and also considerable leftover amount in the contract is stuck which can't be withdrawn now or transferred.
ROOT CAUSE: Usage of totalVotes instead of totalVotesFor while calculating rewardPerVoter which gives an incorrect result when totalVotesFor totalVotes for a passed proposal.
Vulnerability Details
VotingBooth.sol#L192 calculates rewardPerVoter as:
uint256 rewardPerVoter = totalRewards / totalVotes;
This causes problems when totalVotesFor totalVotes and the proposal has passed. Consider the following scenario:
Suppose s_totalAllowedVoters = 5
Votes needed to reach quorum = 3 votes (Since )
Let us have totalRewards = 10 ether
voter votes AGAINST the proposal.
voter votes FOR the proposal.
voter votes FOR the proposal.
Hence, totalVotes = 3 and totalVotesFor = 2.
rewardPerVoter is calculated as per L192 which says:
. This equals 3333333333333333333 wei. This is incorrect & should be:
. Which equals 5000000000000000000 wei.
Also, the current reward distribution looks like this:
voter receives 3333333333333333333 wei
voter receives 3333333333333333334 wei
Dust left in the contract = 10 ether - 3333333333333333333 wei - 3333333333333333334 wei = 3333333333333333333 wei.
This leftover amount is now stuck in the contract since there is no withdraw or transfer or selfDestruct function supported by the protocol.
PoC-1
Add this test and run via forge test --mt testIncorrectReward -vv to see it fail.
@@ -46,6 +46,22 @@ contract VotingBoothTest is Test {
// required to receive refund if proposal fails
receive() external payable {}
+ function testIncorrectReward() public {
+ vm.prank(address(0x1));
+ booth.vote(false);
+
+ vm.prank(address(0x2));
+ booth.vote(true);
+
+ vm.prank(address(0x3));
+ booth.vote(true);
+
+ assert(!booth.isActive());
+ assertEq(address(0x2).balance, 5_000_000_000_000_000_000); // @audit : should have been 5000000000000000000 wei = 5 ether
+ assertEq(address(0x3).balance, 5_000_000_000_000_000_000); // @audit : should have been 5000000000000000000 wei = 5 ether
+ assertEq(address(booth).balance, 0, "dust remains"); // @audit : should have been 0, but is 3333333333333333333
+ }
+
function testVotePassesAndMoneyIsSent() public {
vm.prank(address(0x1));
booth.vote(true);
Output:
[FAIL. Reason: assertion failed] testIncorrectReward() (gas: 282888)
Logs:
Error: a == b not satisfied [uint]
Left: 3333333333333333333
Right: 5000000000000000000
Error: a == b not satisfied [uint]
Left: 3333333333333333334
Right: 5000000000000000000
Error: dust remains
Error: a == b not satisfied [uint]
Left: 3333333333333333333
Right: 0
PoC-2 (Fuzz Test - SIMPLER approach)
We can also uncover the bug using fuzzing. Let's modify the existing test testVotePassesAndMoneyIsSent() into a fuzz-style one which checks different vote (for/against) combinations for 3 voters.
Add this test and run via forge test --mt testFuzz_IncorrectReward -vv to see the counterexample which makes it fail.
function testFuzz_IncorrectReward(bool vote1, bool vote2, bool vote3) public {
vm.prank(address(0x1));
booth.vote(vote1);
vm.prank(address(0x2));
booth.vote(vote2);
vm.prank(address(0x3));
booth.vote(vote3);
assert(!booth.isActive() && address(booth).balance == 0);
}
Output:
Running 1 test for test/VotingBoothTest2.t.sol:VotingBoothTest
[FAIL. Reason: panic: assertion failed (0x01); counterexample: calldata=0xe6502645000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001 args=[false, true, true]] testFuzz_IncorrectReward(bool,bool,bool) (runs: 5, μ: 192560, ~: 196542)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 15.09ms
PoC-3 (Fuzz style - LONGER Approach)
We'll design a fuzz test which tries out different combination of not only the votes (true/false) but also the number of total eligible voters. This will help us see various counterexamples of failures.
Params being fuzzed:
Save the following code inside a new file test/VotingBoothTestFuzzer2.t.sol and run via forge test --mt testFuzz_Style2_IncorrectReward -vv.
pragma solidity ^0.8.23;
import {VotingBooth} from "../src/VotingBooth.sol";
import {Test} from "forge-std/Test.sol";
contract VotingBoothTestFuzzer2 is Test {
uint256 constant ETH_REWARD = 10e18;
uint256 constant MIN_QUORUM = 51;
address[] voters;
VotingBooth booth;
receive() external payable {}
function testFuzz_Style2_IncorrectReward(uint256 eligibleVotersCount, bool[] memory voted) public {
eligibleVotersCount = bound(eligibleVotersCount, 3, 9);
vm.assume(eligibleVotersCount % 2 == 1);
vm.assume(voted.length == eligibleVotersCount);
deal(address(this), ETH_REWARD);
for (uint256 n = 1; n <= eligibleVotersCount; ++n) {
voters.push(address(uint160(n)));
}
booth = new VotingBooth{value: ETH_REWARD}(voters);
assert(address(booth).balance == ETH_REWARD);
assert(booth.isActive());
assert(booth.getTotalAllowedVoters() == voters.length);
assert(booth.getCreator() == address(this));
for (uint256 i = 1; i <= eligibleVotersCount; ++i) {
vm.prank(voters[i - 1]);
booth.vote(voted[i - 1]);
if (i * 100 / eligibleVotersCount >= MIN_QUORUM) {
break;
}
}
assertEq(booth.isActive(), false, "still active");
assertEq(address(booth).balance, 0, "balance not zero");
}
We'll run the fuzz test a couple of times so that we can see multiple counterexamples :
Output of run:
[FAIL. Reason: assertion failed; counterexample: calldata=0x800ff8430000000000000000000000000000000000000000000000000000000000001aae0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000 args=[6830, [false, true, true, true, false]]] testFuzz_Style2_IncorrectReward(uint256,bool[]) (runs: 2, μ: 908225, ~: 908225)
Logs:
Bound Result 5
Error: balance not zero
Error: a == b not satisfied [uint]
Left: 3333333333333333333
Right: 0
Output of run:
[FAIL. Reason: assertion failed; counterexample: calldata=0x800ff84300000000000000000000000000000000000000000000000000000000000008b9000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000070000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001 args=[2233, [true, true, false, true, true, true, true]]] testFuzz_Style2_IncorrectReward(uint256,bool[]) (runs: 3, μ: 849984, ~: 850117)
Logs:
Bound Result 7
Error: balance not zero
Error: a == b not satisfied [uint]
Left: 2500000000000000000
Right: 0
Impact
Tools Used
Foundry, manual inspection.
Recommendations
Make the following changes:
VotingBooth.sol#L182-L208
// if the proposal was defeated refund reward back to the creator
// for the proposal to be successful it must have had more `For` votes
// than `Against` votes
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) {
// 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); // @audit: This fix can be improved. See note below.
}
Note: The above fix should be implemented in conjunction with the recommendation mentioned in another bug (raised separately) titled: Dust can still remain after _distributeRewards(). This will make sure that no dust amount remains at the end. Otherwise for certain combinations of totalRewards and totalVotesFor, there are chances that dust still remains in the contract in spite of:
VotingBooth.sol#L207
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+ rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, Math.Rounding.Ceil); // @audit: This fix can be improved. See note below.
The bug report Dust can still remain after _distributeRewards() explains how to fix this. It basically says that we need to change this to:
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+ rewardPerVoter = address(this).balance; // @audit: No dust will remain now
