011.md

Active Taffy Hornet

Medium

Re-registering a previously deleted address leads to incorrect checkMaxAddresses check for subsequent address registrations

Summary

The EthosProfile::checkMaxAddresses function considers the total associated addresses as a sum of currently associated addresses with previously removed addresses.

  function checkMaxAddresses(uint256 profileId) internal view {
    uint256 sum = profiles[profileId].addresses.length;       <@ // currently associated addresses
    sum += profiles[profileId].removedAddresses.length;    <@ // previously removed addresses
    if (sum > maxNumberOfAddresses) {
      revert MaxAddressesReached(profileId);
    }
  }

The variable maxNumberOfAddresses depicts the number of addresses that can be associated with a single profile.

In a scenario where a

1. User registers an address X

  checkMaxAddresses(X) == 2
  // Because 
  function registerAddress(
    address addressStr,
    uint256 profileId,
    uint256 randValue,
    bytes calldata signature
  ) external whenNotPaused onlyNonZeroAddress(addressStr) {
    // Rest of the code
    validateAndSaveSignature(
      _keccakForRegisterAddress(addressStr, profileId, randValue),
      signature
    );

    profiles[profileId].addresses.push(addressStr); <@ // added to addresses
    profileIdByAddress[addressStr] = profileId;
    // Rest of the code
  }

2. User deletes the same address X

  checkMaxAddresses(X) == 2
  // Because, 
  function _deleteAddressAtIndexFromArray(
    uint256 index,
    address[] storage addresses,
    address[] storage removedAddresses
  ) private {
    address addr = addresses[addresses.length - 1];
    addresses[index] = addr;
    removedAddresses.push(addr); <@ // added to removedAddresses 
    addresses.pop(); // Removed from addresses
  }

3. User wants to re-register X back

  checkMaxAddresses(X) == 3
  // Because, we still count the removedAddresses array which contains the same address X
  sum += profiles[profileId].removedAddresses.length;    <@ // previously removed addresses contains the same address that we are re-registering

This would mean that a user even though has associated only 1 extra address, his check against maxNumberOfAddresses comes down to 3 for the next registration which is incorrect as technically the user has associated only 1 extra address.

Root Cause

The EthosProfile::checkMaxAddresses function does not properly calculate for addresses that are re-registered by the same user.

Internal pre-conditions

1. User needs to have a valid ethos profile

External pre-conditions

No response

Attack Path

1. The user registers an address X.
2. Later, he decides to remove the address X using the deleteAddressAtIndex function
3. The user realises the need to once again register the same address X again, so he calls the registerAddress with the same address, but now the checkMaxAddresses would be still consider the old removedAddresses even though the user is associating the same address as before.

Impact

This would lead to incorrect checkMaxAddresses where even though the user has only ever associated a single address with his profile, the check would be made against an inflated checkMaxAddresses. For example, if the limit of maxNumberOfAddresses is set to 100 and for some reason the user registers and unregisters the same address 100 times, he would be denied to register any address going further even though technically he was involved with only 1 address during his entire protocol's user journey.

PoC

The below test case was added in the EthosProfile.test.ts file

  describe('registerAddress', () => {
    // other test cases
    it('re-registering the same address should lead to inflated checkMaxAddresses', async () => {
      const { ethosProfile, PROFILE_CREATOR_0, EXPECTED_SIGNER, OTHER_0, OWNER, ADMIN } =
        await loadFixture(deployFixture);

      const profileId = String(2);
      let rand = '123';
      const signature = await common.signatureForRegisterAddress(
        OTHER_0.address,
        profileId,
        rand,
        EXPECTED_SIGNER,
      );
      // set max limit as 2 for testing purposes
      await ethosProfile.connect(ADMIN).setMaxAddresses(2);

      await ethosProfile.connect(OWNER).inviteAddress(PROFILE_CREATOR_0.address);
      await ethosProfile.connect(PROFILE_CREATOR_0).createProfile(1);

      await ethosProfile
        .connect(PROFILE_CREATOR_0)
        .registerAddress(OTHER_0.address, profileId, rand, signature);

      const adddresses = await ethosProfile.addressesForProfile(2);
      expect(adddresses.length).to.be.equal(2, 'should be 2 addresses');
      
      // De-register the address
      await ethosProfile.connect(PROFILE_CREATOR_0).deleteAddressAtIndex(1);

      rand = "1234"; // Changing the random value here
      const signature2 = await common.signatureForRegisterAddress(
        OTHER_0.address,
        profileId,
        rand,
        EXPECTED_SIGNER,
      );
      await expect(ethosProfile
        .connect(PROFILE_CREATOR_0)
        .registerAddress(OTHER_0.address, profileId, rand, signature2)
      ).to.be.revertedWithCustomError(ethosProfile, 'MaxAddressesReached') // reverts as expected even though the extra associated address till now was only 1
        .withArgs(2);
      
    });
  });

Mitigation

Consider looping through the removedAddresses array and pop() the address which is being re-registered.