duc - Hat wearers who are not the safe's owners can execute safe's transaction

[sherlock-admin]

duc

high

Hat wearers who are not the safe's owners can execute safe's transaction

Summary

In contract HatsSignerGateBase.sol, function countValidSignatures is used to count number of valid signers from the signatures. Because of maxSigners, the valid hat wearers can be unable to claim signer permission to be safe's owners. However, these wearers who are not the safe's owners can be counted in countValidSignatures, and they can execute safe's transaction.

Vulnerability Detail

Function countValidSignatures always increases validSigCount if currentOwner is a valid wearer, even this address is not the one of safe's owners.

//function `countValidSignatures`, contract `HatsSignerGateBase.sol`
if (isValidSigner(currentOwner)) {
    // shouldn't overflow given reasonable sigCount
    unchecked {
        ++validSigCount;
    }
}

• isValidSigner in contract HatsSignerGate:

  function isValidSigner(address _account) public view override returns (bool valid) {
  valid = HATS.isWearerOfHat(_account, signersHatId);
  }

• isValidSigner in contract MultiHatsSignerGate:

  function isValidSigner(address _account) public view override returns (bool valid) {
  /// @dev existing `claimedSignerHats` are always valid, since `validSignerHats` is append-only
  valid = HATS.isWearerOfHat(_account, claimedSignerHats[_account]);
  }

  Therefore, the wearers of specific hats who are not the safe's owners still can execute the safe's transaction. Example, there are 20 wearers have the same hat, but only 10 of them are safe's owners (maxSigner = 10). However, the other 10 wearers (who are not safe's owners) can execute safe's transaction.

Impact

Valid hat wearers who are not the safe's owners can execute safe's transaction

Code Snippet

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L580-L585

Tool used

Manual review

Recommendation

Add the check to confirm the signer from signatures is safe's owner in function countValidSignatures:

if (isValidSigner(currentOwner) && safe.isOwner(currentOwner)) {
    // shouldn't overflow given reasonable sigCount
    unchecked {
        ++validSigCount;
    }
}

[spengrah]

This is not an issue. While countValidSignatures is a public function, it does not itself gate Safe transaction execution. Only when it is called from within the context of checkTransaction does it actually gate Safe transaction execution, and in that context the signatures argument comes directly from checkTransaction, which in turn comes directly from the signatures submitted to the Safe when signers are attempting a transaction.