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

Logical error or Business logic flaw, in VotingBooth::_distributeRewards, resulting in unfair reward distribution

azmaeengh

Summary

According to the contract, the eth reward is provided to the for voters if the proposal passes.
Meaning that if reward pool is 10 ether, and there are 5 participants and 3 voted for for. Then those 10 ether should be distributed among those 3 voters, by calculating something like 10/3 = 3.33 (just an example). And not 10/5, just because 5 participants were there. And this also creates a possibility of the entire reward pool not being properly distributed among the participates/voters.

Thats the issue that is found in this contract, where the price distribution is calculated by :

// wrong way -
rewardPerVoter = totalRewards / all Votes - both For-Votes and against-Votes
// correct way -
rewardPerVoter = totalRewards / only For-Votes

And a Rounding error occurs, in the for-loop of the next line to the unfair distribution code.

Vulnerability Details

This is problematic because we already have a code logic, for dividing the money, BUT this made it so the money gets divided in more shares than necessary, resulting in each FOR-VOTER getting less reward, and some reward being left in the contract.

First issue, using totalVotes instead of totalVotesFor in this line:

uint256 rewardPerVoter = totalRewards / totalVotes;

and, again the issue when using totalVotes instead of totalVotesFor in this if statement:

for (uint256 i; i < totalVotesFor; ++i) {
if (i == totalVotesFor - 1) {
rewardPerVoter = Math.mulDiv(
totalRewards,
1,
@> totalVotes,
Math.Rounding.Ceil
);
}

In a properly implemented code, this section is not a vulnerability/bug and will distribute the totalRewards accurately. (with the last voter getting, something like 1 wei, more than the rest)
But, that not the case here.

  • POC CODE: Add the following code snippet to the test suite

Click to Expand

Add this two imports to test file:

import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {console} from "forge-std/console.sol";

Test Function:

function testDistributeRewardsFaulty() public {
uint8 totalVotesFor = 3;
uint8 totalVotesAgainst = 2;
// The total number of votes is the sum of 'for' and 'against' votes.
uint256 totalVotes = totalVotesFor + totalVotesAgainst;
uint256 totalRewards = ETH_REWARD;
console.log("totalRewards: ", totalRewards);
uint256 initialBalance = address(booth).balance;
console.log("Initial Balance: ", initialBalance);
​
//////////
/// Casting For and Against Votes, and calculating reward per For-Voter:
//////////
​
// Cast 'against' votes.
for (
uint256 i = totalVotesFor;
i < totalVotesFor + totalVotesAgainst;
i++
) {
if (!booth.isActive()) {
break; // Stop voting if the voting period has ended
}
vm.prank(voters[i]);
booth.vote(false);
}
​
// Cast 'for' votes.
for (uint256 i = 0; i < totalVotesFor; i++) {
if (!booth.isActive()) {
break; // Stop voting if the voting period has ended
}
vm.prank(voters[i]);
booth.vote(true);
}
​
// Calculate the reward per voter based on the total number of votes(for+against). Wrong logic.
uint256 rewardPerVoter = totalRewards / totalVotes;
console.log("rewardPerVoter: ", rewardPerVoter);
​
//////////
/// Distributing reward:
//////////
​
for (uint256 i; i < totalVotesFor; ++i) {
if (i == totalVotesFor - 1) {
rewardPerVoter = Math.mulDiv(
totalRewards,
1,
totalVotes,
Math.Rounding.Ceil
);
}
_sendEth(voters[i], rewardPerVoter);
}
​
//////////
/// All FOR-VOTER balances, after distributing the reward:
//////////
​
// 1st
address firstForVoter = voters[totalVotesFor - 3];
uint256 firstForVoterBalance = firstForVoter.balance;
console.log("firstForVoterBalance: ", firstForVoterBalance);
​
// 2nd
address secondForVoter = voters[totalVotesFor - 2];
uint256 secondForVoterBalance = secondForVoter.balance;
console.log("secondForVoterBalance:", secondForVoterBalance);
​
// 3rd. Last For-Voter
address lastForVoter = voters[totalVotesFor - 1];
uint256 lastForVoterBalance = lastForVoter.balance;
console.log("lastForVoterBalance: ", lastForVoterBalance);
​
//////////
/// Check the balance of the contract to ensure it has distributed the rewards correctly:
//////////
​
uint256 remainingBalance = initialBalance -
(firstForVoterBalance +
secondForVoterBalance +
lastForVoterBalance);
console.log("Remaining Balance: ", remainingBalance);
}
​
//////////
/// Separate function needed for distributing rewards. (Copied from main .sol file)
//////////
​
// sends eth using low-level call as we don't care about returned data
function _sendEth(address dest, uint256 amount) private {
bool sendStatus;
assembly {
sendStatus := call(gas(), dest, amount, 0, 0, 0, 0)
}
require(sendStatus, "DP: failed to send eth");
}

The LOG from the above test (5 voters - 3 voted FOR, 2 voted Against):

Logs:
totalRewards: 10000000000000000000
Initial Balance: 10000000000000000000
rewardPerVoter: 2000000000000000000
firstForVoterBalance: 2000000000000000000
secondForVoterBalance: 2000000000000000000
lastForVoterBalance: 2000000000000000000
Remaining Balance: 4000000000000000000
​

All of the reward is not properly distributed.

Impact

Since the code is wrong in 2 different places, the resulting code is such that - It will distribute reward in smaller portions, and the rest of the reward will remain in the contract (assuming we had a majority of FOR-VOTES).

This undermines the integrity of the voting reward system and can lead to voter dissatisfaction.
This also leaves the contract vulnerable and could be exploited if an attacker finds a way retrieve the undistributed reward.

Tools Used

Manual review, AI.

Recommendations

Modify the following lines of code, to make the calculations go as per intended:

- uint256 rewardPerVoter = totalRewards / totalVotes;
+ uint256 rewardPerVoter = totalRewards / totalVotesFor ;
for (uint256 i; i < totalVotesFor; ++i) {
if (i == totalVotesFor - 1) {
rewardPerVoter = Math.mulDiv(
totalRewards,
1,
- totalVotes,
+ totalVotesFor,
Math.Rounding.Ceil
);
}
_sendEth(voters[i], rewardPerVoter);
}

After implenting this two changes, the code will now work as per the intended logic. And it will distribute rewards to all the FOR-VOTERs, equally (with the last voter sometimes getting slighty more (ex- 1wei) because of overall rounding of numbers).

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.