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

Missing Voting Completion Check in `completeVoting` and `_distributeRewards` Functions (Potential Loss of Funds)

imth3ak

Summary

The completeVoting function does not use a modifier to check that the voting is not completed before. This could make the code less readable and reusable, as the require statement is repeated in the function body. The _distributeRewards function does not check that s_votingComplete == true before executing. This could allow anyone to call the function and trigger payouts or refunds, even if the voting is not completed.

Vulnerability Details

The completeVoting function is responsible for completing the voting process and distributing the rewards or refunds for the proposal. However, it does not use a modifier to check that the voting is not completed before. Instead, it uses a require statement in the function body, which checks that s_votingComplete == false. This could make the code less readable and reusable, as the require statement is repeated in the function body. It could also increase the gas cost of the function, as the require statement is executed every time the function is called.

The _distributeRewards function is responsible for distributing the rewards to the voters who voted for the proposal, or refunding the reward amount back to the proposal creator, depending on the proposal outcome. However, this function does not check that s_votingComplete == true before executing. This means that anyone can call the function and trigger payouts or refunds, even if the voting is not completed. This could result in a loss of funds for the proposal creator and the voters, as the contract could transfer funds based on an incorrect proposal outcome.

Impact

The impact of this issue is that the contract could lose all the funds that it holds as a reward for the voters. Anyone could exploit this issue by calling the _distributeRewards function and triggering payouts or refunds, even if the voting is not completed. This would also prevent the voting process from completing and the rewards from being distributed to the legitimate voters.

Tools Used

  • Manual

  • Foundry

Test case

The following test case demonstrates how anyone could exploit this issue:

  • Deploy the contract with an array of three allowed voters and 1 ether as the reward.

  • Call the _distributeRewards function from any address that is not the proposal creator or one of the allowed voters, before the voting is completed.

  • Observe that the contract balance is transferred to the caller and the contract is emptied.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "hardhat/console.sol";
import "./VotingBooth.sol";
​
contract VotingBoothTest {
VotingBooth votingBooth;
​
function testDistributeRewards() public {
// deploy the contract with three allowed voters and 1 ether as the reward
address[] memory allowList = new address;
allowList[0] = 0x1234567890123456789012345678901234567890;
allowList[1] = 0x2345678901234567890123456789012345678901;
allowList[2] = 0x3456789012345678901234567890123456789012;
votingBooth = new VotingBooth{value: 1 ether}(allowList);
​
// check the initial contract balance
console.log("Initial contract balance: %s wei", address(votingBooth).balance);
​
// call the _distributeRewards function from any address that is not the proposal creator or one of the allowed voters, before the voting is completed
votingBooth._distributeRewards{from: 0x4567890123456789012345678901234567890123}();
​
// check the final contract balance
console.log("Final contract balance: %s wei", address(votingBooth).balance);
​
// check the balance of the caller
console.log("Caller balance: %s wei", 0x4567890123456789012345678901234567890123.balance);
}
}
​

Logs

Initial contract balance: 1000000000000000000 wei
Final contract balance: 0 wei
Caller balance: 1000000000000000000 wei
​

Recommendations

Use a modifier to check that the voting is not completed before and apply it to the completeVoting function. This way, the code is more readable and reusable, as the modifier can be applied to other functions that need the same check. It could also reduce the gas cost of the function, as the modifier is executed only once before the function is called. You should also add a require statement in the _distributeRewards function to check that s_votingComplete == true, as this function can be called by anyone and should only be executed after the voting is completed.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "./Math.sol";
​
contract VotingBooth {
// ... omitted for brevity
​
// modifier to check that the voting is not completed
modifier onlyWhenNotCompleted() {
// require that the voting is not completed
require(!s_votingComplete, "DP: voting has already been completed");
​
// continue execution
_;
}
​
// completes the voting process and distributes the rewards or refunds
// for the proposal
function completeVoting() external onlyWhenNotCompleted { // apply the modifier
// mark voting as having been completed
s_votingComplete = true;
​
// distribute the rewards or refunds
_distributeRewards();
}
​
// distributes rewards to the `for` voters if the proposal has
// passed or refunds the rewards back to the creator if the proposal
// failed
function _distributeRewards() private {
// require that the voting is completed
require(s_votingComplete, "DP: voting is not completed"); // add this check
​
// 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;
​
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);
}
}
}
​
// ... omitted for brevity
}
​
Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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