unforgiven - middle level admins can steal child trees because function unlinkTopHatFromTree() is callable by them

[sherlock-admin]

unforgiven

high

middle level admins can steal child trees because function unlinkTopHatFromTree() is callable by them

Summary

in normal scenario middle level admins can't relink a child tree to another tree and when code link a tree it checks that the old and new admin is in the same tree(have same tippy hat). but middle level admins can bypass this buy using unlink logic.

Vulnerability Detail

a middle level admin can perform this actions:
(x -> y) means x is admin of y (y linked to x)

1. suppose we have this tree: Root0 (admin0) -> H1 (Admin1) -> H2 (Admin2) -> Root3 (Admin3) -> H4 (Admin4). (H1, H2, H4 are hats and Root0 is tophat and Root3 is "root tree")
2. now Admin2 wants to remove (Root3 -> H4) tree from higher level admins(Admin1) access.
3. Admin2 would create new TopHat Root1 and link it to H2. (H2 -> Root1).
4. now Admin2 would change the link of Root3 from H2 to Root1 by calling relinkTopHatWithinTree(Root3, Root1). because Admin2 is admins of the H2 and both Root3 and Root1 is linked to the H2 so this action would be possible. the tree would become: (Root2 -> H1 -> H2 -> Root1 -> Root3 -> H4)
5. now Admin2 would call unlinkTopHatFromTree(Root3) and unlink Root1 from Root0 tree. because Admin2 created Root1 he would become admin of the Root1 and would be only one that have admin access to Root3. (Root1 -> Root3 -> H4)

simple unlinking can't always make middle level admin to be new admin so middle level admin should perform one relink and then unlink.

Impact

middle level admin can steal the child trees and become the solo admin of them

Code Snippet

https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/Hats.sol#L726-L732
https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/Hats.sol#L739-L755

Tool used

Manual Review

Recommendation

This is a logical issue and fixing it is a little hard. one fix is that only allow the tippy hat admin to unlink a child tree. one another solution is only allow relink if the child tree to upper level trees not another sub level tree. (the path from the tree to the tippy hat)

[spengrah]

This is a good one, and a tough one! I can see a few potential resolutions (including those recommended by op):

1. Only allow tippy top hats to relink child trees
2. Only allow tippy top hats to unlink child trees
3. Only allow relinking if the destination is the main tree
4. Remove relinking altogether and require that relinking be done in three steps: a) admin unlinks, b) unlinked top hat requests new link, c) destination admin approves new link

Not yet sure which is best...will thinking more about this.

[spengrah]

Likely going with option 3, cc @zobront

[zobront]

@spengrah I agree with this. Best solution to me seems to be continuing to allow anyone to relink but checking that tippy top hat of sending tree and receiving tree are the same. Is that what you have in mind to enforce option 3?

[spengrah]

I agree with this. Best solution to me seems to be continuing to allow anyone to relink but checking that tippy top hat of sending tree and receiving tree are the same. Is that what you have in mind to enforce option 3?

@zobront actually that is not quite what I was thinking, but it's better, so now its what I'm thinking :)

[spengrah]

checking that tippy top hat of sending tree and receiving tree are the same

Actually, this is not sufficient since the described attack begins under the same tippy top hat. I think what we need to do instead is check that one or more of the following is true:

1. destination is the same local tree as origin, ie by checking that they both share the same local tophat
2. destination is the tippy top hat's local tree, ie by checking that the tippy tophat is equal to the local tophat for the destination
3. caller wears the tippyTopHat

cc @zobront

[spengrah]

Hats-Protocol/hats-protocol#113

[zobront]

Escalate for 10 USDC

This is a valid issue but the severity is wrong.

A core assumption of the protocol is that admins are trustable. If we start questioning the admin trust assumptions, a whole new field of bugs opens up.

With that in mind, it does feel like users would assume that if they are higher up the tree, they wouldn't be able to lose control of a sub hat. While they are giving complete control of the subhat to the mid-level admin, it seems like a fair assumption that that user wouldn't be able to remove their ownership over the hat.

For that reason, I think it's fair to call this a Medium, but it's definitely not a High, as the only possible paths for exploit are through a trusted address.

[sherlock-admin]

Escalate for 10 USDC

This is a valid issue but the severity is wrong.

A core assumption of the protocol is that admins are trustable. If we start questioning the admin trust assumptions, a whole new field of bugs opens up.

With that in mind, it does feel like users would assume that if they are higher up the tree, they wouldn't be able to lose control of a sub hat. While they are giving complete control of the subhat to the mid-level admin, it seems like a fair assumption that that user wouldn't be able to remove their ownership over the hat.

For that reason, I think it's fair to call this a Medium, but it's definitely not a High, as the only possible paths for exploit are through a trusted address.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Given the complexity around the user admin structure and the permissions, and the preconditions for this attack to happen,
considering this issue a valid medium.

[sherlock-admin]

Escalation accepted

Given the complexity around the user admin structure and the permissions, and the preconditions for this attack to happen,
considering this issue a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[sherlock-admin]

Escalation accepted

Given the complexity around the user admin structure and the permissions, and the preconditions for this attack to happen,
considering this issue a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[MLON33]

zobront added "Fix Approved" label