Inconsistent Index Management in removeVoters Function

[nathanogaga118]

Vulnerability Summary

The removeVoters function in the ParanetNeuroIncentivesPool contract fails to properly update the votersIndexes mapping when voters are removed from the voters array. This oversight can lead to incorrect data access, logic errors, and potential exploits due to stale or incorrect index references.

Vulnerable Code

https://github.com/OriginTrail/dkg-evm-module/blob/main/contracts/paranets/ParanetNeuroIncentivesPool.sol

function removeVoters(uint256 limit) external onlyVotersRegistrar {
require(voters.length >= limit, "Limit exceeds the num of voters");

for (uint256 i; i < limit; ) {
cumulativeVotersWeight -= uint16(voters[voters.length - 1 - i].weight);

delete votersIndexes[voters[voters.length - 1 - i].addr];
voters.pop();

unchecked {
    i++;
}

}
}

Detailed Description

The removeVoters function is intended to remove a specified number of voters from the voters array. However, the function only deletes the mapping entry for the removed voter without updating the indexes of the remaining voters. This can lead to several issues:

Stale Indexes: When a voter is removed, the indexes of the remaining voters in the votersIndexes mapping are not updated. This means that the mapping may point to incorrect positions, leading to data retrieval errors.

Logic Errors: Functions that rely on the votersIndexes mapping to access voter data may operate on incorrect data, leading to unintended behavior or incorrect reward calculations.

Potential Exploits: An attacker could exploit this inconsistency to manipulate the contract's state or claim rewards they are not entitled to, by creating scenarios where the contract logic fails due to incorrect indexing.

Maintenance Challenges: The inconsistency between the voters array and the votersIndexes mapping makes the contract harder to maintain and debug, as the actual state may not match the expected state based on the mappings.

Impact

Data Inconsistency: Incorrect index references can lead to data retrieval errors and logic failures.
Incorrect Logic Execution: Functions relying on the mapping may execute incorrectly, leading to potential financial discrepancies.

Potential Exploits: The inconsistency can be exploited to perform unauthorized actions or claim rewards.
Maintenance and Debugging Issues: The contract becomes more challenging to maintain and debug due to inconsistent state representation.

Severity

critical: it can cause significant operational issues and potential exploits if not addressed. The severity is critical due to the potential for incorrect logic execution and data inconsistency.

Proof of Concept (PoC) Using Foundry's Forge

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import "../src/ParanetNeuroIncentivesPool.sol"; // Adjust the import path as necessary

contract ParanetNeuroIncentivesPoolTest is Test {
ParanetNeuroIncentivesPool pool;

function setUp() public {
// Initialize the contract with dummy data
pool = new ParanetNeuroIncentivesPool(
address(this), // hubAddress
address(this), // paranetsRegistryAddress
address(this), // knowledgeMinersRegistryAddress
bytes32(0), // paranetId
1e12, // tracToNeuroEmissionMultiplier
1000, // paranetOperatorRewardPercentage
1000 // paranetIncentivizationProposalVotersRewardPercentage
);

// Add voters
ParanetLib.ParanetIncentivizationProposalVoterInput[] memory voters = new ParanetLib.ParanetIncentivizationProposalVoterInput[](3);voters[0] = ParanetLib.ParanetIncentivizationProposalVoterInput({addr: address(0x1), weight: 100});
voters[1] = ParanetLib.ParanetIncentivizationProposalVoterInput({addr: address(0x2), weight: 200});
voters[2] = ParanetLib.ParanetIncentivizationProposalVoterInput({addr: address(0x3), weight: 300});

pool.addVoters(voters);

}

function testVoterRemovalIssue() public {
// Remove a voter
pool.removeVoters(1);

// Check if the index mapping is incorrect
uint256 index1 = pool.votersIndexes(address(0x1));
uint256 index2 = pool.votersIndexes(address(0x2));
uint256 index3 = pool.votersIndexes(address(0x3));

// Assert that the indexes are incorrect after removal
assertEq(index1, 0, "Index for address(0x1) should be 0");
assertEq(index2, 1, "Index for address(0x2) should be 1");
assertEq(index3, 2, "Index for address(0x3) should be 2");

// After removal, the index for address(0x3) should be updated, but it won't be in the current implementation

}
}

Recommended Fix

To address the issue, update the votersIndexes mapping whenever the voters array is modified. Here's a suggested fix for the removeVoters function:

function removeVoters(uint256 limit) external onlyVotersRegistrar {
require(voters.length >= limit, "Limit exceeds the num of voters");

for (uint256 i; i < limit; ) {
uint256 lastIndex = voters.length - 1 - i;
address voterToRemove = voters[lastIndex].addr;

cumulativeVotersWeight -= uint16(voters[lastIndex].weight);

// If the voter to remove is not the last one, swap it with the last one
if (lastIndex != voters.length - 1) {
    voters[lastIndex] = voters[voters.length - 1];
    votersIndexes[voters[lastIndex].addr] = lastIndex;
}

// Remove the last voter
voters.pop();
delete votersIndexes[voterToRemove];

unchecked {
    i++;
}

}
}
This fix ensures that when a voter is removed, the last voter in the array is moved to the removed voter's position, and the votersIndexes mapping is updated accordingly. This maintains consistency between the array and the mapping, preventing stale or incorrect index references.