President Elector

Summary

The audit of the RankedChoice smart contract identified a critical vulnerability in the implementation of EIP712 signature verification for off-chain vote submissions.

Vulnerability Details

Incorrect EIP712 Signature Verification with Dynamic Arrays

• Location: rankCandidatesBySig function

• Description: The contract attempts to verify EIP712 signatures that include a dynamic array (address[] memory orderedCandidates). However, EIP712 does not natively support dynamic arrays in the struct type definitions. The current implementation uses abi.encode with a TYPEHASH that mismatches the actual data types (uint256[] vs. address[]), leading to incorrect hash computation.

• Issue Details:

  • Type Mismatch: The TYPEHASH specifies uint256[], but orderedCandidates is an address[], causing a mismatch in the hashed data.

  • Dynamic Array Encoding: EIP712 requires special handling for dynamic arrays, typically involving hashing each element and then hashing the array. The contract does not implement this, resulting in incorrect signature hashes.

  • Potential Exploit: Since signatures will not verify correctly, voters using rankCandidatesBySig may find their votes unaccepted, leading to a denial of service for off-chain vote submissions.

Impact

• Denial of Service: Voters attempting to submit their rankings via rankCandidatesBySig will be unable to do so due to invalid signature verification.

• Reduced Participation: The inability to submit votes off-chain can discourage voter participation, undermining the integrity and accessibility of the election process.

• Reputation Risk: The malfunctioning of a critical feature can damage the trust in the contract and the organization deploying it.

Recommendations

1. Correct EIP712 Implementation:

   • Define a Struct for the Vote: Create a struct that includes all necessary fields, including the voter's address to prevent replay attacks.

   • Handle Dynamic Arrays Properly: Implement a method to hash the dynamic array according to EIP712 specifications. This often involves hashing each element and then hashing the array as a whole.

   • Update the TYPEHASH: Ensure the TYPEHASH matches the actual data types used in the struct.

2. Include Voter Address in Signature:

   • Bind Signature to Voter: Include the voter's address in the signed data to prevent misuse of signatures and enhance security.

3. Add Nonce for Replay Protection:

   • Implement Nonce Management: Use a nonce to prevent replay attacks where a signature could be reused to overwrite a voter's ranking unintentionally.

4. Thorough Testing:

   • Unit Tests: Implement comprehensive unit tests for the signature verification process.

   • Integration Tests: Test the off-chain signing and on-chain verification flow end-to-end.

5. Consult EIP712 Libraries:

   • Use Established Libraries: Consider using well-tested libraries or utilities that handle EIP712 encoding for dynamic arrays.

Incorrect TYPEHASH in rankCandidatesBySig

The TYPEHASH constant is incorrectly defined as:

bytes32 public constant TYPEHASH = keccak256("rankCandidates(uint256[])");

However, the orderedCandidates parameter is an array of address, not uint256. This mismatch leads to incorrect hash calculations, causing signature verifications to fail or allowing malicious actors to forge signatures.

Recommendation:

Update the TYPEHASH to match the correct data type:

bytes32 public constant TYPEHASH = keccak256("rankCandidates(address[])");

Ensure that all hashing and signature recovery processes use the correct data types to maintain the integrity of the signature verification.

State Contamination Due to Unreset Variables

Severity: High

Description:

The variables s_candidateList and s_candidateVotesByRound are not cleared at the beginning of each new election cycle in the selectPresident function. This oversight can cause residual data from previous elections to interfere with the current election results.

Impact:

• Persistent Candidates: Old candidates may remain in s_candidateList, causing inaccurate candidate pools.

• Incorrect Vote Tallies: Previous rounds' votes may persist in s_candidateVotesByRound, leading to erroneous vote counts.

Recommendation:

• Reset s_candidateList at the start of the selectPresident function:

  function selectPresident() external {

  if (block.timestamp - s_previousVoteEndTimeStamp <= i_presidentalDuration) {

  revert RankedChoice__NotTimeToVote();

  }

  // Reset candidate list

  s_candidateList = new address;

  // Rest of the function...

  }

Clear s_candidateVotesByRound for the current s_voteNumber:

Lack of Input Validation for Candidates

There's no validation to ensure that candidate addresses are valid (e.g., non-zero addresses) or that there are no duplicate candidates in a voter's ranking.

Recommendation:

• Validate Non-Zero Addresses:

  for (uint256 i = 0; i < orderedCandidates.length; i++) {

  if (orderedCandidates[i] == address(0)) {

  revert RankedChoice__InvalidInput();

  }

  }

  ​

Potential Privacy Concerns

The getUserCurrentVote function allows anyone to view a voter's current rankings. If voter anonymity is a concern, this could lead to privacy issues.

Recommendation:

• Restrict Access:

  Limit the visibility of voters' rankings or remove the function entirely if privacy is essential.

• Provide Transparency Notice:

  If transparency is acceptable, document this behavior so voters are aware that their rankings are public.

Voters Can Change Their Votes Multiple Times

Severity: Informational

Description:

Voters can overwrite their previous rankings by calling rankCandidates multiple times before the election period ends. While this may be intended, it could be exploited to manipulate the election if not properly managed.

Recommendation:

• Prevent Multiple Voting:

  If only one vote per voter is desired, add a check:

  if (s_rankings[voter][s_voteNumber].length > 0) {

  revert RankedChoice__AlreadyVoted();

  }

• Accept Vote Changes:

  If voters are allowed to change their votes, ensure this behavior is clearly documented.

Potential Denial of Service via Unbounded Loops

Functions like _isInArray and loops within selectPresident and _selectPresidentRecursive iterate over dynamic arrays, which could grow large and cause transactions to run out of gas.

Recommendation:

• Limit Array Sizes:

  Enforce maximum sizes for VOTERS and candidate arrays.

• Optimize Loop Logic:

  Consider using mappings or more efficient data structures to minimize gas consumption.

Tools Used

• Manual code review

• Solidity documentation and EIP712 specification

• OpenZeppelin EIP712 utilities