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

Invalid variable used for reward distribution `totalVotes` instead of `totalVotesFor`

0xrektified

Summary

In the _distributeRewards method, the protocol incorrectly calculates rewardPerVoter by dividing totalRewards by totalVotes.

However, the reward is actually distributed only to rewardPerVoter users.

--> uint256 rewardPerVoter = totalRewards / totalVotes;

This error is also present in the last user reward distribution and needs correction.

--> rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);

As the reward is intended for "voters For," a significant amount will remain as leftover in the contract.

Vulnerability Details

First, let's configure the foundry to properly fuzz this contract:

[fuzz]
runs = 1000
max_test_rejects = 65430
seed = '0x3e84'
dictionary_weight = 40
include_storage = true
include_push_bytes = true

To catch this error, we can run the following stateless fuzz test:

pragma solidity ^0.8.23;
​
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
​
import {VotingBooth} from "../src/VotingBooth.sol";
import {Test} from "forge-std/Test.sol";
import {_CheatCodes} from "./mocks/CheatCodes.t.sol";
import {StdInvariant} from "forge-std/StdInvariant.sol";
import {console2} from "forge-std/console2.sol";
​
contract VotingBoothTest is Test {
// allowed voters
address[] voters;
​
// contracts required for test
VotingBooth booth;
​
_CheatCodes cheatCodes = _CheatCodes(HEVM_ADDRESS);
address[] votersFor;
uint256 private constant MIN_QUORUM = 51;
mapping(address user => bool result) s_voters_result;
​
function setUpFuzz(uint256 numberOfUser, uint160 valueInETHToFundTheContract) internal {
for (uint256 i; i < numberOfUser; i++) {
voters.push(makeAddr(vm.toString(valueInETHToFundTheContract + i)));
}
booth = new VotingBooth{value: valueInETHToFundTheContract}(voters);
// verify setup
//
// proposal has rewards
assert(address(booth).balance == valueInETHToFundTheContract);
// 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));
}
​
function userVote(uint256 numberOfUser, uint256 totalVoter) internal {
uint256 totalAllowedVoter = booth.getTotalAllowedVoters();
​
for (uint256 i; i <= numberOfUser; i++) {
// Generate random vote value based on use address modulo
bool voteValue = uint256(uint160(voters[i])) % 2 == 0;
// Impersonate the user to pass a vote
vm.startPrank(voters[i]);
// Submit the vote
booth.vote(voteValue);
​
// Store the expected result in our test
s_voters_result[voters[i]] = voteValue;
// Keep track of the number of voterFor and
if (voteValue) {
votersFor.push(voters[i]);
}
++totalVoter;
if (!booth.isActive()) {
require(totalVoter * 100 / totalAllowedVoter >= MIN_QUORUM, "quorum: should be reach");
break;
} else {
require(totalVoter * 100 / totalAllowedVoter < MIN_QUORUM, "quorum: should not be reach");
}
vm.stopPrank();
}
}
​
function testFuzz_BalanceFocus(uint256 numberOfUser, uint160 valueInETHToFundTheContract) public {
numberOfUser = bound(numberOfUser, 3, 9);
vm.assume(numberOfUser % 2 != 0);
vm.assume(valueInETHToFundTheContract >= 1e18);
deal(address(this), valueInETHToFundTheContract);
setUpFuzz(numberOfUser, valueInETHToFundTheContract);
userVote(numberOfUser, 0);
​
uint256 totalVotesFor = votersFor.length;
if (!booth.isActive()) {
uint256 userFor = booth.getVotersForLength();
uint256 userAgainst = booth.getVotersAgainstLength();
if (userAgainst >= userFor) {
// The contract creator should get all the amount used to fund the contract
require(
address(this).balance == valueInETHToFundTheContract, "post-run: creator should get all funds back"
);
} else {
// Check if all users got their funds back properly
for (uint256 i; i < totalVotesFor; i++) {
uint256 rewardPerVoter = valueInETHToFundTheContract / totalVotesFor;
if (i == totalVotesFor - 1) {
require(
address(votersFor[i]).balance == rewardPerVoter,
"post-run: last user should have max +1 wei"
);
} else {
require(
address(votersFor[i]).balance == rewardPerVoter,
string.concat(
"post-run: each user should have equal share expected: ",
Strings.toString(rewardPerVoter),
" user Balance: ",
Strings.toString(votersFor[i].balance)
)
);
}
}
}
} else {
// Value should still be in the contract as the pool is still active
require(address(booth).balance == valueInETHToFundTheContract, "2");
}
// Contract balance should be zero at the end of the fuzz test
require(
address(booth).balance == 0,
string.concat("post-run: contract invalid final balance", Strings.toString(address(booth).balance))
);
}

This fuzz test is divided into three parts:

  • setUpFuzz: It takes the number of users and the random ETH value for deploying the contract. We use bound and assume to ensure compliance with contract deployment constraints.

function testFuzz_BalanceFocus(uint256 numberOfUser, uint160 valueInETHToFundTheContract) public {
numberOfUser = bound(numberOfUser, 3, 9);
vm.assume(numberOfUser % 2 != 0);
vm.assume(valueInETHToFundTheContract >= 1e18);
deal(address(this), valueInETHToFundTheContract);
setUpFuzz(numberOfUser, valueInETHToFundTheContract);

Then we generate random user address based on the amount sent in the contract for each run

for (uint256 i; i < numberOfUser; i++) {
voters.push(makeAddr(vm.toString(valueInETHToFundTheContract + i)));
}

  • userVote: Sets the user vote to true or false based on the random user address, ensuring random vote values.

// Generate random vote value based on user address modulo
bool voteValue = uint256(uint160(voters[i])) % 2 == 0;

The vote is submitted, and for each loop, we verify that the QUORUM is reached, breaking if so. Additional requirements are added to catch potential Quorum issues.

if (!booth.isActive()) {
require(totalVoter * 100 / totalAllowedVoter >= MIN_QUORUM, "quorum: should be reach");
break;
} else {
require(totalVoter * 100 / totalAllowedVoter < MIN_QUORUM, "quorum: should not be reach");
}

  • Then our main Logic is executed to catch the logic issue in the main function testFuzz_BalanceFocus

As we can see in this test we have 7 users, the vote result is the following:

Traces:
[1293211] VotingBoothTest::testFuzz_BalanceFocus(4467094641387972160 [4.467e18], 1000000000000000081 [1e18])
├─ [0] console::log(Bound Result, 7) [staticcall]
​
├─ [0] VM::startPrank(1000000000000000081: [0x04aeFE65e75021Be0bcBEa13d9E8e0c64D380FA2])
├─ [67691] VotingBooth::vote(true)
├─ [0] VM::startPrank(1000000000000000082: [0x44A99cFEc0eD35B1DaB03318431E31ec5C42fC54])
├─ [23891] VotingBooth::vote(true)
├─ [0] VM::startPrank(1000000000000000083: [0x70d8854B422383edA8Ca19f831EF6849C7D76FcE])
├─ [23891] VotingBooth::vote(true)
├─ [0] VM::startPrank(1000000000000000084: [0xE6174b8B66cc4E1a286c5DcC0491FBd04884E5C1])
├─ [163913] VotingBooth::vote(false)

Based on the contract spec the reward per user should have been 3.333333333333334e+17 -> 1000000000000000081/3
as we only have 3 users who vote "For". But the amount sent to each user was 2.5000000000000003e+17 -> 1000000000000000081/4

As you can see bellow the test failed in the first run

Failing tests:
Encountered 1 failing test in test/VotingBoothAudit.t.sol:VotingBoothTest
[FAIL. Reason: revert: post-run: each user should have equal share expected: 333333333333333360 user Balance: 250000000000000020 Counterexample: calldata=0x171113320000000000000000000000000000000000000000000000003dfe4ee89dfb26400000000000000000000000000000000000000000000000000de0b6b3a7640051, args=[4467094641387972160 [4.467e18], 1000000000000000081 [1e18]]] testFuzz_BalanceFocus(uint256,uint160) (runs: 1, μ: 1359981, ~: 1359981)

This case would also have been caught by applying the wrong logic, the test bellow is also failing showing that the contract hold funds:

-> uint256 rewardPerVoter = valueInETHToFundTheContract / (userFor + userAgainst);
// Contract balance should be zero at the end of the fuzz test
require(
address(booth).balance == 0,
string.concat("post-run: contract invalid final balance: ", Strings.toString(address(booth).balance))
);

with the following error message:

[FAIL. Reason: revert: post-run: contract invalid final balance: 32371780119278284223719909664104272519 Counterexample: calldata=0x17111332000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000490fbbf4e3719a71dcb648d9ccbef796, args=[2, 97115340357834852671159728992312817558 [9.711e37]]] testFuzz_BalanceFocus(uint256,uint160) (runs: 14, μ: 1146981, ~: 1161538)

Impact

This vulnerability has a high impact as the rewards intended for users voting "For" are significantly lower than expected, leading to lost or misallocated funds.

A malicious creator could exploit this by rigging the next voting session to retrieve the leftover funds, as a failed vote returns the total contract amount to the creator.

Tools Used

Forge stateless fuzzing

Recommendations

instead of using totalVotes use totalVotesFor it will create a new issue where the contract end up with +3 gwei in the worst case scenario but at least each user will get the same reward minus 1 wei for the last user

- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+_ rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, Math.Rounding.Ceil);

And update the last use reward

-rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotesFor, Math.Rounding.Ceil);

These changes enhance readability and ensure grammatical accuracy, maintaining the integrity of your technical analysis.

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.