roguereddwarf - Hats._isEligible and Hats._isActive functions might access old data

[sherlock-admin]

roguereddwarf

medium

Hats._isEligible and Hats._isActive functions might access old data

Summary

That Hats contract makes use of the Hats._isEligible and Hats._isActive function to retrieve the eligibility or active status of a hat.

The issue is that both functions do not save the data they query to storage. So the data that can be retrieved from storage at any time might not be the most recently queried data.

This means that when the eligibility or toggle module are turned off (i.e. they provide no longer any eligibility or active status data) the eligibility or active status data that the contract has access to might not be the most recent one.

Vulnerability Detail

Think of the following scenario:

1. Hats.balanceOf is called which calls _isEligible. It is determined that User A is not eligible to wear a certain hat. However this information is not stored to storage
2. The eligiblity module starts returning invalid data for some reason
3. When Hats.balanceOf is called again now, it will fall back to reading the eligibility from storage. The storage is not yet aware that User A is not eligible. So it returns that User A is eligible to wear the hat.

The first call to Hats.balanceOf should have saved the result to storage such that even when the module returns invalid data later on, the most recently queried data can be returned.

Impact

If the eligibility or toggle module do not provide valid data anymore, the fallback data from storage might not be up-to-date. This means hats might be active that should be inactive or addresses eligible to wear hats that should not be eligible (and vice versa).

Code Snippet

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

    function _isActive(Hat storage _hat, uint256 _hatId) internal view returns (bool active) {
        (bool success, bytes memory returndata) =
            _hat.toggle.staticcall(abi.encodeWithSignature("getHatStatus(uint256)", _hatId));

        /* 
        * if function call succeeds with data of length == 32, then we know the contract exists 
        * and has the getHatStatus function.
        * But — since function selectors don't include return types — we still can't assume that the return data is a boolean, 
        * so we treat it as a uint so it will always safely decode without throwing.
        */
        if (success && returndata.length == 32) {
            // check the returndata manually
            uint256 uintReturndata = uint256(bytes32(returndata));
            // false condition
            if (uintReturndata == 0) {
                active = false;
                // true condition
            } else if (uintReturndata == 1) {
                active = true;
            }
            // invalid condition
            else {
                active = _getHatStatus(_hat);
            }
        } else {
            active = _getHatStatus(_hat);
        }
    }

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

    function _isEligible(address _wearer, Hat storage _hat, uint256 _hatId) internal view returns (bool eligible) {
        (bool success, bytes memory returndata) =
            _hat.eligibility.staticcall(abi.encodeWithSignature("getWearerStatus(address,uint256)", _wearer, _hatId));

        /* 
        * if function call succeeds with data of length == 64, then we know the contract exists 
        * and has the getWearerStatus function (which returns two words).
        * But — since function selectors don't include return types — we still can't assume that the return data is two booleans, 
        * so we treat it as a uint so it will always safely decode without throwing.
        */
        if (success && returndata.length == 64) {
            bool standing;
            // check the returndata manually
            (uint256 firstWord, uint256 secondWord) = abi.decode(returndata, (uint256, uint256));
            // returndata is valid
            if (firstWord < 2 && secondWord < 2) {
                standing = (secondWord == 1) ? true : false;
                // never eligible if in bad standing
                eligible = (standing && firstWord == 1) ? true : false;
            }
            // returndata is invalid
            else {
                eligible = !badStandings[_hatId][_wearer];
            }
        } else {
            eligible = !badStandings[_hatId][_wearer];
        }
    }

Tool used

Manual Review

Recommendation

When _isEligible or _isActive is called, they should save the data they receive to storage.

Fix:

diff --git a/src/Hats.sol b/src/Hats.sol
index ae41a54..5b17c25 100644
--- a/src/Hats.sol
+++ b/src/Hats.sol
@@ -903,9 +903,11 @@ contract Hats is IHats, ERC1155, HatsIdUtilities {
             // false condition
             if (uintReturndata == 0) {
                 active = false;
+                _processHatStatus(_hatId, active);
                 // true condition
             } else if (uintReturndata == 1) {
                 active = true;
+                _processHatStatus(_hatId, active);
             }
             // invalid condition
             else {
@@ -987,6 +989,7 @@ contract Hats is IHats, ERC1155, HatsIdUtilities {
                 standing = (secondWord == 1) ? true : false;
                 // never eligible if in bad standing
                 eligible = (standing && firstWord == 1) ? true : false;
+                _processHatWearerStatus(_hatId, _wearer, eligible, standing);
             }
             // returndata is invalid
             else {

[spengrah]

This is expected behavior. Mechanistic toggle and eligibility modules (ie those conforming to the IHatsToggle or IHatsEligibility interfaces, respectively) should be treated as the source of truth for hat status and hat wearer status.