unforgiven - middle level admins can take control of the tree after unlinking it because function unlinkTopHatFromTree() doesn't reset the value of the linkedTreeRequests[]

[sherlock-admin]

unforgiven

high

middle level admins can take control of the tree after unlinking it because function unlinkTopHatFromTree() doesn't reset the value of the linkedTreeRequests[]

Summary

function unlinkTopHatFromTree() unlink a Tree from the parent tree but it doesn't reset the value of the linkedTreeRequests[] for the tree. this can give ability for middle level admins to take control of the unlinked tree after unlinking it and giving admin permission to the topHat of the tree.

Vulnerability Detail

This is unlinkTopHatFromTree() code:

    function unlinkTopHatFromTree(uint32 _topHatDomain) external {
        uint256 fullTopHatId = uint256(_topHatDomain) << 224; // (256 - TOPHAT_ADDRESS_SPACE);
        _checkAdmin(fullTopHatId);

        delete linkedTreeAdmins[_topHatDomain];
        emit TopHatLinked(_topHatDomain, 0);
    }

As you can see it doesn't reset the value of the linkedTreeRequests[_topHatDomain]. This would give ability for middle level admin to take control of the unlinked tree by calling approveLinkTopHatToTree() after getting unlinked. This is the steps:

1. User1 which is middle admin of the Tree1 wants to steal it and make it child of MaliciousTree1. User1 can't perform this action right now because Tree1 is child of Tree2 and also User1 is not the admin of the root hat of the Tree1.
2. User2 which is higher level of the Tree1 wants to unlink Tree1 and then Tree1 become his own admin.
3. User1 would see User2 transaction in the mempool and would perform a sandwich attack to steal the Tree1.
4. User1 would create a transaction which calls requestLinkTopHatToTree(Tree1, MaliciousTree1) with high gas price and because he is middle level admin of the Tree1 this transaction won't fail.
5. and User1 would create another transaction which calls approveLinkTopHatToTree(Tree1, MaliciousTree1).
6. now first requestLinkTopHatToTree() get executed and code would set linkedTreeRequests[Tree1] = MaliciousTree1 and then unlinkTopHatFromTree() get executed and code would set linkedTreeAdmins[Tree1] = 0x0 and then approveLinkTopHatToTree() would get executed and code would set new admin for Tree1 and User1 would steal the Tree1 when upper level admin trying to unlink it.

the attack is possible without the sandwich attack, malicious middle level admin can perform this alone without waiting for upper level admin to call unlink. in this scenario we assumed that only upper level can unlink a tree because of another issue in the unlink logic which if it was fixed only upper level should unlink a tree. the point is even if you limit unlinking access for upper level admins because code won't reset linkedTreeRequests[] when unlinking so middle level admins can put malicious values in it and after unlinking, they would link the tree to malicious value. (they can link before unlinking because of the tippy check)

Impact

see summery or detail

Code Snippet

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

Tool used

Manual Review

Recommendation

in unlinkTopHatFromTree() remove the value of the linkedTreeRequests[_topHatDomain] too.

Duplicate of #35