obront - Owners of linkedin tophats cannot have eligibility revoked

[sherlock-admin]

obront

medium

Owners of linkedin tophats cannot have eligibility revoked

Summary

Tophats that are linked to new admins are supposed to behave like normal hats, rather than tophats, but their eligibility variable will be permanently set to address(0). This turns off the badStanding functionality, which allows all owners of the hat to be immune to punishments from their admins.

Vulnerability Detail

When a Tophat is linked to a new admin, the assumption is that it will behave like any other immutable hat. From the docs:

a tophat that has been linked (aka grafted) onto another hat tree is no longer considered a tophat

However, while most hats are not able to have their toggle or eligibility set to the zero address (which is carefully checked against in createHat()), tophats will always have them set to the zero address.

Since these hats are immutable, even when they are linked, there is no way to change the values of these two variables. They are permanently set to address(0).

This is especially problematic for eligibility, as even the local value of badStandings that can be used if the event that there is no external contract is set in _processHatWearerStatus(), which can only be called if eligibility is either an EOA that calls setHatWearerStatus() or a contract that implements the IHatsEligibility interface.

This causes major problems for admins of linked tophats, as while they are expected to behave like other immutable hats, instead will not give the admin the privileges they need.

Impact

Admins controlling linked tophats cannot turn off the eligibility of wearers who misbehave, giving tophat owners extra power and breaking the principle that linked tophats are no longer considered tophats.

Code Snippet

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

Tool used

Manual Review

Recommendation

Create the ability for linked tophats to change their toggle and eligibility addresses one time upon being linked. This could be accomplished with an extra bit value in the config storage, which could be set and unset when tophats are linked and unlinked.

[spengrah]

The reason I didn't originally include something like this is that it could introduce weird behavior as trees are unlinked. If we allow eligibility and toggle modules to be added to linked tophats, then they'd need to be cleared if the tree is unlinked. But maybe that's actually not so bad.

We could even modify approveLinkTopHatToTree (and maybe relinkTopHatToTree?) to take toggle and eligibility parameters that (if non-zero) change a linked tophat's modules once on linking. I think this would forgo the need to write/read from a config bit.

When unlinked, those values would be reset to address(0).

Note: since relinkTopHatToTree can be called with the same origin and destination — ie to not move the topHat — under the above logic admins could use that function to change a linked topHat's eligibility and toggle an arbitrary number of times. I actually think this is fine, but something to note.

@zobront how does that sound?

[zobront]

@spengrah This sounds right to me. Part of linking and unlinking should be all the setup / wind down needed to turn it frmo tophat to normal hat and back. Your solution sound right.

[MLON33]

zobront added "Fix Approved" label

[jacksanford1]

spengrah added the fix in this PR for reference: https://github.com/Hats-Protocol/hats-protocol/pull/113