LeFy - Attestation Reviews does not properly handle the case when attestation ownership has changed

[sherlock-admin2]

LeFy

Medium

Attestation Reviews does not properly handle the case when attestation ownership has changed

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].

[sherlock-admin2]

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