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

On calculating the `rewardPerVoter` we divide by the `totalVotes` instead of the `totalVotesFor` which means that the `rewardPerVoter` is lower than expected and some amount is locked

naman1729

Summary

  • On calculating the reward per voter we divide by the total number of votes instead of the total number of For voters which means that the reward per voter is lower than expected and some amount of money is left in the contract after the voting is complete.

Vulnerability Details

  • Here, we dividing the totalRewards by the totalVotes instead of the totalVotesFor. This means that the reward per voter is lower than expected and some amount of money is left in the contract after the voting is complete.

else {
@> uint256 rewardPerVoter = totalRewards / totalVotes;
​
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);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}

POC

  • Here, we see that after the completion of the voting the money is still locked in the VotingBooth. By the console.log, we can see in the teminal that locked money = 3333333333333333333. which means that the division of the totalRewards is must be wrong (i.e. 10e18 / 3 = 3333333333333333333) instead of 10e18 / 2 = 5000000000000000000. here we change the division part in the VotingBooth.sol to 10e18 / 2, (i.e. uint256 rewardPerVoter = totalRewards / totalVotes;) to (uint256 rewardPerVoter = totalRewards / totalVotesFor;)

  • Here, we see that the money is sent to the voter is lower than expected. By the console.log, we can see in the teminal that the money sent to the voter is 3333333333333333333 instead of 5000000000000000000.

function testIsVoterGetCorrectAmount() public {
uint256 startingAmount = address(booth).balance;
console.log("startingAmount: ", startingAmount);
​
vm.prank(address(0x1));
booth.vote(true);
​
vm.prank(address(0x2));
booth.vote(true);
​
vm.prank(address(0x3));
booth.vote(false);
​
// this shows that voting is complete
assert(!booth.isActive());
​
// this shows that the money is still locked in the VotingBooth after voting completion. By the console.log, we can see
// in the teminal that locked money = 3333333333333333333. which means that the division of the totalRewards is must be
// wrong (i.e. 10e18 / 3 = 3333333333333333333) instead of 10e18 / 2 = 5000000000000000000. here we change the division part
// in the VotingBooth.sol to 10e18 / 2, (i.e. uint256 rewardPerVoter = totalRewards / totalVotes;) to
// (uint256 rewardPerVoter = totalRewards / totalVotesFor;)
assert(!(address(booth).balance == 0));
console.log("address(booth).balance: ", address(booth).balance);
​
// this shows that the money is sent to the voter is lower than expected. By the console.log, we can see
// in the teminal that the money sent to the voter is 3333333333333333333 instead of 5000000000000000000.
assertEq(address(0x1).balance, startingAmount / 3);
console.log("address(0x1).balance: ", address(0x1).balance);
}

  • Run this command to test the POC

forge test --mt testIsVoterGetCorrectAmount -vv

  • Terminal output

[⠒] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/VotingBoothTest.t.sol:VotingBoothTest
[PASS] testIsVoterGetCorrectAmount() (gas: 260932)
Logs:
startingAmount: 10000000000000000000
address(booth).balance: 3333333333333333333
address(0x1).balance: 3333333333333333333
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.78ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

  • Stateless fuzzing

function testIsMoneyLockedAfterVote(bool voterss) public {
vm.prank(address(0x1));
booth.vote(voterss);
​
vm.prank(address(0x2));
booth.vote(voterss);
​
vm.prank(address(0x3));
booth.vote(!voterss);
​
assert(!booth.isActive());
assertEq(address(booth).balance, 0);
}

  • Terminal output of Stateless fuzzing

[⠒] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/VotingBoothTest.t.sol:VotingBoothTest
[FAIL. Reason: assertion failed; counterexample: calldata=0x400235fa0000000000000000000000000000000000000000000000000000000000000001 args=[true]] testIsMoneyLockedAfterVote(bool) (runs: 4, μ: 196379, ~: 196379)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.12ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/VotingBoothTest.t.sol:VotingBoothTest
[FAIL. Reason: assertion failed; counterexample: calldata=0x400235fa0000000000000000000000000000000000000000000000000000000000000001 args=[true]] testIsMoneyLockedAfterVote(bool) (runs: 4, μ: 196379, ~: 196379)
​
Encountered a total of 1 failing tests, 0 tests succeeded

Impact

  • reward given to the voter is lower than expected.

  • Some amount of money is left in the contract after the voting is complete.

Tools Used

  • Manual Review

  • foundry testing

Recommendations

  • On replacing totalVotes to totalVotesFor in the division part of the VotingBooth.sol will fix the issue of lower rewardPerVoter and also resolve the locked money in contract issue.

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);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}
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.