President Elector
First Flight #24
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Unbounded For Loop in `_isInArray` when looping through voters at `_rankCandidates()` function may cause the transaction to fail

whizz1

Summary

The function isInArray when used at _rankCandidates(), function contains an unbounded for loop that can cause transactions to revert due to exceeding gas limits .
https://github.com/Cyfrin/2024-09-president-elector/blob/fccb8e2b6a32404b4664fa001faa334f258b4947/src/RankedChoice.sol#L167

Vulnerability Details

The current implementation of _isInArray iterates over the entire array of voters to check if a given address is in the array. If the array size is very large (since there is no limit set for the number of voters), the loop may exceed the gas limits, causing the transaction to revert. This prevents the last voters from casting their votes and could disrupt the voting process.

Impact

Transaction Failure: Transactions can fail if the loop runs out of gas, particularly affecting voters located towards the end of the array.
Disruption in Voting Process: Valid voters may be unable to cast their votes if the function reverts due to gas limits.

Tools Used

Manual code review + Foundry.

POC

If we had 30,000 voters, the transaction will revert at set Up due to running out of gas. Additionally, voters at index 25000 onwards would not be able to rank candidates
Add this to your test suite:

contract RankedChoiceTest is Test {
address[] voters;
address[] candidates;
​
uint256 constant MAX_VOTERS = 30000;
uint256 constant MAX_CANDIDATES = 4;
uint256 constant VOTERS_ADDRESS_MODIFIER = 100;
uint256 constant CANDIDATES_ADDRESS_MODIFIER = 200;
​
RankedChoice rankedChoice;
​
address[] orderedCandidates;
​
function setUp() public {
for (uint256 i = 0; i < MAX_VOTERS; i++) {
voters.push(address(uint160(i + VOTERS_ADDRESS_MODIFIER)));
}
rankedChoice = new RankedChoice(voters);
​
for (uint256 i = 0; i < MAX_CANDIDATES; i++) {
candidates.push(address(uint160(i + CANDIDATES_ADDRESS_MODIFIER)));
}
}

Recommendations

Use a Mapping: Replace the array with a mapping for voter validation, which provides O(1) lookup time and avoids gas issues associated with large arrays.

Set a Reasonable Maximum Voter Limits : Implement a maximum length for the voter array (eg.9000 voters) and check this limit in the constructor to prevent excessive sizes.

Update Code: Modify the constructor and _rankCandidates() function to use a mapping instead of an array for voter validation. Here’s the code:

+ mapping(address => bool) private isVoter;
+ uint256 private constant MAX_VOTERS = 9000;
​
​
constructor(address[] memory voters) EIP712("RankedChoice", "1") {
@>++ uint256 votersLength = voters.length;
+ require(
+ votersLength > 0 && votersLength <= MAX_VOTERS,
+ "Invalid voter number"
+ );
+ for (uint256 i = 0; i < votersLength; i++) {
+ require(voters[i] != address(0), "Voter cannot be zero address");
+ require(!isVoter[voters[i]], "Address is already a registered voter");
+ isVoter[voters[i]] = true;
+ VOTERS.push(voters[i]);
}
- VOTERS = voters;
i_presidentalDuration = 1460 days;
s_currentPresident = msg.sender;
s_voteNumber = 0;}
function _rankCandidates(
address[] memory orderedCandidates,
address voter
) internal {
// Checks
if (orderedCandidates.length > MAX_CANDIDATES) {
revert RankedChoice__InvalidInput();
}
- if (!_isInArray(VOTERS, voter)) {
+ if(!isVoter[voter]){
revert RankedChoice__InvalidVoter();
​
} ";
}
s_rankings[voter][s_voteNumber] = orderedCandidates;
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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