Relevant GitHub Links
https://github.com/Cyfrin/2024-09-president-elector/blob/main/src/RankedChoice.sol#L98C14-L98C39
Summary
The selectPresident does not check if the candidate with most votes exceeds 50% of total number of all votes which leads to incorrect selection of the president.
Vulnerability Details
In the Ranking Choice Voting system, the winning candidate not only needs to get most votes but also his votes must exceed the 50% of total number of votes in order to be selected as the winner.
The current implementation of the _selectPresidentRecursive function which is called via the selectPresident function does not check for the 50% passing criteria. This leads to inaccurate and wrong selection of the winning candidate as the president if the candidate just has more votes than other candidates without passing the 50% percent of total number of votes.
In addition, the current logic of the _selectPresidentRecursive is not the best approach to use. The functions uses recursive call to eliminate the candidate with the lowest number of votes till it settles on the winner, however the recommended approach is to check in each round for the candidate with the most votes and also passing 50% of total number of votes.
Impact
Wrong selection of winning president candidate due to missing critical winning criteria of RCV system for the winning candidate to have most votes and be more than 50% of total number of votes. This breaks the whole purpose of the smart contract of selecting the winning president.
Proof of Concept
To demonstrate the issue, add the following test in RankedChoiceTest.t.sol, and execute the following
forge test --mt testSelectPresidentWithoutPassingWinningThreshold -vv
The below unit test demonstrates simple voting for three candidates where first two got 10 votes as first choice from voters while the third candidate has 11 votes. The third candidate will be marked as the winner while his total votes (11) did not exceed 50% of total number of votes (31/2 = 15) per this elimination round.
Note: Only first choice votes are presented to simplify the demonstration of the issue.
Unit Test:
function testSelectPresidentWithoutPassingWinningThreshold() public {
assert(rankedChoice.getCurrentPresident() != candidates[0]);
console.log("Candidate[0] Address", candidates[0]);
console.log("Candidate[1] Address", candidates[1]);
console.log("Candidate[2] Address", candidates[2]);
console.log(
"###########################################################################################"
);
console.log("Adding 10 votes for candidate[0]: ", candidates[0]);
orderedCandidates = [candidates[0]];
uint256 startingIndex = 0;
uint256 endingIndex = 10;
for (uint256 i = startingIndex; i < endingIndex; i++) {
vm.prank(voters[i]);
rankedChoice.rankCandidates(orderedCandidates);
}
console.log("Adding 10 votes for candidate[1]: ", candidates[1]);
startingIndex = endingIndex + 1;
endingIndex = endingIndex + 11;
orderedCandidates = [candidates[1]];
for (uint256 i = startingIndex; i < endingIndex; i++) {
vm.prank(voters[i]);
rankedChoice.rankCandidates(orderedCandidates);
}
console.log("Adding 11 votes for candidate[2]: ", candidates[2]);
startingIndex = endingIndex + 1;
endingIndex = endingIndex + 12;
orderedCandidates = [candidates[2]];
for (uint256 i = startingIndex; i < endingIndex; i++) {
vm.prank(voters[i]);
rankedChoice.rankCandidates(orderedCandidates);
}
vm.warp(block.timestamp + rankedChoice.getDuration());
console.log(
"###########################################################################################"
);
rankedChoice.selectPresident();
console.log("Winner: ", rankedChoice.getCurrentPresident());
}
Output:
Ran 1 test for test/RankedChoiceTest.t.sol:RankedChoiceTest
[PASS] testSelectPresidentWithoutPassingWinningThreshold() (gas: 3187402)
Logs:
Candidate[0] Address 0x00000000000000000000000000000000000000C8
Candidate[1] Address 0x00000000000000000000000000000000000000C9
Candidate[2] Address 0x00000000000000000000000000000000000000ca
###########################################################################################
Adding 10 votes for candidate[0]: 0x00000000000000000000000000000000000000C8
Adding 10 votes for candidate[1]: 0x00000000000000000000000000000000000000C9
Adding 11 votes for candidate[2]: 0x00000000000000000000000000000000000000ca
###########################################################################################
Winner: 0x00000000000000000000000000000000000000ca
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.08ms (3.30ms CPU time)
Ran 1 test suite in 6.19ms (4.08ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Tools Used
Manual Code Review and Foundry
Recommendations
Example of adding 50% threshold check in the "_selectPresident" function is shown below. Noe that this is a PoC and a more efficient solution would need to use "Batch Processing" instead of using unbounded loops:
function _selectPresidentRecursive(
address[] memory candidateList,
uint256 roundNumber
) internal returns (address[] memory) {
if (candidateList.length == 1) {
return candidateList;
}
// NEW Variable
uint256 total_votes = 0;
// Tally up the picks
for (uint256 i = 0; i < VOTERS.length; i++) {
for (
uint256 j = 0;
j < s_rankings[VOTERS[i]][s_voteNumber].length;
j++
) {
address candidate = s_rankings[VOTERS[i]][s_voteNumber][j];
if (_isInArray(candidateList, candidate)) {
s_candidateVotesByRound[candidate][s_voteNumber][
roundNumber
] += 1;
total_votes += 1;
break;
} else {
continue;
}
}
}
+ // Get canididate with most votes for this round
+ address highestVotesCandidate = candidateList[0];
+ uint256 highestVotes = s_candidateVotesByRound[highestVotesCandidate][
+ s_voteNumber
+ ][roundNumber];
+ for (uint256 i = 1; i < candidateList.length; i++) {
+ uint256 votes = s_candidateVotesByRound[candidateList[i]][
+ s_voteNumber
+ ][roundNumber];
+ if (votes > highestVotes) {
+ highestVotes = votes;
+ highestVotesCandidate = candidateList[i];
+ }
+ }
+ // check if votes of highestVotesCandidate is larger than 50% of total votes, then return it as winner
+ if (
+ s_candidateVotesByRound[highestVotesCandidate][s_voteNumber][
+ roundNumber
+ ] > total_votes / 2
+ ) {
+ address[] memory newCandidateList = new address[]();
+ newCandidateList[0] = highestVotesCandidate;
+ return newCandidateList;
+ }
// Remove the lowest candidate or break
address fewestVotesCandidate = candidateList[0];
uint256 fewestVotes = s_candidateVotesByRound[fewestVotesCandidate][
s_voteNumber
][roundNumber];
for (uint256 i = 1; i < candidateList.length; i++) {
uint256 votes = s_candidateVotesByRound[candidateList[i]][
s_voteNumber
][roundNumber];
if (votes < fewestVotes) {
fewestVotes = votes;
fewestVotesCandidate = candidateList[i];
}
}
address[] memory newCandidateList = new address[](
candidateList.length - 1
);
bool passedCandidate = false;
for (uint256 i; i < candidateList.length; i++) {
if (passedCandidate) {
newCandidateList[i - 1] = candidateList[i];
} else if (candidateList[i] == fewestVotesCandidate) {
passedCandidate = true;
} else {
newCandidateList[i] = candidateList[i];
}
}
return _selectPresidentRecursive(newCandidateList, roundNumber + 1);
}
