aycozyOx - Incorrect Address removal Handling in _deleteAddressAtIndexFromArray() of EthosProfile.sol

[sherlock-admin2]

aycozyOx

Medium

Incorrect Address removal Handling in _deleteAddressAtIndexFromArray() of EthosProfile.sol

Summary

The _deleteAddressAtIndexFromArray function in Ethosreview.sol incorrectly handles the deletion of an address from the addresses array by pushing a valid address into the removedAddresses array instead of the address being removed. This leads to incorrect tracking of removed addresses.

Root Cause

The root cause of this issue is the logic that pushes the last address in the addresses array (which is used to replace the address at the specified index) into the removedAddresses array, instead of pushing the address that is actually being removed.

https://github.com/sherlock-audit/2024-10-ethos-network/blob/main/ethos/packages/contracts/contracts/EthosReview.sol#L585-L594

 function _deleteAddressAtIndexFromArray(
    uint256 index,
    address[] storage addresses,
    address[] storage removedAddresses
  ) private {
    address addr = addresses[addresses.length - 1];//
    addresses[index] = addr;
    removedAddresses.push(addr);
    addresses.pop();
  }

Internal pre-conditions

-The function is called with a valid index within the bounds of the addresses array. -The removedAddresses array is intended to track addresses that have been removed from the addresses array.

External pre-conditions

No response

Attack Path

-The function is called to remove an address at a specific index in the addresses array. -The address at the specified index is replaced with the last address in the array. -The last address (which is still valid) is pushed into the removedAddresses array. -The actual address that was intended to be removed is not tracked in the removedAddresses array.

Impact

The removedAddresses array contains valid addresses instead of the addresses that were actually removed, leading to incorrect tracking.

PoC

Profile A owner endeavors to delete one of their addresses that is now being operated by a malicious user(or for whatever reasons) and calls deleteAddressAtIndex() which goes ahead to call the internal function _deleteAddressAtIndexFromArray() with the index of address to be removed, the array of address and the array of removed addresses currently linked to Profile A.

_deleteAddressAtIndexFromArray() however during its attempt to delete the intended address fails to add the removed address to the removed address array but erroneously adds the last valid address in the array of addresses linked to profile A.

Here is the internal function _deleteAddressAtIndexFromArray() again

 function _deleteAddressAtIndexFromArray(
    uint256 index,
    address[] storage addresses,
    address[] storage removedAddresses
  ) private {
    address addr = addresses[addresses.length - 1];  //caches the last addresses
    addresses[index] = addr;
    removedAddresses.push(addr);//erroneously adds the address to the removedAddresses array 
    addresses.pop();
  }

Mitigation

first of all store the address to be removed and then push it properly into the removedAddresses array.

function _deleteAddressAtIndexFromArray(
    uint256 index,
    address[] storage addresses,
    address[] storage removedAddresses
) private {
    address removedAddress = addresses[index]; // Store the address being removed
    address addr = addresses[addresses.length - 1];
    addresses[index] = addr;
    removedAddresses.push(removedAddress); // Push the removed address to removedAddresses
    addresses.pop();
}