duc - Function `_removeSigner` updates incorrect signerCount and threshold

[sherlock-admin]

duc

medium

Function _removeSigner updates incorrect signerCount and threshold

Summary

In contract HatsSignerGateBase.sol, when owners.length > 1, function _removeSigner updates incorrect signerCount and threshold, in both cases valid and invalid signer.

Vulnerability Detail

In function _removeSigner, when owners.length > 1:

uint256 validSignerCount = _countValidSigners(owners);

if (validSignerCount == currentSignerCount) {
    newSignerCount = currentSignerCount;
} else {
    newSignerCount = currentSignerCount - 1;
}

After that, newSignerCount is used to update signerCount and threshold. But newSigner is incorrect, as following:

• In case remove valid signer (not happen in current version, but can happen in future forks): newSignerCount should be validSignerCount - 1
• In case remove invalid signer: When validSignerCount !=currentSignerCount, newSignerCount should be updated to validSignerCount instead of currentSignerCount - 1. Because of this wrong update, signerCount and threshold of safe will be updated incorrectly. Furthermore, fuction removeSigner can be reverted by underflow bug even valid signers still exist.

Impact

Update incorrect signerCount and threshold, and removeSigner can be reverted by underflow bug.

Code Snippet

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

Tool used

Foundry

Recommendation

newSignerCount should be the number of valid signers after executing the remove signer transaction.

Duplicate of #79