cccz - reconcileSignerCount may not update safe's threshold when safe's threshold > traget

[sherlock-admin]

cccz

medium

reconcileSignerCount may not update safe's threshold when safe's threshold > traget

Summary

reconcileSignerCount may not update safe's threshold when safe's threshold > traget.

Vulnerability Detail

safe's threshold should always be less than or equal to targetThreshold. In reconcileSignerCount, when validSignerCount <= target, as long as validSignerCount != currentThreshold, will change safe's threshold to validSignerCount But when validSignerCount > target, only when currentThreshold < target, safe's threshold will be changed, and when currentThreshold > target, safe's threshold will not be changed, which will make safe's threshold exceed targetThreshold.

    function reconcileSignerCount() public {
        address[] memory owners = safe.getOwners();
        uint256 validSignerCount = _countValidSigners(owners);

        if (validSignerCount > maxSigners) {
            revert MaxSignersReached();
        }

        // update the signer count accordingly
        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);

In general, currentThreshold <= target, but the parameter of calling changeThreshold in reconcileSignerCount is validSignerCount, which make currentThreshold > target

Impact

This will prevent reconcileSignerCount from adjusting safe's threshold to be less than or equal to targetThreshold , thus more signatures are requested in 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

    function reconcileSignerCount() public {
        address[] memory owners = safe.getOwners();
        uint256 validSignerCount = _countValidSigners(owners);

        if (validSignerCount > maxSigners) {
            revert MaxSignersReached();
        }

        // update the signer count accordingly
        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) {
+       } else if (validSignerCount > target && currentThreshold != target) {
            newThreshold = target;
        }
        if (newThreshold > 0) {
            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);

Duplicate of #44