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

Issues in President Elector contract

AniketTyagi

Immutable voters Array

[H-1] The voters array is set to the constructor and cannot be updated afterwards in RankedChoice::constructor which will make the contract unable to add new voters.

IMPACT: HIGH

LIKELIHOOD: MEDIUM

Description: The voters array in this contract is initilised in the constructor, without any function to update and modify it. which means it cannot be updated after deployment.

Impact: Because the voters is only set in the constructor, Only the deployer has full control over the voters list. ths could centralise so much control in one entity leading to governance issue.

If the private key of a voter is compromised they'll be no way to revoke thier voting rights.
This will probably lead to unauthorised voting or manipulation of election process.

The contract will not be able to add or remove voters.
The voting pool is permanetly fixed at contract deployment.

Proof of Concept:

POC place the following Test into `RankedChoiceTest.t.sol`.
function testCannotUpdateVotersList()public {
address[] memory candidate = new address[]();
candidates[0] = voter1;
​
vm.prank(voter2);
rankedChoice.rankCandidates(candidates);
​
vm.prank(newVoter);
vm.expectRevert(abi.encodeWithSignature("RankedChoice__InvalidVoter()"));
rankedChoice.rankCandidates(candidates);
​
}
​

Recomended Mitigation: create functions that will add and also remove voters, and will also restrict s_currentPresident and multi-sig

make contract upgradable, allowing future improvement for voters management.

[H-2] selectPresident() Vulnerability Review.

IMPACT: HIGH

LIKELIHOOD: HIGH

Description: The selectPresident() has no restrictions, leaving it vulnerable to be called by any address at any given time.

Impact: An attacker could call the function selectPresident() as soon as the voting period ends and even before all voters will have the chance to submit thier rankings.

Without giving a fixed time or a specific caller the voters will never know or predict when the president will be elected.

Proof Of concept:

POC Here, you'll have to create a mock folder then a mockfile and paste the following code in it. Reasons been that, you'll get an array out of bound error whenyou run it directly from the test bcos the function assumes ther's at least one candidate, but if no votes or candiddate have been added,it will cause an out-of-bound error when trying to access the first element of an empty array. The Mock contract mimics the behavior of the `selectPresident()` function.
Immutable voters Array
### [H-1] The voters array is set to the constructor and cannot be updated afterwards in `RankedChoice::constructor` which will make the contract unable to add new voters.
​
IMPACT: HIGH
​
LIKELIHOOD: MEDIUM
​
​
**Description:** The `voters` array in this contract is initilised in the constructor, without any function to update and modify it. which means it cannot be updated after deployment.
​
**Impact:** Because the `voters` is only set in the constructor, Only the deployer has full control over the voters list. ths could centralise so much control in one entity leading to governance issue.
​
If the private key of a voter is compromised they'll be no way to revoke thier voting rights.
This will probably lead to unauthorised voting or manipulation of election process.
​
The contract will not be able to add or remove voters.
The voting pool is permanetly fixed at contract deployment.
​
**Proof of Concept:**
​
<details>
<summary>POC</summary>
place the following Test into `RankedChoiceTest.t.sol`.
​
```javascript
function testCannotUpdateVotersList()public {
address[] memory candidate = new address[]();
candidates[0] = voter1;
​
vm.prank(voter2);
rankedChoice.rankCandidates(candidates);
​
vm.prank(newVoter);
vm.expectRevert(abi.encodeWithSignature("RankedChoice__InvalidVoter()"));
rankedChoice.rankCandidates(candidates);
​
}
​
```
</details>
​
​
**Recomended Mitigation:** create functions that will add and also remove voters, and will also restrict `s_currentPresident` and multi-sig
​
make contract upgradable, allowing future improvement for voters management.
​
​
### [H-2] `selectPresident()` Vulnerability Review.
​
IMPACT: HIGH
​
LIKELIHOOD: HIGH
​
​
**Description:** The `selectPresident()` has no restrictions, leaving it vulnerable to be called by any address at any given time.
​
**Impact:** An attacker could call the function `selectPresident()` as soon as the voting period ends and even before all voters will have the chance to submit thier rankings.
​
Without giving a fixed time or a specific caller the voters will never know or predict when the president will be elected.
​
**Proof Of concept:**
​
<details>
<summary>POC</summary>
Here, you'll have to create a mock folder then a mockfile and paste the following code in it.
Reasons been that, you'll get an array out of bound error whenyou run it directly from the test bcos the function assumes ther's at least one candidate, but if no votes or candiddate have been added,it will cause an out-of-bound error when trying to access the first element of an empty array. The Mock contract mimics the behavior of the `selectPresident()` function.
​
```javascript
​
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
​
import "forge-std/Test.sol";
​
​
contract MockRankedChoice {
uint256 public constant PRESIDENTIAL_DURATION = 1460 days;
uint256 public lastElectionTimestamp;
​
constructor() {
lastElectionTimestamp = block.timestamp;
}
​
function selectPresident() external {
require(block.timestamp > lastElectionTimestamp + PRESIDENTIAL_DURATION, "Not time to vote");
lastElectionTimestamp = block.timestamp;
}
}
​
contract RankedChoiceTest is Test{
​
MockRankedChoice public mockRankedChoice;
​
address public attacker;
​
function setUp() public {
attacker = address(0xBEEF);
​
mockRankedChoice = new MockRankedChoice();
}
​
function testSelectPresidentVulnerability() public {
vm.warp(block.timestamp + mockRankedChoice.PRESIDENTIAL_DURATION() + 1 days);
vm.prank(attacker);
mockRankedChoice.selectPresident();
assertTrue(true, "Attacker was able to call selectPresident");
}
}
​
​
```
</details>
​
​
​
**Recomended Mitigation:** Note the function has a time check
```dif
if (
block.timestamp - s_previousVoteEndTimeStamp <=
i_presidentalDuration
) {
revert RankedChoice__NotTimeToVote();
}
```
but this only prevents the function from been called too early and not controlling it from beign called at the wrong time.
​
Implement a system where only an address or msg.sender as the case maybe will be able to call the function.
​
Make the selection of the president a formal governance proposal that must be voted on before execution.
​
Modify a function to be callable only at a specific time after the voting period ends.
​
​
​
### [H-#3]Multiple entrants in`SelectPresident()`and `_selectPresidentRecursive` doing too many loops in same function(looping through voters, looping through candidates and selecting a president)leaves a function vulnerable to Denial Of Service Attack(DOS).
​
**Description:** The current Implementation of `selectPresident()` gave rise to a potential denial of service(DOS)attack. An attacker could manipulate the voting process to prevent the selection of a president effectively freezing the contracts core functionality.
​
**Impact:** There's no limit on the number of unique candidates that can be added to `s_candidateList`.
​
An attacker could vote for a large number of addresses as candidates.
​
This can cause the function to run out of gas when trying to process all candidates making it unable to select a president.
​
**Recomended Mitigation:** Use mapping to track candidates instead of array.
​
Seperate the candidate selection phase from the president selection phase.
​
reject votes that could exceed the limits of the function.
​
​
**Tools Used:** Slither, claudeAi, ChatGpt.
​
Though I ignored Errors from Slither bcos the error were directly from Openzeppelin. and I believe there will be handled.
​
```javascript
Bitwise XOR Warnings.
The report mentions that Math.mulDiv uses a bitwise XOR operator (^) instead of the exponentiation operator (**).
​
2.Multiplication after division warning.
```
​
​

Recomended Mitigation: Note the function has a time check

if (
block.timestamp - s_previousVoteEndTimeStamp <=
i_presidentalDuration
) {
revert RankedChoice__NotTimeToVote();
}

but this only prevents the function from been called too early and not controlling it from beign called at the wrong time.

Implement a system where only an address or msg.sender as the case maybe will be able to call the function.

Make the selection of the president a formal governance proposal that must be voted on before execution.

Modify a function to be callable only at a specific time after the voting period ends.

[H-#3]Multiple entrants inSelectPresident()and _selectPresidentRecursive doing too many loops in same function(looping through voters, looping through candidates and selecting a president)leaves a function vulnerable to Denial Of Service Attack(DOS).

Description: The current Implementation of selectPresident() gave rise to a potential denial of service(DOS)attack. An attacker could manipulate the voting process to prevent the selection of a president effectively freezing the contracts core functionality.

Impact: There's no limit on the number of unique candidates that can be added to s_candidateList.

An attacker could vote for a large number of addresses as candidates.

This can cause the function to run out of gas when trying to process all candidates making it unable to select a president.

Recomended Mitigation: Use mapping to track candidates instead of array.

Seperate the candidate selection phase from the president selection phase.

reject votes that could exceed the limits of the function.

Updates

Lead Judging Commences

inallhonesty Lead Judge
9 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

A high number of candidates could cause an OOG

People who get to the age of 18 won't be able to vote, because VOTERS is provided in constructor and it can't be modified

Support

FAQs

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