Issue M-1: Compromise check will prevent malicious operations

Source: #23

Found by

056Security, 0x37, 0xBhumii, 0xMosh, 0xbrivan, Bozho, DigiSafe, Dliteofficial, IvanFitro, John44, KlosMitSoss, LeFy, PNS, TessKimy, dany.armstrong90, debugging3, dobrevaleri, durov, heeze, justAWanderKid, newspacexyz, onthehunt, s0x0mtee, sammy, shaflow01, t.aksoy, y4y

Summary

A missing compromise check in verifiedProfileIdForAddress will cause unauthorized access for affected contracts as compromised addresses can bypass security measures and perform malicious actions. (e.g: Attacker can steal user's private key, so address is compromised)

Root Cause

In EthosProfile.sol, the verifiedProfileIdForAddress function is missing a check to ensure _address is not compromised, allowing compromised addresses to interact with other contracts without restriction. https://github.com/sherlock-audit/2024-10-ethos-network/blob/979e352d7bcdba3d0665f11c0320041ce28d1b89/ethos/packages/contracts/contracts/EthosProfile.sol#L568-L574

The verifiedProfileIdForAddress function used in many contracts.

Here: https://github.com/sherlock-audit/2024-10-ethos-network/blob/979e352d7bcdba3d0665f11c0320041ce28d1b89/ethos/packages/contracts/contracts/EthosAttestation.sol#L228 https://github.com/sherlock-audit/2024-10-ethos-network/blob/979e352d7bcdba3d0665f11c0320041ce28d1b89/ethos/packages/contracts/contracts/EthosDiscussion.sol#L111-L113 https://github.com/sherlock-audit/2024-10-ethos-network/blob/979e352d7bcdba3d0665f11c0320041ce28d1b89/ethos/packages/contracts/contracts/EthosDiscussion.sol#L158-L160 ...

In this project, there are many issues about this compromised address. In almost contracts, it doesn't check msg.sender is compromised address. Address is already unregistered from profile by deleteAddressAtIndex function, but it is still used in many functions.

Internal pre-conditions

User needs to call deleteAddressAtIndex to set isAddressCompromised to be true for the target address.

External pre-conditions

No response

Attack Path

1. Attack steal user's private key.
2. User detected it is compromised and calls deleteAddressAtIndex for marking isAddressCompromised as true for the target address.
3. Attacker can calls addReview in EthorsReview contract by compromised address. (private key is stolen so attacker can do this operation) It calls ethosProfile.verifiedProfileIdForAddress(msg.sender); msg.sender is compromised but it doesn't revert.

Impact

The protocol suffers a potential security breach as compromised addresses can bypass verification and execute unauthorized actions in dependent contracts, potentially leading to manipulation of contract functionality. The attacker gains access to otherwise restricted operations without proper authorization.

PoC

No response

Mitigation

Add modifier checkIfCompromised and use checkIsAddressCompromised function.

Discussion

sherlock-admin2

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

Issue M-2: Attestation Reviews does not properly handle the case when attestation ownership has changed

Source: #304

Found by

0x37, 0xbrivan, Bozho, LeFy, TessKimy, debugging3, dobrevaleri, heeze, justAWanderKid, pashap9990, pkqs90, shaflow01, t.aksoy, tjonair

Summary

In 'EthosReview.sol' , when reviews of attestations are added , its pushed to reviewIdsBySubjectProfileId[subjectProfileId] , but in 'EthosAttestation.sol' one can claim an attestation that was previously claimed by another user using _claimAttestation() but then the reviews of the attestation will still be linked to the previous owner only.

Root Cause

In 'EthosReview.sol' , when reviews of attestations are added , its pushed to reviewIdsBySubjectProfileId[subjectProfileId]:

function _addReview(
    uint256 mockId,
    address subject,
    bool isAttestation,
    bytes32 attestationHash,
    IEthosProfile ethosProfile
  ) internal returns (uint256 subjectProfileId) {
    // if profileId does not exist for subject, create and record a "mock"
    if (mockId == 0) {
      subjectProfileId = ethosProfile.incrementProfileCount(
        isAttestation,
        subject,
        attestationHash
      );
    } else {
      subjectProfileId = mockId;
    }

    reviewIdsBySubjectProfileId[subjectProfileId].push(reviewCount);
  }

Repo Link

Now consider if the attestation has been claimed by another user who called the _claimAttestation():

  * @dev Claim previously created attestation.
   * @param profileId Profile id.
   * @param attestationHash Hash of the attestation.
   * @param evidence Evidence of attestation.
   * @return Whether the attestation was successfully claimed.
   */
  function _claimAttestation(
    uint256 profileId,
    bytes32 attestationHash,
    string calldata evidence
  ) private returns (bool) {
    if (!attestationExistsForHash(attestationHash)) {
      return false;
    }

    Attestation memory attestation = attestationByHash[attestationHash];

    if (attestation.profileId == profileId) {
      return false;
    }

    address ethosProfile = _getEthosProfile();

    (bool profileExists, ) = ITargetStatus(ethosProfile).targetExistsAndAllowedForId(profileId);

    if (!profileExists) {
      revert ProfileNotFound(profileId);
    }
    bool senderBelongsToProfile = IEthosProfile(ethosProfile).addressBelongsToProfile(
      msg.sender,
      profileId
    );

    if (!senderBelongsToProfile) {
      revert AddressNotInProfile(msg.sender, profileId);
    }

    // Remove attestation from the previous profile
    _removeAttestationFromPreviousProfile(attestation.profileId, attestationHash);

    // Set new profileId for attestation
    attestationByHash[attestationHash].profileId = profileId;
    attestationHashesByProfileId[profileId].push(attestationHash);
    // Update the index of the hash in the new profile
    hashIndexByProfileIdAndHash[profileId][attestationHash] =
      attestationHashesByProfileId[profileId].length -
      1;

    // Restore attestation if it was previously archived
    if (attestationByHash[attestationHash].archived) {
      attestationByHash[attestationHash].archived = false;
    }

    // Keep the profile contract up to date re: registered attestations
    IEthosProfile(ethosProfile).assignExistingProfileToAttestation(attestationHash, profileId);

    emit AttestationClaimed(
      attestationByHash[attestationHash].attestationId,
      attestationByHash[attestationHash].service,
      attestationByHash[attestationHash].account,
      evidence,
      profileId
    );

    return true;
  }

Now the attestation belongs to the second user, but in 'EthosReview.sol' its still linked to the previous owner profileId and every function that fetch reviews will read the reviewIdsBySubjectProfileId and the reviews will always be associated with the previous owner.

Impact

Even though attestation ownership has changed, the attestation reviews will still be linked to previous owner

Mitigation

There are 2 ways in which this can be mitigated, one is to define another mapping to explicitly track revieIds by attestation Hash instead of adding both address and attestation reviews to reviewIdsBySubjectProfileId[]:

 mapping(uint256 => uint256[]) public reviewIdsByAttestationHash;

Other way would be to implement a function which will be invoked whenever an attestation ownership has changed, that deletes the entry from reviewIdsBySubjectProfileId[previosOwner] and adds it to reviewIdsBySubjectProfileId[newOwner].

Discussion

sherlock-admin2

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

Issue M-3: The incorrect address is removed in _deleteAddressAtIndexFromArray()

Source: #316

Found by

0xNirix, 4gontuk, Albort, IvanFitro, LeFy, ParthMandale, Spomaria, TessKimy, araj, aycozyOx, dhananjay.pai, durov, heeze, justAWanderKid, newspacexyz, pashap9990, philmnds, smbv-1923, utsav, zeGarcao

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

Discussion

sherlock-admin2

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