Summary
The hashed transaction message in rankCandidatesBySig is not correctly done resulting any voter who calls this function could either end up with a invalid recovered signature that is not in the VOTERS list or the voter is forced to cast their rankedCandidates in the exact order as the sponsor if they choose to use this function to avoid paying gas fee.
Ref Line of Code: #L23, #L54
https://github.com/Cyfrin/2024-09-president-elector/blob/fccb8e2b6a32404b4664fa001faa334f258b4947/src/RankedChoice.sol#L23
https://github.com/Cyfrin/2024-09-president-elector/blob/fccb8e2b6a32404b4664fa001faa334f258b4947/src/RankedChoice.sol#L54
Vulnerability Details
In rankCandidatesBySig function, the hash transaction message is stated as bytes32 structHash = keccak256(abi.encode(TYPEHASH, orderedCandidates)); in which the TYPEHASH is defined as keccak256("rankCandidates(uint256[])"). The structHash has factored in the function parameter orderedCandidates during the hashing. If the sponsor who has his signature shared to voter to use this function has the orderedCandidates different from the voter's orderedCandidates, the recovered signature will then be invalid.
function rankCandidatesBySig(
address[] memory orderedCandidates,
bytes memory signature
) external {
<@@>! bytes32 structHash = keccak256(abi.encode(TYPEHASH, orderedCandidates));
bytes32 hash = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(hash, signature);
_rankCandidates(orderedCandidates, signer);
}
Alternatively, if voter still wants to use the rankCandidatesBySig function to cast their votes to save gas fee, the voter can only cast their vote with the exact same candidate order list as the sponsor. This however has violated the democratic election as the casting has now been forcefully to follow a predefined candidate list favorable to the sponsor.
Prior to tapout the PoC, it is best to note that the original TYPEHASH in the contract is also found not appropriately hashed as the function rankCandidates takes in an array of address not an array of uint256. Though this step doesn't affect the recovery of signature, for completeness purpose, the following PoC will use the corrected TYPEHASH as keccak256("rankCandidates(address[])") instead.
contract RankedChoice is EIP712 {
....
uint256 private s_voteNumber;
uint256 private immutable i_presidentalDuration;
- bytes32 public constant TYPEHASH = keccak256("rankCandidates(uint256[])");
+ bytes32 public constant TYPEHASH = keccak256("rankCandidates(address[])");
uint256 private constant MAX_CANDIDATES = 10;
....
}
Proof of Concept:
Setup a HelperContract for ease of PoC to interact with various functions in contract RankedChoice and EIP712 during test case.
pragma solidity 0.8.24;
import {RankedChoice} from "./RankedChoice.sol";
contract HelperContract is RankedChoice {
constructor(address[] memory voters) RankedChoice(voters) {}
function getAdjustedTypeHashMessage(address[] memory orderedCandidates) public view returns(bytes32) {
bytes32 TYPEHASH = keccak256("rankCandidates(address[])");
bytes32 structHash = keccak256(abi.encode(TYPEHASH, orderedCandidates));
bytes32 hash = _hashTypedDataV4(structHash);
return hash;
}
}
2.Modify the test/RankedChoiceTest.t.sol by adding in more specific setup for use in PoC test cases
contract RankedChoiceTest is Test {
address[] voters;
address[] candidates;
address[] voters_poc;
address[] candidates_poc;
address[] orderedCandidates_sponsor = new address[]();
address sponsor;
uint256 sponsorKey;
bytes sponsorSignature;
uint256 constant MAX_VOTERS = 100;
uint256 constant MAX_CANDIDATES = 20;
uint256 constant VOTERS_ADDRESS_MODIFIER = 100;
uint256 constant CANDIDATES_ADDRESS_MODIFIER = 200;
RankedChoice rankedChoice;
HelperContract helperContract;
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)));
}
address[] memory newVoters = new address[]();
(sponsor, sponsorKey) = makeAddrAndKey("Sponsor");
newVoters[0] = sponsor;
for (uint256 i = 0; i < voters.length; i++) {
newVoters[i + 1] = voters[i];
}
voters_poc = newVoters;
for (uint256 i = 0; i < 10; i++) {
candidates_poc.push(address(uint160(i + CANDIDATES_ADDRESS_MODIFIER)));
}
orderedCandidates_sponsor[0] = candidates_poc[0];
orderedCandidates_sponsor[1] = candidates_poc[1];
orderedCandidates_sponsor[2] = candidates_poc[2];
helperContract = new HelperContract(voters_poc);
}
modifier tapoutSponsorSignature() {
vm.prank(sponsor);
bytes32 hash = helperContract.getAdjustedTypeHashMessage(orderedCandidates_sponsor);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(sponsorKey, hash);
bytes memory signature = abi.encodePacked(r, s, v);
sponsorSignature = signature;
_;
}
function test_audit_incorrectHashMessage() public tapoutSponsorSignature {
address voter = voters_poc[1];
address[] memory voterPreferredCandidates = new address[]();
voterPreferredCandidates[0] = candidates_poc[4];
voterPreferredCandidates[1] = candidates_poc[5];
voterPreferredCandidates[2] = candidates_poc[6];
voterPreferredCandidates[3] = candidates_poc[7];
if (!_isInArray(VOTERS, signer)) {
revert RankedChoice__InvalidSigner();
}
*/
vm.startPrank(voter);
vm.expectRevert(RankedChoice.RankedChoice__InvalidSigner.selector);
helperContract.rankCandidatesBySig(voterPreferredCandidates, sponsorSignature);
helperContract.rankCandidatesBySig(orderedCandidates_sponsor, sponsorSignature);
address[] memory voterVote = helperContract.getUserCurrentVote(voter);
assertEq(voterVote, orderedCandidates_sponsor);
vm.stopPrank();
}
3.Run the test with forge test --match-test test_audit_incorrectHashMessage
The test passes showing that revert will occur if voter uses their own orderedCandidates list and if they were to use the rankCandidatesBySig to save the gas fee, they have to cast their votes to the exact same orderedCandidate as the sponsor
Impact
Voter couldn't cast with their own orderedCandidates list and if they were to use the rankCandidatesBySig to save the gas fee, they have to cast their votes to the exact same orderedCandidate as the sponsor, violating the democratic of election.
Tools Used
Manual review with test
Recommendations
Amend the structHash variable in rankCandidatesBySig to exclude hashing the orderedCandidates on top of the fixes on erroneous _rankCandidates and add in signer verification check as shown below:
function rankCandidatesBySig(
address[] memory orderedCandidates,
bytes memory signature
) external {
- bytes32 structHash = keccak256(abi.encode(TYPEHASH, orderedCandidates));
+ bytes32 structHash = keccak256(abi.encode(TYPEHASH));
bytes32 hash = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(hash, signature);
+ if (!_isInArray(VOTERS, signer)) {
+ revert RankedChoice__InvalidSigner();
+ }
+ _rankCandidates(orderedCandidates, msg.sender);
- _rankCandidates(orderedCandidates, signer);
}
