This repository has been archived by the owner on Jun 11, 2023. It is now read-only.
obront - Unlinked tophat retains linkedTreeRequests, can be rugged #35
Labels
Fix Approved
Has Duplicates
A valid issue with 1+ other issues describing the same vulnerability
Hats.sol
High
A valid High severity issue
Reward
A payout will be made for this issue
Sponsor Confirmed
The sponsor acknowledged this issue is valid
Will Fix
The sponsor confirmed this issue will be fixed
obront
high
Unlinked tophat retains linkedTreeRequests, can be rugged
Summary
When a tophat is unlinked from its admin, it is intended to regain its status as a tophat that is fully self-sovereign. However, because the
linkedTreeRequests
value isn't deleted, an independent tophat could still be vulnerable to "takeover" from another admin and could lose its sovereignty.Vulnerability Detail
For a tophat to get linked to a new tree, it calls
requestLinkTopHatToTree()
function:This creates a "request" to link to a given admin, which can later be approved by the admin in question:
This function shows that if there is a pending
linkedTreeRequests
, then the admin can use that to link the tophat into their tree and claim authority over it.When a tophat is unlinked, it is expected to regain its sovereignty:
However, this function does not delete
linkedTreeRequests
.Therefore, the following set of actions is possible:
requestLinkTopHatToTree
with any address as the adminapproveLinkTopHatToTree
and take over admin controls for the TopHat without the TopHat's permissionImpact
Tophats that expect to be fully self-sovereign and without any oversight can be surprisingly claimed by another admin, because settings from a previous admin remain through unlinking.
Code Snippet
https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/Hats.sol#L688-L732
Tool used
Manual Review
Recommendation
In
unlinkTopHatFromTree()
, thelinkedTreeRequests
should be deleted:function unlinkTopHatFromTree(uint32 _topHatDomain) external { uint256 fullTopHatId = uint256(_topHatDomain) << 224; // (256 - TOPHAT_ADDRESS_SPACE); _checkAdmin(fullTopHatId); delete linkedTreeAdmins[_topHatDomain]; + delete linkedTreeRequests[_topHatDomain]; emit TopHatLinked(_topHatDomain, 0); }
The text was updated successfully, but these errors were encountered: