False default value for badStanding can expose the hat to side effects.

Summary

While, checking whether a wearer is eligible, if the call to the getWearerStatus function of the eligibility module set by the admin fails (reverting or does not return proper data), then there is a fallback mechanism that checks the value stored in badStandings mapping to determine whether a wearer is eligible. However, the default value for all values in badStandings is false and by implementing a fallback mechanism based on badStandings with false as a default value, this raises the risk of an attack exploiting the eligibility module and having side effects on the hat.

In a nutshell, if an attacker can try to find any way possible for the call to the eligibility module fail, then leveraging the current code she can force a read of badStandings for the given wearer but this value is likely not initialized and would return false. Since eligibility = !badStandings, it means that if the attacker finds a way to exploit the eligibility module it can make a previously non-eligible wearer to become eligible again.

In the rather non nominal case where the call to eligibility fails, we'd rather be on the safe side and consider everyone to be non eligible.

I can see two possible improvements to prevent this issue:

Have a variable called goodStandings instead of badStandings. Since the default value for the bool would still be false, any potential wearer would be considered in bad standing by default and therefore a wearer previously tagged as non eligible by the module would remain non eligible at least until manual intervention. If the eligibility module becomes exploited at some point this solution would prevent any side effect on the hat.
Or an alternative solution could be to revert when the call to the eligibility module fails so that the attack cannot have side effects on the hat if he or she finds a vulnerability in the eligibility module.

Vulnerability Detail

A user can wear a hat if he or she is eligible. This is checked by looking at the balance of the given hat in

function balanceOf(address _wearer, uint256 _hatId)
        public
        view
        override(ERC1155, IHats)
        returns (uint256 balance)
    {
        Hat storage hat = _hats[_hatId];

        balance = 0;

        if (_isActive(hat, _hatId) && _isEligible(_wearer, hat, _hatId)) { <==========================================
            balance = super.balanceOf(_wearer, _hatId);
        }
    }

However, _isEligible calls the eligibility module in order to compute whether a wearer is eligible and in the case this call fails (reverts or send unexpected data), then there is a fallback mechanism used to determine whether the wearer is eligible as we can read in the function below.

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]; <===================================================
        }
    }

Now, let say Bob was wearing a hat and is considered non eligible anymore by the rules of the eligibility contracts but this contracts gets broken by an attack, then the attacker can force execution of the last branch of this function which would consider the wearer eligible while it is not anymore regarding the eligibility rules defined in the eligibility contract.

Impact

An exploit to an eligibility module can have side effect on the hat and consider a non-eligible wearer to still be eligible.

Code Snippet

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

Tool used

Manual Review

Recommendation

Revert when the call to the eligibility module fails so that the attack cannot have side effects on the hat if he or she finds a vulnerability in the eligibility module. (preferred solution imo for future proofing the protocol).
or have a mapping called goodStandings instead of badStandings. This mapping would consider everyone to be in bad standing state by default. If the eligibility module ends up in a non-nominal state at some point this solution would at least ensure that if no default value has been set for this wearer, he or she does not wrongly get considered eligible while it should not.

sherlock-audit / 2023-02-hats-judging

clems4ever - False default value for badStanding can expose the hat to side effects. #69