A compromised address can still interact with the protocol

Summary

In the Profile contract, an owner can choose to delete address in case of any address being compromised, and such address should not be able to interact with other entities, however, they still can due to how profile is fetched in other contracts.

Root Cause

According to the design, it's encourged for an user to have multiple addresses associated with one profile. User can add their alternate addresses to a profile by registering them in registerAddress, similarly, an address can also be deleted from the profile in case of compromise. This would keep compromised address from being invited to create a new profile, as well as interacting with other profiles/replies, etc.

In the deletion logic, to be deleted address will be popped from Profile.addresses, and added to the Profile.removedAddresses array by:

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

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

However, we see that the address still exists in profileIdByAddress map. This is somewhat intended, as the protocol would need to record past addresses associated with the profile as well, but in other contracts, profileIdByAddress is used to verify user identity. For example, in the Review contract _validateReviewDetails:

  function _validateReviewDetails(
    address subject,
    AttestationDetails calldata attestationDetails
  ) private view {
    if (
      subject == address(0) &&
      (bytes(attestationDetails.account).length == 0 ||
        bytes(attestationDetails.service).length == 0)
    ) {
      revert InvalidReviewDetails("None set");
    }

    if (
      subject != address(0) &&
      (bytes(attestationDetails.account).length != 0 ||
        bytes(attestationDetails.service).length != 0)
    ) {
      revert InvalidReviewDetails("Both set");
    }

    // sender and subject are the exact same address
    if (subject == msg.sender) {
      revert SelfReview(subject);
    }

    uint256 authorProfileId = _getEthosProfile().profileIdByAddress(msg.sender); // <=(1)
    if (subject != address(0)) {
      // if reviewing by address, check if the author's profile is the same as the subject's profile
      uint256 subjectProfileId = _getEthosProfile().profileIdByAddress(subject); // <=(2)
      if (authorProfileId == subjectProfileId) {
        revert SelfReview(subject);
      }
    } else {
      // if reviewing by attestation, check if the author's profile is the same as the subject's profile
      bytes32 attestationHash = _getEthosAttestation().getServiceAndAccountHash(
        attestationDetails.service,
        attestationDetails.account
      );
      uint256 subjectProfileId = _getEthosProfile().profileIdByAttestation(attestationHash);
      if (authorProfileId == subjectProfileId) {
        revert SelfReview(subject);
      }
    }
  }

Which we can see, the contract uses profileIdByAddress to get profileId, which is equivalent of profileIdByAddress[msg.sender]. In the case of compromised address, this would return the profileId associated with this compromised address, despite, technically this address is not longer linked to this profile after deletion. This is one of the example, the similar logic also happens in Discussion and Attestation contract, this means a compromised address would still be interacting with other entities, under the innocent Profile.

Other examples would be inviting and archiving, as those function don't check if msg.sender is compromised either, but only checks profileIdByAddress, which would also return the profile this compromised address had registered before.

Internal pre-conditions

Bob has 5 addresses, and he created a profile with invitation. Later, unfortunately, one of his address gets compromised by Alice. Bob realized this, and immediately deleted this address from his Profile.

External pre-conditions

No response

Attack Path

Despite Alice holds a deleted address, she can still comment/review, so she would intentionally comment bad things on other people's profile or reply. The community noticed this Profile belongs to Bob, and he would be potentially subjected with a slash due to bad behavior.

Impact

Since there is a slash mechanics, so Bob would lose some of his funds due to the consequences caused by Alice, with a compromised address he does not own anymore.

PoC

No response

Mitigation

Write a function to get profile by address, and revert when the address is compromised.

sherlock-audit / 2024-10-ethos-network-judging

Sleepy Myrtle Grasshopper - A compromised address can still interact with the protocol #63