090.md

Striped Amber Bird

Medium

EthosAttestation.sol :: archiveAttestation()/restoreAttestation() deleted addresses from the profile can still archive and restore attestations.

Summary

The archiveAttestation() and restoreAttestation() are intended to archive or restore attestations. However, the code only verifies if msg.sender is associated with the profile without checking if the address has been removed. This oversight allows removed addresses to still archive or restore attestations.

Root Cause

To illustrate the issue, archiveAttestation() is used here, but the same applies to restoreAttestation(). The implementation is as follows.

function archiveAttestation(bytes32 attestationHash) external whenNotPaused {
    // ensure attestation exists
    Attestation storage attestation = attestationByHash[attestationHash];
    if (attestation.createdAt == 0) {
      revert AttestationNotFound(attestationHash);
    }
    // ensure attestation belongs to sender
    uint256 profileId = attestationByHash[attestationHash].profileId;
@>  bool senderBelongsToProfile = IEthosProfile(_getEthosProfile()).addressBelongsToProfile(
      msg.sender,
      profileId
    );

    if (!senderBelongsToProfile) {
      revert AddressNotInProfile(msg.sender, profileId);
    }

    attestationByHash[attestationHash].archived = true;

    emit AttestationArchived(
      profileId,
      attestation.service,
      attestation.account,
      attestation.attestationId
    );
  }

As shown, the code only verifies if msg.sender is associated with the profile (senderBelongsToProfile) but does not check if the address has been removed. This allows deleted addresses from the profile to archive or restore attestations, which should not be permitted.

Internal pre-conditions

None.

External pre-conditions

None.

Attack Path

None.

Impact

Deleted addresses from the profile can still archive or restore attestations.

PoC

To observe the issue, copy the following POC into EthosAttestation.test.ts.

it('deleted addresses from the profile can archive/restore attestations', async () => {
      const {
        OWNER,
        OTHER_1,
        ethosProfile,
        ethosAttestation,
        SERVICE_X,
        ACCOUNT_NAME_BEN,
        ATTESTATION_EVIDENCE_0,
        EXPECTED_SIGNER,
        OTHER_0,
      } = await loadFixture(deployFixture);

      const randValue = '123';
      const randValue2 = '234';

      //create profile
      await ethosProfile.connect(OWNER).inviteAddress(OTHER_0.address);
      await ethosProfile.connect(OTHER_0).createProfile(1);

      //add a new address to the created profile.
      const signature2 = await common.signatureForRegisterAddress(
        OTHER_1.address,
        "2",
        randValue2,
        EXPECTED_SIGNER,
      );
      await ethosProfile.connect(OTHER_0).registerAddress(OTHER_1.address, 2, randValue2, signature2);

      //remove the previous added address from the profile
      await ethosProfile.connect(OTHER_0).deleteAddressAtIndex(1);

      //create attestation
      const other0profileId = String(await ethosProfile.profileIdByAddress(OTHER_0.address));
      const signature = await common.signatureForCreateAttestation(
        other0profileId,
        randValue,
        ACCOUNT_NAME_BEN,
        SERVICE_X,
        ATTESTATION_EVIDENCE_0,
        EXPECTED_SIGNER,
      );
      await ethosAttestation
        .connect(OTHER_0)
        .createAttestation(
          other0profileId,
          randValue,
          { account: ACCOUNT_NAME_BEN, service: SERVICE_X },
          ATTESTATION_EVIDENCE_0,
          signature,
        );

      const attestationHash = await ethosAttestation.getServiceAndAccountHash(
        SERVICE_X,
        ACCOUNT_NAME_BEN,
      );

      //removed address from the profile can archive attestation
      await ethosAttestation.connect(OTHER_1).archiveAttestation(attestationHash);

      const attestation = await ethosAttestation.attestationByHash(attestationHash);

      expect(attestation.archived).to.be.equal(true, 'Wrong archived');
    });

Mitigation

To address the problem, check if msg.sender is in the profile's addresses; if not, the transaction should revert.

This can be done as follows; however, this approach may not be optimal, as it could lead to a DOS issue if the profile contains many addresses. A more effective solution would be to store the indices of deleted addresses in a mapping for reliable verification.

function archiveAttestation(bytes32 attestationHash) external whenNotPaused {
    // ensure attestation exists
    Attestation storage attestation = attestationByHash[attestationHash];
    if (attestation.createdAt == 0) {
      revert AttestationNotFound(attestationHash);
    }
    // ensure attestation belongs to sender
    uint256 profileId = attestationByHash[attestationHash].profileId;
    bool senderBelongsToProfile = IEthosProfile(_getEthosProfile()).addressBelongsToProfile(
      msg.sender,
      profileId
    );

    if (!senderBelongsToProfile) {
      revert AddressNotInProfile(msg.sender, profileId);
    }

+   address[] memory currentAddresses = IEthosProfile(_getEthosProfile()).addressesForProfile(profileId);
    
+   for(uint256 i = 0; i < currentAddresses.length(); i++) {
+     if (currentAddresses[i] == msg.sender) {
+       break;
+     } else if (i == currentAddresses.length() - 1) {
+       revert;
+     }
+   }

    attestationByHash[attestationHash].archived = true;

    emit AttestationArchived(
      profileId,
      attestation.service,
      attestation.account,
      attestation.attestationId
    );
  }