pkqs90 - EthosReview `reviewIdsBySubjectAddress()` function returns incorrect result if address is registered to another profileId.

[sherlock-admin2]

pkqs90

Medium

EthosReview reviewIdsBySubjectAddress() function returns incorrect result if address is registered to another profileId.

Summary

EthosReview reviewIdsBySubjectAddress() function returns incorrect result if address is registered to another profileId.

Root Cause

When users are adding a review to an address, but the address is not bind to a profileId yet, a mock profileId will be created. The review for that address would be recorded in reviewIdsBySubjectProfileId[subjectProfileId], with the subjectProfileId being the profileId of the address.

However, if this address is later registered to another ProfileId, it's profileId will change. This means all previous reviewIds in EthosReview would not be accessible anymore. Specifically this function used to look up address reviews: reviewIdsBySubjectAddress(). This function will lookup the reviewIds of the address's new profileId, but the old reviews (which was recorded on the mock profileId) will not be returned.

This is a very normal use case, and reviews would not be correctly returned. Considering that the EthosContract serve as a database layer to serve the upper level frontend apps, this is a critical issue.

https://github.com/sherlock-audit/2024-10-ethos-network/blob/main/ethos/packages/contracts/contracts/EthosReview.sol#L173-L249

  function addReview(
    Score score,
    address subject,
    address paymentToken,
    string calldata comment,
    string calldata metadata,
    AttestationDetails calldata attestationDetails
  ) external payable whenNotPaused {
    _validateReviewDetails(subject, attestationDetails);

    IEthosProfile ethosProfile = _getEthosProfile();
    bytes32 attestationHash;
    uint256 mockId;
    if (subject != address(0)) {
      mockId = ethosProfile.profileIdByAddress(subject);
@>    mockId = _addReview(mockId, subject, false, 0x0, ethosProfile);
    } else {
      // convert the service/account to a hash as an identifier
      attestationHash = _getEthosAttestation().getServiceAndAccountHash(
        attestationDetails.service,
        attestationDetails.account
      );
      mockId = ethosProfile.profileIdByAttestation(attestationHash);
      mockId = _addReview(mockId, subject, true, attestationHash, ethosProfile);
    }
    ...
  }

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

  function reviewIdsBySubjectAddress(
    address subjectAddress
  ) external view returns (uint256[] memory) {
    uint256 profileId = _getEthosProfile().profileIdByAddress(subjectAddress);
@>  return reviewIdsBySubjectProfileId[profileId];
  }

Internal pre-conditions

1. An address is created with a mock profileId.
2. The address is later registered into a new profileId.

External pre-conditions

N/A

Attack Path

N/A

Impact

Review ids are not correctly returned for the address, and some reviews would be lost.

PoC

Run the following code in a new file.

import { type HardhatEthersSigner } from '@nomicfoundation/hardhat-ethers/signers';
import { loadFixture } from '@nomicfoundation/hardhat-toolbox/network-helpers';
import { type EthosReview } from '../../typechain-types';
import { createDeployer, type EthosDeployer } from '../utils/deployEthos';
import { type EthosUser } from '../utils/ethosUser';
import { common } from '../utils/common';
import { type EthosProfile } from '../../typechain-types';

describe('EthosReview View Functions', () => {
  let deployer: EthosDeployer;
  let userA: EthosUser;
  let userB: EthosUser;
  let nonEthosUser: HardhatEthersSigner;
  let ethosReview: EthosReview;
  let ethosProfile: EthosProfile;

  beforeEach(async () => {
    deployer = await loadFixture(createDeployer);

    userA = await deployer.createUser();
    userB = await deployer.createUser();
    nonEthosUser = await deployer.newWallet();

    ethosProfile = deployer.ethosProfile.contract;
    ethosReview = deployer.ethosReview.contract;
  });

  describe('reviewIdsBySubjectAddress returns incorrect value', () => {
    it('', async () => {
      // Step 1: UserA creates two review to a completely new address
      {
        await userA.review({ address: nonEthosUser.address });
        await userA.review({ address: nonEthosUser.address });
      }
      // Step 2: Check that `reviewIdsBySubjectAddress()` returns [0, 1].
      {
        const reviewId = await ethosReview.reviewIdsBySubjectAddress(nonEthosUser.address);
        console.log(reviewId);
      }

      // Step 3: Register nonEthosUser address to UserB's profileId.
      {
        const randValue = Math.floor(Math.random() * 1000000);
        const signature = await common.signatureForRegisterAddress(
          nonEthosUser.address,
          userB.profileId.toString(),
          randValue.toString(),
          deployer.EXPECTED_SIGNER,
        );
        await ethosProfile.registerAddress(nonEthosUser.address, userB.profileId, randValue, signature);
      }
      // Step 4: Check that `reviewIdsBySubjectAddress()` returns an empty array [].
      {
        const reviewId = await ethosReview.reviewIdsBySubjectAddress(nonEthosUser.address);
        console.log(reviewId);
      }
    });
  });
});

The output is the following, proving the review ids are lost.

Result(2) [ 0n, 1n ]
Result(0) []

Mitigation

Record the reviewIds by address rather than profileIds.