Summary
Due to flawed calculations in _distributeRewards function, Maximum of 33% ETH could stuck forever in contract.
Vulnerability Details
In _distributeRewards function, users who have voted for for will be awarded with eth if proposal is passed. If proposal is not passed then all eth is sent back to the s_creator.
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);
}
}
}
Check the highlighted code. There is flaw in calculation where rewardPerVoter is calculated, here rewardPerVoter means users who have voted for for.
As per current logic of the contract, it divides the available eth with number of total votes. Which reduce the voter share, as it is including the voters which has voted for 'against` as well.
Consider the following example, there are 5 allowed users and contract is initialized with 10 ether. first 2 votes true(for) and 3rd one votes false(against). Now proposal is passed, as per current contract logic, it will calculate reward share as 10 / 3 = 3.33 eth. so user who voted for for, will get 3.33 eth each and 3.33 eth will remain in the contract. There is no way to take that eth out.
This will lead to loss of funds and voters dissatisfaction (as they are receiving less rewards than it supposed to be).
POC
Replace the import like this in existing test suite
- import {Test} from "forge-std/Test.sol";
+ import {Test, console2} from "forge-std/Test.sol";
then add the following function
function testUsersWillReceiveLessEthAndExtraFundStuckInContractForever() public {
console2.log("VotingBooth balance before:", address(booth).balance);
console2.log("user1 balance before:", address(0x1).balance);
vm.prank(address(0x1));
booth.vote(true);
console2.log("user2 balance before:", address(0x2).balance);
vm.prank(address(0x2));
booth.vote(true);
vm.prank(address(0x3));
booth.vote(false);
console2.log("user1 balance after:", address(0x1).balance);
console2.log("user2 balance after:", address(0x2).balance);
console2.log("VotingBooth balance after:", address(booth).balance);
}
now run forge test --mt testUsersWillReceiveLessEthAndExtraFundStuckInContractForever -vvvcommand in your terminal to see the result.
Result will be shown like below:
[⠢] Compiling...
[⠒] Compiling 1 files with 0.8.23
[⠑] Solc 0.8.23 finished in 1.31s
Compiler run successful!
Running 1 test for test/VotingBoothTest.t.sol:VotingBoothTest
[PASS] testUsersWillReceiveLessEthAndExtraFundStuckInContractForever() (gas: 262272)
Logs:
VotingBooth balance before: 10000000000000000000
user1 balance before: 0
user2 balance before: 0
user1 balance after: 3333333333333333333
user2 balance after: 3333333333333333334
VotingBooth balance after: 3333333333333333333
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.42ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Loss of funds, due to eth stuck in contract and user dissatisfaction.
Tools Used
Manual Review, Foundry
Recommendations
Remove the extra code and implement it in following way.
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);
}
_sendEth(s_votersFor[i], rewardPerVoter);
}
}
}
