095.md

Cheerful Opal Viper

Medium

A missing check in EthosReview::archiveReview() allows deleted addresses to archive reviews

Summary

When archiving a review, there is no check to verify if the msg.sender is deleted or marked as compromised.

As a result, a compromised address can still archive a review even if it has been deleted.

Vulnerability Detail

In the EthosReview::addReview() function, the author of the review is set as the address that called the function.

  function addReview(
    Score score,
    address subject,
    address paymentToken,
    string calldata comment,
    string calldata metadata,
    AttestationDetails calldata attestationDetails
  ) external payable whenNotPaused {
    ... ...
    reviews[reviewCount] = Review({
      archived: false,
      score: score,
      authorProfileId: authorProfileId,
      author: msg.sender,
      subject: subject,
      reviewId: reviewCount,
      // solhint-disable-next-line not-rely-on-time
      createdAt: block.timestamp,
      comment: comment,
      metadata: metadata,
      attestationDetails: attestationDetails
    });
    ... ...
  }

In the EthosReview::archiveReview() function, a check ensures that the calling address is the author.

  function archiveReview(uint256 reviewId) external whenNotPaused {
    ... ...
    if (review.author != msg.sender) {
      revert UnauthorizedArchiving(reviewId);
    }
    ... ...
  }

However, it is not checked whether the author has been deleted beforehand. This means that a compromised address will still be able to archive any review it has added.

PoC

The following test should be added in EthosReview.test.ts:

    it('should set review as archived if address has been deleted', async () => {
      const { ethosReview, ADMIN, REVIEW_CREATOR_0, REVIEW_SUBJECT_0, OWNER, ethosProfile, OTHER_0, EXPECTED_SIGNER, } =
        await loadFixture(deployFixture);

      const reviewPrice = ethers.parseEther('1.23456789');
      await allowPaymentToken(ADMIN, ethosReview, ethers.ZeroAddress, true, reviewPrice);

      const params = {
        score: Score.Positive,
        subject: REVIEW_SUBJECT_0.address,
        paymentToken: ethers.ZeroAddress,
        comment: defaultComment,
        metadata: defaultMetadata,
        attestationDetails: {
          account: '',
          service: '',
        } satisfies AttestationDetails,
      };

      const profileId = String(2);
      let rand = '123';
      let signature = await common.signatureForRegisterAddress(
        OTHER_0.address,
        profileId,
        rand,
        EXPECTED_SIGNER,
      );

      await ethosProfile.connect(OWNER).inviteAddress(REVIEW_CREATOR_0.address);
      await ethosProfile.connect(REVIEW_CREATOR_0).createProfile(1);
      await ethosProfile
        .connect(REVIEW_CREATOR_0)
        .registerAddress(OTHER_0.address, profileId, rand, signature);

      let addresses = await ethosProfile.addressesForProfile(2);
      expect(addresses.length).to.be.equal(2, 'should be 2 addresses');

      await ethosReview
        .connect(REVIEW_CREATOR_0)
        .addReview(
          params.score,
          params.subject,
          params.paymentToken,
          params.comment,
          params.metadata,
          params.attestationDetails,
          { value: reviewPrice },
        );

      let review = await ethosReview.reviews(0);
      expect(review.archived).to.equal(false, 'wrong before');

      await ethosProfile.connect(OTHER_0).deleteAddressAtIndex(0);

      addresses = await ethosProfile.addressesForProfile(2);
      expect(addresses.length).to.be.equal(1, 'should be 1 address');
      expect(addresses[0]).to.be.equal(OTHER_0.address, 'wrong address[0]');

      await ethosReview.connect(REVIEW_CREATOR_0).archiveReview(0);

      review = await ethosReview.reviews(0);
      expect(review.archived).to.equal(true, 'wrong after');
    });

Impact

A compromised address can archive any review made by that address even if it has been deleted from the profile.

Code Snippet

EthosReview.sol#L287-307

Tool Used

Manual Review

Recommendation

Consider adding a check to ensure that msg.sender has not been deleted.