reconcileSignerCount calls safe.changeThreshold with incorrect parameters

Summary

reconcileSignerCount should use newThreshold instead of validSignerCount to call safe.changeThreshold.

Vulnerability Detail

In reconcileSignerCount, safe.changeThreshold is called when newThreshold > 0, but it incorrectly uses validSignerCount instead of newThreshold for its argument. When validSignerCount <= target && validSignerCount ! = currentThreshold, newThreshold == validSignerCoun. However, when validSignerCount > target && currentThreshold < target, newThreshold == target < validSignerCount.

        signerCount = validSignerCount;

        uint256 currentThreshold = safe.getThreshold();
        uint256 newThreshold;
        uint256 target = targetThreshold; // save SLOADs

        if (validSignerCount <= target && validSignerCount != currentThreshold) {
            newThreshold = validSignerCount;
        } else if (validSignerCount > target && currentThreshold < target) {
            newThreshold = target;
        }
        if (newThreshold > 0) {
            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);

Consider the current validSignerCount = 6, targetThreshold = 5, safe'threshold= 5, After alice and bob lose their signer eligibility, reconcileSignerCount is called, validSignerCount = 4, targetThreshold = 5, safe'threshold = 4. After alice and bob regain signer eligibility, reconcileSignerCount is called, validSignerCount = 6, targetThreshold = 5, safe'threshold should be 5 but is incorrectly changed to 6.

Impact

This makes safe'threshold large, so that safe'threshold exceeds targetThreshold, and more signatures are requested in the checkTransaction.

        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);

        // revert if there aren't enough valid signatures
        if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {
            revert InvalidSigners();
        }

Then in checkAfterExecution, it will revert at the following code

        if (safe.getThreshold() != _getCorrectThreshold()) {
            revert SignersCannotChangeThreshold();
        }

Code Snippet

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

Tool used

Manual Review

Recommendation

Change to

        if (validSignerCount <= target && validSignerCount != currentThreshold) {
            newThreshold = validSignerCount;
        } else if (validSignerCount > target && currentThreshold < target) {
            newThreshold = target;
        }
        if (newThreshold > 0) {
-           bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
+           bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);

Duplicate of #37

sherlock-audit / 2023-02-hats-judging

cccz - reconcileSignerCount calls safe.changeThreshold with incorrect parameters #84