sherlock-audit / 2023-02-hats-judging

2 stars 0 forks source link

Avci - There is WRONG calculation in lastHatId logic #122

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Avci

medium

There is WRONG calculation in lastHatId logic

Summary

There is the WRONG calculation in the lastHatId logic.

Vulnerability Detail

if you look at the function Hats.sol#createHat function L 159 NextHatidit it calculates the nexthat id and in getNextId

 function getNextId(uint256 _admin) public view returns (uint256 nextId) {
        uint16 nextHatId = _hats[_admin].lastHatId + 1;
        nextId = buildHatId(_admin, nextHatId);
    }

doing increasing logic but it also doing same increasing in line 169 hats.sol contract

        ++_hats[_admin].lastHatId;

Impact

the value of lastHatId will be more than what protocol expects and its unwanted thing will happen to if not fix and the value will be double

Code Snippet

    function createHat(
        uint256 _admin,
        string calldata _details,
        uint32 _maxSupply,
        address _eligibility,
        address _toggle,
        bool _mutable,
        string calldata _imageURI
    ) public returns (uint256 newHatId) {
        if (uint16(_admin) > 0) {
            revert MaxLevelsReached();
        }

        if (_eligibility == address(0)) revert ZeroAddress();
        if (_toggle == address(0)) revert ZeroAddress();

        newHatId = getNextId(_admin);

        // to create a hat, you must be wearing one of its admin hats
        _checkAdmin(newHatId);

        // create the new hat
        _createHat(newHatId, _details, _maxSupply, _eligibility, _toggle, _mutable, _imageURI);

        // increment _admin.lastHatId
        // use the overflow check to constrain to correct number of hats per level
        ++_hats[_admin].lastHatId;
    }

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

Tool used

Manual Review

Recommendation

consider modifying the code to the way it will no longer double the value,

spengrah commented 1 year ago

This is not a duplicate of #131, which deals with admin level off-by-1 error. This issue refers to the count of children hats of a given admin.

This issue claims that admin.lastHatId is incremented twice, but that is not actually the case. The incrementation inside of getHatId() is a to a local variable, which does not impact the storage value of admin.lastHatId. So I'm marking this one as disputed.