ParthMandale - The incorrect address is removed in _deleteAddressAtIndexFromArray()

[sherlock-admin3]

ParthMandale

Medium

The incorrect address is removed in _deleteAddressAtIndexFromArray()

Summary

function _deleteAddressAtIndexFromArray - is used to delete the address of the user that is associated with that current profile id at that index.

And therefore this function is supposed to - remove that particular index of arrays(addresses) from the addresses array.

And at the same time that same particular address of index of arrays(addresses) is supposed to be added in removedAddresses array.

But here the last index is added in removedAddresses array -> removedAddresses.push(addr); which is completely wrong.

Root Cause

https://github.com/sherlock-audit/2024-10-ethos-network/blob/db37b9dc2b792e245eb683d8a956bcb7ef2f1a27/ethos/packages/contracts/contracts/EthosProfile.sol#L591

Whenever a user deletes an address at an index through deleteAddressAtIndex() it would delete wrong address.

function deleteAddressAtIndex(uint256 addressIndex) external whenNotPaused {
    uint256 profileId = profileIdByAddress[msg.sender];
    (bool verified, bool archived, bool mock) = profileStatusById(profileId);
    if (!verified) {
      revert ProfileNotFoundForAddress(msg.sender);
    }
    if (archived || mock) {
      revert ProfileAccess(profileId, "Profile is archived");
    }

    address[] storage addresses = profiles[profileId].addresses;
    address[] storage removedAddresses = profiles[profileId].removedAddresses;
    if (addresses.length <= addressIndex) {
      revert InvalidIndex();
    }

    address addressStr = addresses[addressIndex];
    isAddressCompromised[addressStr] = true;
    _addressShouldDifferFromSender(addressStr);

@>  _deleteAddressAtIndexFromArray(addressIndex, addresses, removedAddresses);

    emit AddressClaim(profileId, addressStr, AddressClaimStatus.Unclaimed);
  }

Then in function _deleteAddressAtIndexFromArray is getting executed -

  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();
  }

We can clearly see that, here the last index is added in removedAddresses array -> removedAddresses.push(addr); which is wrong and instead index that was passed was supposed to be push into this removedAddresses array.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

1. The incorrect address is removed in _deleteAddressAtIndexFromArray()

2. If someone has two addresses, one is compromised, but the other one is still valid. If removal of the wrong address is happening, the other account that was supposed to be removed index, is now has control over the account. The attacker can archive the profile and front-run any attempt to un-archive it, keeping the profile permanently out of the hands of the valid owner.

PoC

No response

Mitigation

Implement _deleteAddressAtIndexFromArray function like this -

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

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/trust-ethos/ethos/pull/1759