Incorrect Mapping Update During Nominee Removal Leads to Inconsistent State

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/governance/contracts/VoteWeighting.sol#L621-L627

Vulnerability details

Summary

When removing a nominee, the function removeNominee in the provided code attempts to replace the nominee being removed with the last nominee in the setNominees array and then remove the last nominee from the array. However, the current implementation updates the mapNomineeIds mapping for the replaced nominee before actually moving the last nominee into the removed nominee's slot. This sequence leads to an incorrect update of mapNomineeIds. It also does not handle the case where the nominee being removed is the last one in the array

Impact

This issue can lead to inconsistencies and incorrect states within the contract's data structures. Specifically, the mapping mapNomineeIds may contain incorrect nominee IDs, potentially causing errors in subsequent operations that rely on accurate nominee data. These inconsistencies can disrupt the proper functioning of the voting and nomination processes, possibly leading to erroneous removal, replacement, or querying of nominees.

Proof of Concept

Here's the problematic part of the removeNominee function:

// Remove nominee from the map
mapNomineeIds[nomineeHash] = 0;

// Shuffle the current last nominee id in the set to be placed to the removed one
nominee = setNominees[setNominees.length - 1];
bytes32 replacedNomineeHash = keccak256(abi.encode(nominee));
mapNomineeIds[replacedNomineeHash] = id;
setNominees[id] = nominee;
// Pop the last element from the set
setNominees.pop();

Issue Breakdown The code prematurely updates mapNomineeIds for the replaced nominee before moving the last nominee to the removed nominee's slot. This can lead to an incorrect state in mapNomineeIds, where the mapping does not reflect the correct positions of the nominees. Here's a step-by-step description of the problematic sequence:

Assume setNominees contains [N1, N2, N3] and we are removing N2. The removeNominee function sets mapNomineeIds[hash(N2)] to 0. It then fetches N3 and updates mapNomineeIds[hash(N3)] to point to the index of N2 before actually moving N3 into the slot previously occupied by N2.

Tools Used

Manual Code Review

Recommended Mitigation Steps

To correct this issue, the nominee should be moved first before updating the mapping. Here is the revised code:

// Get the last index of the nominees array
uint256 lastIndex = setNominees.length - 1;
if (id != lastIndex) {
    // Shuffle the current last nominee id in the set to be placed to the removed one
    Nominee memory lastNominee = setNominees[lastIndex];
    bytes32 lastNomineeHash = keccak256(abi.encode(lastNominee));
    // Update the mapping after moving the nominee
    setNominees[id] = lastNominee;
    mapNomineeIds[lastNomineeHash] = id;
}
// Pop the last element
setNominees.pop();
// Finally, remove the nominee from the mapping
mapNomineeIds[nomineeHash] = 0;

This sequence ensures that the nominee is moved first before updating the mapping, preventing inconsistencies. It also correctly handles the case where the nominee being removed is the last one in the array.

Assessed type

Other