PNS - The incorrect counting of profile addresses wrongly limits their total number

[sherlock-admin3]

PNS

Medium

The incorrect counting of profile addresses wrongly limits their total number

Summary

Each profile has two lists of addresses, added to it and active, and deleted. After the user deletes the address, he can add it again.

The problem will appear in the calculation of limits, because the re-added address appears on two lists, which is why it is counted twice towards the limit and unfairly deprives the user of the possibility of adding another address.

Root Cause

After re-register the address, it is not removed from the removed list, thus unfairly increasing the limit and limiting the user.

Internal pre-conditions

• user removes the address previously added to the profile

External pre-conditions

No response

Attack Path

No response

Impact

By blocking a user unfairly due to an incorrectly calculated total, the function of registering new addresses may be unfairly blocked earlier.

PoC

• on line 388 you can see that you can add the same address again,
• on line 403 the address is added again, but it is not removed from the deleted list
• on line 406 the limit is checked

File: ethos/packages/contracts/contracts/EthosProfile.sol
  373:   function registerAddress(
  374:     address addressStr,
  375:     uint256 profileId,
  376:     uint256 randValue,
  377:     bytes calldata signature
  378:   ) external whenNotPaused onlyNonZeroAddress(addressStr) {
  379:     (bool verified, bool archived, bool mock) = profileStatusById(profileId);
  380:     if (!verified) {
  381:       revert ProfileNotFound(profileId);
  382:     }
  383:     if (archived || mock) {
  384:       revert ProfileAccess(profileId, "Profile is archived");
  385:     }
  386:     // you may restore your own previously deleted address,
  387:     // but you cannot register an address that has been deleted by another user
  388:     if (profileIdByAddress[addressStr] != profileId && isAddressCompromised[addressStr]) { //audit
  389:       revert AddressCompromised(addressStr);
  390:     }
  391:     (bool addressAlreadyRegistered, , , uint256 registeredProfileId) = profileStatusByAddress(
  392:       addressStr
  393:     );
  394:     if (addressAlreadyRegistered && registeredProfileId != profileId) {
  395:       revert ProfileExistsForAddress(addressStr);
  396:     }
  397: 
  398:     validateAndSaveSignature(
  399:       _keccakForRegisterAddress(addressStr, profileId, randValue),//audit:info:hash collision jest niemozliwe, bo wszystkie parametry maja stala dlugosc, nie sa dynamiczne jak string czy bytes
  400:       signature
  401:     );
  402: 
  403:     profiles[profileId].addresses.push(addressStr); //audit: dodaje ten sam adres, który usunąłem i checkMaxAddresses() liczy go podwujnie bo jest i w usunietych i w aktywnych i zmniejsza nieslusznie limit userowi
  404:     profileIdByAddress[addressStr] = profileId;
  405: 
  406:     checkMaxAddresses(profileId); //sprawdza sume usunietych i aktywnych czy nie wieksza od maxNumberOfAddresses
  407: 
  408:     emit AddressClaim(profileId, addressStr, AddressClaimStatus.Claimed);
  409:   }

EthosProfile.registerAddress

File: ethos/packages/contracts/contracts/EthosProfile.sol
  732:   function checkMaxAddresses(uint256 profileId) internal view {
  733:     uint256 sum = profiles[profileId].addresses.length;
  734:     sum += profiles[profileId].removedAddresses.length;
  735:     if (sum > maxNumberOfAddresses) {
  736:       revert MaxAddressesReached(profileId);
  737:     }
  738:   }

Mitigation

You must remove the address from the deleted list in order to correctly calculate the total.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/trust-ethos/ethos/pull/1836