Request and approval flow for tophat links by spengrah · Pull Request #85 · Hats-Protocol/hats-protocol

spengrah · 2023-01-13T22:43:19Z

This PR modifies the tophat linking functionality to be consentful and prevent spam and social attack vectors. Now, tophats wishing to link to a new tree submit a request, which must be approved by an admin in the new tree before the linkage takes place.

Additionally, requests can now be submitted by either a) the wearer of an unlinked tophat, or b) the admin of an already-linked tophat. This latter option is new, and creates a pathway for parent trees to move child trees to different places within the parent tree.

Also, accounts that are admins of both the linked tree root and the destination admin hat can now "relink" without needing to first go through the request-approval flow.

implement new flow
initial testing
more comprehensive tests

topocount

Looks great! One potential question, but otherwise this is a very good extension of this feature. Starting to cut it close, though:

topocount · 2023-01-15T02:17:20Z

src/Hats.sol

+    /// @notice Internal function to link a Tree under a parent Tree
+    /// @dev Linking `_topHatId` replaces any existing links
+    /// @param _topHatId The domain of the tophat to link
+    /// @param _newAdminHat The new admin for the linked tree


Pretty sure natspec docstings aren't extracted from non-external/public functions, but I still think these are good practice.

topocount · 2023-01-15T02:20:16Z

src/Hats.sol

+        // Linkages must be initiated by a request
+        if (_newAdminHat != linkedTreeRequests[_topHatId]) revert LinkageNotRequested();


might we want to potentially bypass this if the msg.sender also wears the tophat of the candidate child tree?

@topocount Defined a separate function relinkTopHatWithinTree for that purpose. I could see the benefit of both clearer separation of concerns (ie w/ separate functions) and also of smaller code size (single function).

gershido

Hi all! @topocount I didn't have the chance to say hi on another platform so better GitHub than nothing :)

gershido · 2023-01-15T12:48:50Z

src/Hats.sol

+    function relinkTopHatWithinTree(uint32 _topHatId, uint256 _newAdminHat) external {
+        uint256 fullTopHatId = uint256(_topHatId) << 224; // (256 - TOPHAT_ADDRESS_SPACE);
+
+        // msg.sender being capable of both requesting and approving allows us to skip the request step
+        _checkAdmin(fullTopHatId); // "requester"
+        _checkAdmin(_newAdminHat); // "approver"
+
+        // execute the new link, replacing the old link
+        _linkTopHatToTree(_topHatId, _newAdminHat);
+    }


Two things that the function can currently do which may be a problem:

Link _topHatId for the first time without passing the noCircularLinkage test, in case the owner of _topHatId is also an admin of _newAdminHat.

Link _topHatId to a completely different tree than it is currently linked.

I guess the ideal is to check that _topHatId is currently linked to the tree that includes _newAdminHat by checking that they have a common ancestor (which is not _topHatId). But if implementing that is too expensive in terms of code size, maybe only preventing 1 by including a check that linkedTreeAdmins[_topHatId] != 0 and the noCircularLinkage test is good enough.

Good points.

For (1) noCircularLinkage should always be checked whenever a new link is being created, so I think the best bet is to include it in _linkTopHatToTree. Will push up that change.

For (2), I'm actually not sure we want to prevent this. If a DAO has a few different linked trees under its main tophat, I think it makes sense for the DAO to be able to move one of those linked trees to underneath another (provided it doesn't create a circular linkage, of course). Similarly, if a DAO was wearing two tophats, it makes sense for the DAO to be able to link one of those tophats under the other, or move a third linked tree from one to the other.

Do you see a downside to that, @gershido? Also interested in @topocount @nintynick @davehrlichman thoughts here.

I agree with Spencer here, that “it makes sense for the DAO to be able to move one of those linked trees to underneath another (provided it doesn't create a circular linkage, of course). Similarly, if a DAO was wearing two tophats, it makes sense for the DAO to be able to link one of those tophats under the other, or move a third linked tree from one to the other.”

In (2), I meant the possibility to relink a subtree to a tree that is not the parent tree and also not a tree that is linked to the parent tree. The thing that worries me is that if an admin hat becomes a bad actor, it can just "steal" a subtree to a dummy tree that it created and this would be non recoverable by the rest of the admins in the original tree. But on the other hand, the use case of a DAO with two top hats seems also useful...

I think the question of resiliency against possible actions will be very important once there are significant resources that are attached to hats

Ah, I see what you mean. So if we allow DAOs to move subtrees among their multiple tophats, we leave open an attack vector where a corrupted hat could steal a subtree.

I think I'm with you that the risk of the latter is larger than the benefit of the former, at least currently. We've been thinking about future registry feature that allows a DAO to "register" multiple tophats, creating a non-hierarchical association between trees; we could then allow moving subtrees across parent trees as long as all parent trees are registered to the same DAO.

But until then, I think it does make sense to constrain relinking to with-in tree moves if we can make the check feasible computationally and wrt bytecode size limits. I'll see what I can whip up today.

nintynick · 2023-01-16T20:15:42Z

@slgraham and I just paired to review this PR. Overall it looks good in the current state, pending some small changes coming shortly.

Documenting some of the considerations we discussed:

As we think about shipping a v1, given the immutability of the contract post-deployment, giving users optionality between two major philosophical approaches to creating Hats trees is something that seemed like it will be beneficial to understand what users what.
The two major philosophical approaches to creating Hats trees as we see them today are: A) creating hats in a single tree, with the benefits of addressable IDs, and B) linking branches that have been permissionlessly created to the official Hats tree for a DAO. (Of course there may be even more approaches than these two.)
Specifically for the linking approach, we want to ensure there is support for i) not breaking the connection between existing hatIDs and token gates, and ii) in the relinking flow, not needing to trust a wearer of a temporarily unlinked tree before reconnecting it to another admin.

Strong work!! 🧢

spengrah · 2023-01-17T00:24:05Z

pending some small changes coming shortly.

The bulk of these changes are:

Enabling the wearer of the new admin of a linked tophat to approve the linkage request. Previously, this was only possible for admins of the new admin hat, leaving the actual new admin unable to have a say in the matter.
Prevent cross-tree linking, as raised by @gershido. This is accomplished by introducing the concept of a "tippy tophat" --which is the domain of the utmost parent tophat for a given tophat (can also be thought of as the tophat of a tree of trees) -- and ensuring that the to-be-relinked tophat and its would-be new admin share the same tippy tophat, ie are part of the same tree of trees.

gershido · 2023-01-17T12:46:00Z

All looks good to me except one edge case regarding point (1):
_checkAdmin(buildHatId(_newAdminHat, 1)) would not work when _newAdminHat is on the last level

spengrah · 2023-01-17T21:44:22Z

All looks good to me except one edge case regarding point (1): _checkAdmin(buildHatId(_newAdminHat, 1)) would not work when _newAdminHat is on the last level

Great catch @gershido! Was hoping to not have to do a separate isWearerOfHat check, but I suppose we need to do that for the last level case. Added that in along with associated tests in the most recent commit.

nintynick

lgtm

add request and approval flow for tophat links

1699ff4

spengrah requested review from gershido, nintynick and topocount January 13, 2023 22:43

add relinking ability

96db519

topocount approved these changes Jan 15, 2023

View reviewed changes

gershido approved these changes Jan 15, 2023

View reviewed changes

spengrah added 2 commits January 15, 2023 18:51

move circular link check to _linkTopHatToTree

0d72aa7

update coverage and gas tracking

8b36c28

Base automatically changed from optimize-v1 to develop January 16, 2023 20:47

spengrah added 5 commits January 16, 2023 18:05

allow newAdminId wearers to approve tree links

a765cea

prevent cross-tree linking

ce23f68

clean up linking names and comments

986b9a0

address test compile warnings

6d9e001

add new link functions to IHats & IHatsIdUtilities

7e569a3

spengrah marked this pull request as ready for review January 17, 2023 00:24

fix admin check for last level hats

eb47136

nintynick approved these changes Jan 18, 2023

View reviewed changes

spengrah merged commit 00423a1 into develop Jan 18, 2023

spengrah deleted the feat/consentful-tree-linking branch January 21, 2023 00:17

		// Linkages must be initiated by a request
		if (_newAdminHat != linkedTreeRequests[_topHatId]) revert LinkageNotRequested();

Conversation

spengrah commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topocount left a comment

Choose a reason for hiding this comment

Uh oh!

topocount Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topocount Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

spengrah Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

gershido left a comment

Choose a reason for hiding this comment

Uh oh!

gershido Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

spengrah Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

davehrlichman Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

gershido Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

spengrah Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

nintynick commented Jan 16, 2023

Uh oh!

spengrah commented Jan 17, 2023

Uh oh!

gershido commented Jan 17, 2023

Uh oh!

spengrah commented Jan 17, 2023

Uh oh!

nintynick left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spengrah commented Jan 13, 2023 •

edited

Loading

topocount Jan 15, 2023 •

edited

Loading