33, 35, 116 - tree linking fixes

[spengrah]

sherlock-audit/2023-02-hats-judging#33
sherlock-audit/2023-02-hats-judging#35
sherlock-audit/2023-02-hats-judging#116

[zobront]

Fixes for 33, 35 both look great.

For 116, couldn't the same attack described be done without the relink functionality?

For example, I could create the new root, approve that as the new link, transfer it over to that, and then unlink? I don't think relink is needed for the attack to work.

I can't really see a fix for that. Honestly, I'm not sure this is an issue though. It seems to be part of the expected delegation flow with the required trust assumptions for admins.

[spengrah]

For example, I could create the new root, approve that as the new link, transfer it over to that, and then unlink? I don't think relink is needed for the attack to work.

@zobront I think there's an important difference between stealing a tree you yourself create and stealing a tree created by somebody else and linked to your hat. Or maybe I'm misunderstanding your message.

[zobront]

@spengrah I think you're misunderstanding.

Let's take the exact scenario in the report:

Root0 (admin0) -> H1 (Admin1) -> H2 (Admin2) -> Root3 (Admin3) -> H4 (Admin4)

now Admin2 wants to remove (Root3 -> H4) tree from higher level admins(Admin1) access.

Why couldn't Admin2 do the same attack without relink functionality?

• Admin2 would create new TopHat Root1 and link it to H2. (H2 -> Root1).
• now Admin2 would change the link of Root3 from H2 to Root1 by calling requestLinkTopHatToTree(Root3, Root1) and then approveLinkTopHatToTree(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: (Root0 -> H1 -> H2 -> Root1 -> Root3 -> H4)
• then they could unlink Root1 as outlined in the report

In order words, I don't think there's anything special about the relink functionality that is needed for this attack. The exact same thing can happen in two steps via request and approve.

[spengrah]

@zobront OK, I understand now what you're saying. And I agree, the same attack is possible with both request/approve and relink, which makes sense since the latter is just a shortcut for scenarios where the same admin has request and approve rights for the same tophat domain.

One way to address this would be to add the same protections as we've just added to relink to approve as well, on the condition that there is an existing link. Could probably move it to _link...() to cover both cases.

[spengrah]

And of course the other approach would to accept and document the risk.