099.md

Sparkly Seaweed Wolf

Medium

Some attestation review may be loss when mockId changes

Summary

One attestation's mock profile may be changed. This will cause the previous review is unavailable.

Root Cause

In EthosProfile, we will use one mockId to record attestation reviews. When we add one review via addReview for one attestation, one mockId for this attestation will be created or used for one attestation. If there is not one mock for this attestation, we will create one mockId, otherwise, we will use the previous created mockId. This will guarantee that all reviews fro this attestation will be aggregated via this mockId. The problem is that one attestation's mockId may be changed if this attestation is claimed via _claimAttestation. When the mockId for this attestation is updated, previous reviews are still connected with the previous mockId. When users want to get all reviews via this attestation hash, they cannot get the previous reviews.

  function addReview(
    Score score,
    address subject,
    address paymentToken,
    string calldata comment,
    string calldata metadata,
    AttestationDetails calldata attestationDetails
  ) external payable whenNotPaused {
    ...
    IEthosProfile ethosProfile = _getEthosProfile();
    bytes32 attestationHash;
    uint256 mockId;
    if (subject != address(0)) {
    ...
    } else {
      attestationHash = _getEthosAttestation().getServiceAndAccountHash(
        attestationDetails.service,
        attestationDetails.account
      );
      mockId = ethosProfile.profileIdByAttestation(attestationHash);
      mockId = _addReview(mockId, subject, true, attestationHash, ethosProfile);
    }
    ...
}
  function _addReview(
    uint256 mockId,
    ...
  ) internal returns (uint256 subjectProfileId) {
    if (mockId == 0) {
      subjectProfileId = ethosProfile.incrementProfileCount(
        isAttestation,
        subject,
        attestationHash
      );
    } else {
      subjectProfileId = mockId;
    }
   ...
  }

  function _claimAttestation(
    uint256 profileId,
    bytes32 attestationHash,
    string calldata evidence
  ) private returns (bool) {
    ...
    // Keep the profile contract up to date re: registered attestations
    IEthosProfile(ethosProfile).assignExistingProfileToAttestation(attestationHash, profileId);
    ...
  }
  function assignExistingProfileToAttestation(
    bytes32 attestationHash,
    uint256 profileId
  ) external isEthosAttestation {
    profileIdByAttestation[attestationHash] = profileId;
  }

Internal pre-conditions

1. One attestation is claimed and assigned to another mockId.

External pre-conditions

N/A

Attack Path

1. Alice creates one attestation.
2. Bob creates one review for this attestation. MockId is 2. Bob's review will be connected with mockId 2.
3. Cathy claim this attestation and assign this attestation's mockId to 3.
4. David create another review for this attestation. MockId is 3.
5. When users want to get all reviews for this attestation, only David's review is available via reviewsByAttestationHashInRange.

Impact

Some previous reviews will not be available for function reviewsByAttestationHashInRange. This will break our design intent that aggregate all reviews together for the same attestation.

PoC

N/A

Mitigation

When one attestation's profile id is changed, we should update reviewIdsBySubjectProfileId timely.