Summary
The vulnerability is related to the calculation of rewards for voters in favor, for, of a proposal. The current implementation calculates the reward based on the total votes cast, including votes for and against, leading to an uneven distribution of funds when a proposal passes. Moreover, there is no other way to withdraw remaining funds from the contract, resulting in funds being stuck in the contract forever.
Vulnerability Details
If the contract distributes the amount to those who favor the proposal, it should be calculated based on the total votes cast in favor of the proposal. If it is calculated based on the total votes cast, then the reward is not distributed evenly.
For example:
Let's assume there are 5 voters in the proposal, and the reward is 1 ETH. Out of five, 2 voted in favor of the proposal and 1 against the proposal. When the quorum is reached, the system should distribute 0.5 ETH to those who favor the proposal. However, with the current system, rewards are distributed as follows:
Logs:
totalRewards : 10000000000000000000
totalVotesFor : 2
totalVotesAgainst : 1
Rewards for voter 0 : 3333333333333333333
Rewards for voter 1 : 3333333333333333334
balance in contract : 3333333333333333333
ā
Technically, the balance in the contract should be 0 ETH when the rewards are distributed, but here it keeps 0.33 ETH.
The vulnerability is located in the following portion of the code:
else {
ā
š uint256 rewardPerVoter = totalRewards / totalVotes;
ā
for (uint256 i; i < totalVotesFor; ++i) {
if (i == totalVotesFor - 1) {
rewardPerVoter = Math.mulDiv(
totalRewards,
1,
š totalVotes,
Math.Rounding.Ceil
);
}
}
}
Impact
The impact of this vulnerability is that when a proposal passes, the funds are not distributed evenly among voters in favor. The current calculation method, based on total votes, may result in an uneven distribution, potentially leading to unfair rewards for voters. Hence, this results in funds not being distributed and being lost forever, as there is no function to withdraw funds from the contract.
POC
Here, three voters participate in the voting process. Out of the three, two vote in favor of the proposal and one votes against it. After the proposal passes, the funds are distributed but not evenly among the voters. This results in funds that are not distributed being stuck in the contract forever.
Test case with current logic
forge test --match-test test_vote_passes_2_vs_1_and_money_is_not_sent_evenly -vvv
function test_vote_passes_2_vs_1_and_money_is_not_sent_evenly() public {
vm.prank(address(0x1));
booth.vote(true);
ā
vm.prank(address(0x2));
booth.vote(true);
ā
vm.prank(address(0x3));
booth.vote(false);
ā
assert(!booth.isActive() && address(booth).balance > 0);
console.log("Funds in the contract after rewards distribution:", address(booth).balance);
}
ā
Logs:
ā
Funds in the contract after rewards distribution: 3333333333333333333
Test case with fixed logic
forge test --match-test test_vote_passes_2_vs_1_and_money_is_sent_evenly
function test_vote_passes_2_vs_1_and_money_is_sent_evenly() public {
vm.prank(address(0x1));
booth.vote(true);
ā
vm.prank(address(0x2));
booth.vote(true);
ā
vm.prank(address(0x3));
booth.vote(false);
ā
assert(!booth.isActive() && address(booth).balance == 0);
console.log("Funds in the contract after rewards distribution:", address(booth).balance);
}
ā
Logs:
ā
Funds in the contract after rewards distribution: 0
Tools Used
The vulnerability was identified through manual code review.
Recommendations
When rewards are distributed, they should be calculated based on the number of voters who support the proposal (totalVotesFor). This will distribute the funds evenly and final balance of the contract will be 0.
function _distributeRewards() private {
// get number of voters for & against
uint256 totalVotesFor = s_votersFor.length;
uint256 totalVotesAgainst = s_votersAgainst.length;
-- uint256 totalVotes = totalVotesFor + totalVotesAgainst;
ā
// rewards to distribute or refund. This is guaranteed to be
// greater or equal to the minimum funding amount by a check
// in the constructor, and there is intentionally by design
// no way to decrease or increase this amount. Any findings
// related to not being able to increase/decrease the total
// reward amount are invalid
uint256 totalRewards = address(this).balance;
ā
// if the proposal was defeated refund reward back to the creator
// for the proposal to be successful it must have had more `For` votes
// than `Against` votes
if (totalVotesAgainst >= totalVotesFor) {
// proposal creator is trusted to create a proposal from an address
// that can receive ETH. See comment before declaration of `s_creator`
_sendEth(s_creator, totalRewards);
}
// otherwise the proposal passed so distribute rewards to the `For` voters
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);
}
}
}
ā
