Top Hats can be overriden due to arithmetic overflow

Summary

In some circumstances, a top hat can be taken over by an anonymous user and consequently an entire hierarchy of permissions can be hijacked.

Vulnerability Detail

The top hat id generator is using bit shifts to compute the next top had id to be assigned to a top hat. However, this logic does not check for arithmetic overflow and might just overflow and reuse an already assigned top hat id.

The logic for computing the hat id is as follows:

topHatId = uint256(++lastTopHatId) << 224;

However, executing uint256(x) << 224 with x = 4294967297, i.e. 2**32+1, returns the top hat id 1 which is then minted again for the new msg.sender because neither _createHat nor _mintHat would revert in this case.

Reaching 4 billion top hats might be unlikely but for future proofing the protocol I would advise to check whether the max has been reached and revert if that's the case. Indeed, we cannot take assumptions on the cost of the attack vs the amount at risk (for instance a hacker could mint the top hats remaining before reaching 2^32 given that the cost to do that is worth the potential gain, i.e, hijacking the permissions of a protocol with high market cap).

Impact

An anonymous user could take control of a top hat.

Code Snippet

https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/Hats.sol#L117

Tool used

Manual Review

Recommendation

Revert if the limit has been reached for future proofing the protocol.

sherlock-audit / 2023-02-hats-judging

clems4ever - Top Hats can be overriden due to arithmetic overflow #62