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

Dust can still remain after _distributeRewards()

t0x1c

Summary

_distributeRewards() attempts to get rid of any dust amount in the contract by awarding 1 wei (rounding up) to the last for voter. This however, is not a sufficient check and can still cause dust to remain, because it could be more than 1 wei.

This dust is then stuck & not redeemable in any way since the contract lacks any direct withdrawal or transfer or selfDestruct function.

VotingBooth.sol#L203-L208

// 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);
}

Vulnerability Details

The protocol incorrectly assumes that rounding up the reward amount for the last for voter is sufficient to get rid of a dust amount which would always be 1 wei. This is not the case for various combinations of totalRewards and totalVotes. Consider the following examples:

Example 1 (exactly like testVotePassesAndMoneyIsSent() but with ETH_REWARD as 20e18:

We'll follow the exact steps in the pre-existing test testVotePassesAndMoneyIsSent(), with just one difference: Change the reward amount or ETH_REWARD from 10e18 to 20e18 -

  • Suppose s_totalAllowedVoters = 5

  • Votes needed to reach quorum = 3 votes (Since )

  • Let us have totalRewards = 20 ether ( = 20e18 )

  • 3 voters vote, all of them for, and then rewards are distributed, i.e. totalVotes = 3 and also totalVotesFor = 3. Let's call this the divisor.

  • First 2 voters get , where i.e. as per L192

  • voter gets (6666666666666666666 + 1) wei = 6666666666666666667 wei as per L207

  • 1 wei of dust now stuck in the contract ( wei).

Example 2

  • Suppose s_totalAllowedVoters = 9

  • Votes needed to reach quorum = 5 votes (Since )

  • Let us have totalRewards = 1 ether + 4 wei

  • 5 voters vote, all of them for, and then rewards are distributed, i.e. totalVotes = 5 and also totalVotesFor = 5. Let's call this the divisor.

  • First 4 voters get , where i.e. as per L192

  • voter gets (0.2 ether + 1 wei) as per L207

  • 3 wei of dust now stuck in the contract.

The above problem will occur for all situations where leaves a remainder . The maximum amount of dust which can be stuck is 3 wei, for which divisor needs to be 5 and totalRewards should be , where is some integer such that i.e. .

PoC-1

Make the following change in test/VotingBoothTest.t.sol and run forge test --mt testVotePassesAndMoneyIsSent -vv. The assertion inside the pre-existing test testVotePassesAndMoneyIsSent() will now fail because 1 wei of dust still remains after distributing rewards. This is because wei, not 1 wei.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import {VotingBooth} from "../src/VotingBooth.sol";
import {Test} from "forge-std/Test.sol";
import {_CheatCodes} from "./mocks/CheatCodes.t.sol";
​
contract VotingBoothTest is Test {
// eth reward
- uint256 constant ETH_REWARD = 10e18;
+ uint256 constant ETH_REWARD = 20e18;
​
// allowed voters
address[] voters;
​
// contracts required for test
VotingBooth booth;
​
_CheatCodes cheatCodes = _CheatCodes(HEVM_ADDRESS);
​
function setUp() public virtual {
// deal this contract the proposal reward
........
........

PoC-2 (Fuzz style)

We can use fuzz testing to let foundry figure out values which will break the invariant of "no dust remains in the contract". We'll design a fuzz test which tests different combinations of the following params:

  • totalRewardEth

  • numberOfVoters (total voters elgible to vote)

Save the following code inside a new file test/VotingBoothTestFuzzer.t.sol and run via forge test --mt testFuzz_DustRemains -vv.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import {VotingBooth} from "../src/VotingBooth.sol";
import {Test} from "forge-std/Test.sol";
​
contract VotingBoothTestFuzzer is Test {
uint256 constant MIN_QUORUM = 51;
// allowed voters
address[] voters;
​
// contracts required for test
VotingBooth booth;
​
receive() external payable {}
​
function testFuzz_DustRemains(uint256 totalRewardEth, uint256 numberOfVoters) public {
// @audit : make sure fuzzed inputs are within allowed limits, i.e. greater than 1 ether
totalRewardEth = bound(totalRewardEth, 1e18, type(uint256).max);
numberOfVoters = bound(numberOfVoters, 3, 9);
vm.assume(numberOfVoters % 2 == 1); // @audit : only odd values allowed
​
deal(address(this), totalRewardEth);
​
// setup the allowed list of voters
for (uint256 n = 1; n <= numberOfVoters; ++n) {
voters.push(address(uint160(n)));
}
​
// setup contract to be tested
booth = new VotingBooth{value: totalRewardEth}(voters);
​
// verify setup
//
// proposal has rewards
assert(address(booth).balance == totalRewardEth);
// 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));
​
for (uint256 i = 1; i <= numberOfVoters; ++i) {
vm.prank(voters[i - 1]);
booth.vote(true);
if (i * 100 / numberOfVoters >= MIN_QUORUM) {
break;
}
}
​
assertEq(booth.isActive(), false, "still active!");
assertEq(address(booth).balance, 0, "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=0xfe77e5cd0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000006d43890c043137684dc1 args=[2, 515983753758422214397377 [5.159e23]]] testFuzz_DustRemains(uint256,uint256) (runs: 31, μ: 842038, ~: 774867)
Logs:
Bound Result 1000000000000000002
Bound Result 7
Error: not zero!!
Error: a == b not satisfied [uint]
Left: 1
Right: 0

Output of run:

[FAIL. Reason: assertion failed; counterexample: calldata=0xfe77e5cd00000000000000000000000000000000541adc2e1132db54064cadd10627d7b1ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff args=[111794617144185845072098160713094649777 [1.117e38], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]]] testFuzz_DustRemains(uint256,uint256) (runs: 128, μ: 1120252, ~: 1071624)
Logs:
Bound Result 111794617144185845072098160713094649777
Bound Result 9
Error: not zero!!
Error: a == b not satisfied [uint]
Left: 1
Right: 0

Impact

Undistributed dust remains in the contract even after rewards have been distributed to the for voters.


Submitting this as Medium impact because it can be seen that the protocol has taken clear steps in L206-208 to ensure that no dust remains in the contract, but the logic fails to achieve the same.

Tools Used

Foundry, manual inspection.

Recommendations

Make the following change. This will result in the last for voter receiving their rewards + all the dust amount:

VotingBooth.sol#L206-L208

if (i == totalVotesFor - 1) {
- rewardPerVoter = Math.mulDiv(totalRewards, 1, totalVotes, Math.Rounding.Ceil);
+ rewardPerVoter = address(this).balance;
}

NOTE: The above fix is a partial fix due to another bug which exists in the contract (raised separately) titled: Incorrect calculation for rewardPerVoter, where we see that the rewardPerVoter in L192 should be calculated by dividing with totalVotesFor instead of totalVotes. Once that is fixed, the above recommendation works well. If the above recommendation is applied without that fix, then the last for voter could be benefitted unfairly and would get rewards much higher than the others (This will happen in cases where totalVotesFor totalVotes). Hence, this recommendation needs to be seen in conjunction with that fix.



It is also to be noted that this particular issue exists independently even after that fix (to bug Incorrect calculation for rewardPerVoter) has been applied, so can't be ignored.

Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

VotingBooth._distributeRewards(): Dust amount can still remain in contract

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.