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

Incorrect hashing in `rankCandidatesBySig` function leads to mismatched signer and msg.sender.

nadiamah

Summary

In the rankCandidatesBySig function logic is to call the _rankCandidates internal function by the address of the signer instead of msg.sender, this implementation allows the user(voters) to cast their vote more securely. The contract uses using EIP712 protocol for interacting with signatures. EIP721 has a function name _hashTypedDataV4 which gets the data of the target function (here the rankCandidates) and the data of its arguments (here address[] memory orderedCandidates) and keccak it in a specific way and then the function uses ECDSA protocol to recover the signer address. The functionality of working with these protocols is causing issues because at the end the signer and msg.sender address are not the same.

Vulnerability Details

The address of the singer which ECDSA protocol recovers is not the same as msg.sender (the voter who calls rankCandidatesBySig function)

Proof of code

- To access the signer in the rankCandidatesBySig function, I implement a child contract, whose functionality is the same as RankedChoice contract.
Put this in test/RankedChoiceTest.t.sol.

By running this test, we can figure out that the address of signer and msg.sender are not the same.

function testIncorrectRecoverSigner() public {
address user = vm.addr(2);
​
vm.startPrank(user);
RankedChoiceChild rankedChoiceChild = new RankedChoiceChild(voters);
orderedCandidates = [candidates[0], candidates[1], candidates[2]];
​
bytes32 typeHash = keccak256("rankCandidates(uint256[])");
bytes32 structHash = keccak256(abi.encode(typeHash, orderedCandidates));
bytes32 hash = _hashTypedDataV4(structHash);
​
(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, hash);
bytes memory signature = abi.encodePacked(r, s, v);
address signer = rankedChoiceChild.rankCandidatesBySig(
orderedCandidates,
signature
);
​
vm.stopPrank();
assert(user == signer);
}

  • Put this outside the test contract.

contract RankedChoiceChild is EIP712 {
bytes32 public constant TYPEHASH = keccak256("rankCandidates(uint256[])");
address[] private VOTERS;
uint256 private immutable i_presidentalDuration;
address private s_currentPresident;
uint256 private s_voteNumber;
​
constructor(address[] memory voters) EIP712("RankedChoice", "1") {
VOTERS = voters;
i_presidentalDuration = 1460 days;
s_currentPresident = msg.sender;
s_voteNumber = 0;
}
function rankCandidatesBySig(
address[] memory orderedCandidates,
bytes memory signature
) external returns (address) {
bytes32 structHash = keccak256(abi.encode(TYPEHASH, orderedCandidates));
bytes32 hash = _hashTypedDataV4(structHash);
address signer = ECDSA.recover(hash, signature);
return signer;
}
}
​

Impact

  1. Malicious Manipulation
    As the function does not have a condition to check if the signer is actually the msg.sender, an attacker could attempt to generate a valid signature for a different address. If the logic does not correctly verify the signer against expected conditions, it could lead to unauthorized actions.

  2. User Confusion
    If users expect a certain behavior based on the signature but see unexpected results due to mismatched signers, it could lead to a lack of trust in the application.

Recommendations

  1. Make sure the process of extracting the signature is correct.

  • Possible issue: There might be a problem with the TYPEHASH constant, which should be assigned like this:

- bytes32 public constant TYPEHASH = keccak256("rankCandidates(uint256[])");
+ bytes32 public constant TYPEHASH = keccak256("rankCandidates(address[])");
​

  1. Make sureEIP712::_hashTypedDataV4 entries are correct.

  • Possible issue: It is possible that different parameters should be passed to the _hashTypedDataV4 function, instead of passing orderedCandidates itself maybe we should pass the keccak256 of it.

  1. Make sure that the rankCandidatesBySig has an if statement to check whether the signer and msg.sender are the same before calling the _rankCandidates function.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Typehash hashes the wrong function input.

Support

FAQs

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