049.md

Cheery Mustard Swallow

High

Potential Race Conditions in EthosVouch::vouchByProfileId can lead to loss of funds, future rewards and vouch related data for users

Summary

A critical race condition exists in EthosVouch::vouchByProfileId where the global state variable vouchCount is used as the key for storing vouches in the vouches mapping. During high traffic periods, multiple transactions can observe the same stale vouchCount value in the mempool. When these transactions execute in subsequent blocks, the later transaction will overwrite the earlier transaction's vouch data at the same mapping position, resulting in permanent vouch data loss and inconsistent protocol state. This is possible because the function lacks checks to prevent multiple vouches from using the same vouchCount value as their mapping key.

Root Cause

The vulnerability stems from the use of a global vouchCount state variable as a vouchId key for storing vouch data in the vouches mapping without a strict check on whether the vouchId already exists and belongs to a different authorProfile when writing to state. When multiple transactions attempt to create vouches simultaneously or in the same timeframe by calling vouchByProfileId() they may read the same vouchCount value before either transaction is mined, leading to a collision/overwriting in the vouches mapping.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. Initial state: vouchCount = 5
2. Transaction A from Alice reads vouchCount as 5
3. Transaction B from Bob reads vouchCount as 5
4. Transaction A gets mined: Creates vouches[5] with Alice's data Updates associated mappings Increments vouchCount to 6
5. Transaction B gets mined: Overwrites vouches[5] with Bob's data and creates chaos within the system.

Impact

1. Data Loss: The first vouch's data is permanently lost when overwritten
2. Inconsistent State: Array mappings (vouchIdsByAuthor, vouchIdsForSubjectProfileId) contain inconsistent references
3. Missing Vouches: Only one vouch is recorded when two should exist
4. Fund Loss Risk: The funds tied to vouch data, could get misattributed or lost
5. Trust System Corruption: Compromises the entire vouch system

PoC

While simulating concurrent vouchByProfileId transactions in the mempool is complex, examining the function's implementation reveals the vulnerability:

 function vouchByProfileId(
    uint256 subjectProfileId,
    string calldata comment,
    string calldata metadata
  ) public payable whenNotPaused nonReentrant {
    // validate author profile
    uint256 authorProfileId = IEthosProfile(
      contractAddressManager.getContractAddressForName(ETHOS_PROFILE)
    ).verifiedProfileIdForAddress(msg.sender);

    // pls no vouch for yourself
    if (authorProfileId == subjectProfileId) {
      revert SelfVouch(authorProfileId, subjectProfileId);
    }

    // users can't exceed the maximum number of vouches
    if (vouchIdsByAuthor[authorProfileId].length >= maximumVouches) {
      revert MaximumVouchesExceeded(
        vouchIdsByAuthor[authorProfileId].length,
        "Exceeds author vouch limit"
      );
    }

    // validate subject profile
    if (subjectProfileId == 0) {
      revert InvalidEthosProfileForVouch(subjectProfileId);
    }
    (bool verified, bool archived, bool mock) = IEthosProfile(
      contractAddressManager.getContractAddressForName(ETHOS_PROFILE)
    ).profileStatusById(subjectProfileId);

    // you may not vouch for archived profiles
    // however, you may vouch for verified AND mock profiles
    // we allow vouching for mock profiles in case they are later verified
    if (archived || (!mock && !verified)) {
      revert InvalidEthosProfileForVouch(subjectProfileId);
    }

    // one vouch per profile per author
    _vouchShouldNotExistFor(authorProfileId, subjectProfileId);

    // don't exceed maximum vouches per subject profile
    if (vouchIdsForSubjectProfileId[subjectProfileId].length >= maximumVouches) {
      revert MaximumVouchesExceeded(
        vouchIdsForSubjectProfileId[subjectProfileId].length,
        "Exceeds subject vouch limit"
      );
    }

    // must meet the minimum vouch amount
    if (msg.value < configuredMinimumVouchAmount) {
      revert MinimumVouchAmount(configuredMinimumVouchAmount);
    }

    (uint256 toDeposit, ) = applyFees(msg.value, true, subjectProfileId);

    // store vouch details
    uint256 count = vouchCount;
    vouchIdsByAuthor[authorProfileId].push(count);
    vouchIdsByAuthorIndex[authorProfileId][count] = vouchIdsByAuthor[authorProfileId].length - 1;
    vouchIdsForSubjectProfileId[subjectProfileId].push(count);
    vouchIdsForSubjectProfileIdIndex[subjectProfileId][count] =
      vouchIdsForSubjectProfileId[subjectProfileId].length -
      1;

    vouchIdByAuthorForSubjectProfileId[authorProfileId][subjectProfileId] = count;
    vouches[count] = Vouch({
      archived: false,
      unhealthy: false,
      authorProfileId: authorProfileId,
      authorAddress: msg.sender,
      vouchId: count,
      balance: toDeposit,
      subjectProfileId: subjectProfileId,
      comment: comment,
      metadata: metadata,
      activityCheckpoints: ActivityCheckpoints({
        vouchedAt: block.timestamp,
        unvouchedAt: 0,
        unhealthyAt: 0
      })
    });

    emit Vouched(count, authorProfileId, subjectProfileId, msg.value);
    vouchCount++;
  }

It is clear to see that there is no explicit check similar to

if (vouches[vouchId].authorAddress != msg.sender) {
      revert AddressNotVouchAuthor(vouchId, msg.sender, vouches[vouchId].authorAddress);
    }

that is used in functions like unvouch although for this function it might look like

if (vouches[count].authorAddress != address(0)) {
    revert someError;
}

A simple way to test this, is to comment out this if statement: if (vouches[vouchId].authorAddress != msg.sender) { revert AddressNotVouchAuthor(vouchId, msg.sender, vouches[vouchId].authorAddress); } from unvouch and then run a test like

it('should allow any address to unvouch due to missing authorization check', async () => {
    // Create vouch from userA to userB
    await userA.vouch(userB);
    const vouch = await ethosVouch.verifiedVouchByAuthorForSubjectProfileId(
      userA.profileId,
      userB.profileId,
    );

    // Create a random attacker address that has no relation to the vouch
    const attackerWallet = await deployer.newWallet();

    // Attacker should be able to call unvouch() successfully
    // Even though they aren't the vouch author
    await expect(ethosVouch.connect(attackerWallet).unvouch(vouch.vouchId))
      .to.not.be.reverted;

    // Verify the vouch was actually unvouched
    const updatedVouch = await ethosVouch.vouches(vouch.vouchId);
    expect(updatedVouch.activityCheckpoints.unvouchedAt).to.be.greaterThan(0);
    expect(updatedVouch.archived).to.be.true;
});

The lack of explicit checks in vouchByProfileId, which uses vouchCount as the vouchId to register vouches, means any caller can potentially overwrite values in the vouches mapping under the right circumstances. In high-traffic scenarios, a second caller could overwrite another's vouch data due to the stale vouchCount value.

Mitigation

1. Add explicit checks that the vouchId exists and belongs to a different author at runtime, and cause the function call to revert e.g

if (vouches[count].authorAddress != address(0)) {
    revert someError;
}

2. Consider implementing a vouchByProfileId transactions queue for potential high-traffic periods.