Skip to content
This repository has been archived by the owner on Jun 11, 2023. It is now read-only.

chaduke - DOS attack to getHatLevel() #32

Closed
sherlock-admin opened this issue Mar 9, 2023 · 0 comments
Closed

chaduke - DOS attack to getHatLevel() #32

sherlock-admin opened this issue Mar 9, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 9, 2023

chaduke

medium

DOS attack to getHatLevel()

Summary

The getHatLevel() function uses a recursive call to obtain the level of a given _hatId. As a result, a malicious user can link ONE tophat domain to another node that has a large level number. As a result, when getHatLevel()
is called on any nodes under the tophat domain, it will revert due to out of gas.

Another limit is that the number of levels is limited to a maximum of 2^32-1, although theoretically we can have (2^32-1)*14 levels.

Vulnerability Detail

The getHatLevel() function uses a recursive call to obtain the level of a given _hatId.

https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/HatsIdUtilities.sol#L96-L106

As a result, if there are too many levels, the getHatLevel() will revert due to out of gas.

A malicious user can connect a tophat domain to a tree that has too many levels. As a result, getHatLevel() will revert when it is called on each node under the tophat domain.

The following code POC shows that when we connect 1048570 trees together, and we call getHatLevel() on the lowest node, we will run out of gas. One can adjust this number for different execution environment to get the idea of how getHatLevel() will run out of gas on large tree.

function testLevel() public {
        uint id;
        uint len = 1048570;
    
        for(uint32 i=1; i<=len; i++){
              id = utils.buildHatId(i << 224, 5);
              if(i>1){
                  utils.linkTree(i-1, id); 
              }
        }
        vm.expectRevert();
        utils.getHatLevel(utils.buildHatId(1 << 224, 5));
    }

Impact

A malicious user can launch a DOS attack to getHatLevel() by connecting a tophat domain to a deep tree. The vulnerability of the function is that it has to use recursion to get the level of a node.

Code Snippet

Tool used

VSCode

Manual Review

Recommendation

Introduce a data structure so that we will keep track of the level for each tophat domain. In this way, we only need to navigate one tree up to get the level of a node without recursion.

Another advantage of keeping track of the level for each tophat domain is the check of circular links: we can keep track of the highest level of tophat domain we have encountered so far, and if we visit a tophat domain that is smaller than before, then we detect a circle.

Duplicate of #96

@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Mar 12, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Mar 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant