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

Reward per voter calculated by `VotingBooth::_distributeRewards()` also considers the `against` voters leading to stuck eth and reduced payout to `for` voters

shikhar229169

Summary

The VotingBooth contract pays the eth reward to for voters if the proposal was passed, but while calculating rewardPerVoter it also considers the against voters and as the protocol pays eth reward only to the for voters, the remaining amount of eth will get stuck in the VotingBooth contract as the eth reward was reduced by also considering against voters.

As the protocol only pays to the for voters on successful proposal, so the intended implementation should be to calculate reward by only considering for voters and not against voters.

Vulnerability Details

The vulnerability lies inside the VotingBooth contract inside the _distributeRewards function at line 192 and 207 while calculating the rewards that is to be distributed to each for voter.

The function is intended to send eth only to the for voters (when the proposal is successful), but while calculating the rewardPerVoter it also considers the against voters as a result of which the eth to be distributed to each for voter is reduced by against voters and for voters will receive less amount of eth.

The total eth reward in the contract is divided by totalVotes which is a sum of both for voters and against voters, thus reducing the eth amount that is to be sent to the for voters and in the for loop eth is distributed to for voters (which is intended) but as a result of this there will be eth remaining in the contract and it will get permanently stuck in there and there is no way to retrieve it.

function _distributeRewards() private {
uint256 totalVotesFor = s_votersFor.length;
uint256 totalVotesAgainst = s_votersAgainst.length;
uint256 totalVotes = totalVotesFor + totalVotesAgainst;
​
uint256 totalRewards = address(this).balance;
​
if (totalVotesAgainst >= totalVotesFor) {
_sendEth(s_creator, totalRewards);
}
else {
@> uint256 rewardPerVoter = totalRewards / totalVotes;
​
for (uint256 i; i < totalVotesFor; ++i) {
if (i == totalVotesFor - 1) {
@> rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}
}

Impact

  • Due to wrong implemented _distributeRewards function, it calculates reward to be distributed to for voters also considering the against voters, leading to reduced amount of eth sent to for voters. Thus it will have a impact on the eth amount to be distributed to for voters and it will be less than the expected amount that is to be sent.

  • Also, as the reward per voter amount is reduced, there will be eth remaining in the VotingBooth contract and it will get permanently stuck in there and there is no way to rescue the remaining eth amount.

PoC

Create a new file VotingBoothInvariantTest.t.sol inside the Test folder and add the below test contract inside it.

Run the test:

forge test --mt invariant_voting -vvvv
// SPDX-License-Identifier: MIT
​
pragma solidity ^0.8.23;
​
import { VotingBooth } from "../src/VotingBooth.sol";
import { Test, console } from "forge-std/Test.sol";
import { StdInvariant } from "forge-std/StdInvariant.sol";
​
contract VotingBoothInvariantTest is StdInvariant {
VotingBooth votingBooth;
uint256 initBalance = 1 ether;
​
function setUp() external {
address[] memory allowList = new address[](9);
for (uint160 i = 1; i < 10; i++) {
allowList[i - 1] = address(i);
targetSender(allowList[i - 1]);
}
votingBooth = new VotingBooth{value: initBalance}(allowList);
targetContract(address(votingBooth));
}
​
function invariant_voting() public view {
// if on voting, the quorum is not reached then voting is still active, and whole money should be there
// but if quorum is reached upon voting then it should be inactive and contract should distribute all the
// eth to the 'for' voters
​
if (votingBooth.isActive()) {
assert(address(votingBooth).balance == initBalance);
}
else {
console.log("Remaining Contract Balance-", address(votingBooth).balance);
assert(address(votingBooth).balance == 0);
}
}
​
// as this (VotingBoothInvariantTest) contract deploys the VotingBooth contract, therefore
// it is the creator of the VotingBooth contract, and if in case proposal fails so it will send
// eth to this contract, thus receive function is req to receive eth
receive() external payable {}
}

  • Here it says the assertion failed. The remaining contract balance is expected to be 0 after voting ends but it is 0.4 eth

+[FAIL. Reason: panic: assertion failed (0x01)]
[Sequence]
sender=0x0000000000000000000000000000000000000005 addr=[src/VotingBooth.sol:VotingBooth]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=vote(bool) args=[true]
sender=0x0000000000000000000000000000000000000003 addr=[src/VotingBooth.sol:VotingBooth]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=vote(bool) args=[true]
sender=0x0000000000000000000000000000000000000004 addr=[src/VotingBooth.sol:VotingBooth]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=vote(bool) args=[false]
sender=0x0000000000000000000000000000000000000006 addr=[src/VotingBooth.sol:VotingBooth]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=vote(bool) args=[true]
sender=0x0000000000000000000000000000000000000009 addr=[src/VotingBooth.sol:VotingBooth]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=vote(bool) args=[false]
invariant_voting() (runs: 256, calls: 3832, reverts: 2574)
Logs:
+ Remaining Contract Balance- 400000000000000000

Tools Used

Manual Review, Foundry Test, Fuzzy Cat (Meow)

Recommendations

1. Calculate the rewardPerVoter correctly by dividing the total rewards by total number of for voters.

  • For line 192

-uint256 rewardPerVoter = totalRewards / totalVotes;
+uint256 rewardPerVoter = totalRewards / totalVotesFor;

  • For line 207

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

2. If the protocol wants to reduce the rewards of the for voters by considering the against voters, then remaining balance should be sent back to the s_creator

Therefore consider sending eth to the creator inside the else statement which is at line 191 inside VotingBooth::_distributeRewards() function.
Add the below line just after the for loop inside the else statement discussed above.

_sendEth(s_creator, address(this).balance);
Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years 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.