sherlock-audit / 2024-10-ethos-network-judging

0 stars 0 forks source link

heeze - Wrong address added to the removedAddresses array field of the profile data #169

Open sherlock-admin2 opened 2 weeks ago

sherlock-admin2 commented 2 weeks ago

heeze

High

Wrong address added to the removedAddresses array field of the profile data

Summary

When an address is removed from a profile it is added to the removedAddresses field of the profile data for historical tracking. However, the address added to the removedAddresses array is not the address being removed.

Root Cause

Wrong address is added to the removedAddress array field of the profile data when deleting an address from a profile. https://github.com/sherlock-audit/2024-10-ethos-network/blob/main/ethos/packages/contracts/contracts/EthosProfile.sol#L591

Internal pre-conditions

External pre-conditions

No response

Attack Path

Impact

The addresses in the removedAddresses array for a profile will be incorrect, leading to wrong data being returned and used off-chain.

PoC

    it('POC -- address in removedAddresses not correct', async () => {
      const { ethosProfile, PROFILE_CREATOR_0, EXPECTED_SIGNER, OTHER_0, OTHER_1, OWNER } =
        await loadFixture(deployFixture);

      const profileId = String(2);
      let rand = '123';
      let signature = await common.signatureForRegisterAddress(
        OTHER_0.address,
        profileId,
        rand,
        EXPECTED_SIGNER,
      );

      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);

      rand = '456';
      signature = await common.signatureForRegisterAddress(
        OTHER_1.address,
        profileId,
        rand,
        EXPECTED_SIGNER,
      );
      await ethosProfile
        .connect(PROFILE_CREATOR_0)
        .registerAddress(OTHER_1.address, profileId, rand, signature);

      let addresses = await ethosProfile.addressesForProfile(2);
      expect(addresses.length).to.be.equal(3, 'should be 3 addresses');

      // delete address at index 1
      await ethosProfile.connect(PROFILE_CREATOR_0).deleteAddressAtIndex(1);

      addresses = await ethosProfile.addressesForProfile(2);
      expect(addresses.length).to.be.equal(2, 'should be 2 addresses');
      expect(addresses[0]).to.be.equal(PROFILE_CREATOR_0.address, 'wrong address[0]');
      expect(addresses[1]).to.be.equal(OTHER_1.address, 'wrong address[1]');

      const removedAddresses = (await ethosProfile.getProfile(2)).removedAddresses;
      //the removedAddresses contains OTHER_1.address but the address removed is OTHER_0.address
      expect(removedAddresses[0]).to.be.equal(addresses[1]);
      expect(removedAddresses[0]).to.be.equal(OTHER_1.address);
    });

Mitigation

Add the correct address being removed to the removedAddresses array

           //..... function header
+          address addressToRemove = addresses[index];
             address addr = addresses[addresses.length - 1];
              addresses[index] = addr;
-            removedAddresses.push(addr);
+           removedAddresses.push(addressToRemove);
              addresses.pop();
sherlock-admin2 commented 2 weeks ago

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