carrot - Contract breaks if `targetThreshold` is ever reduced

[sherlock-admin]

carrot

high

Contract breaks if targetThreshold is ever reduced

Summary

The function setTargetThreshold allows the setting of the target threshold. If it is ever set lower than the current Threshold, the contract can get bricked.

Vulnerability Detail

The threshold of the contract is set according to the following conditions

1. If validSigners < minThreshold, revert in checkTransaction
2. if minThreshold <= validSigners < targetThreshold, threshold = validSigners
3. If validSigners > targetThreshold, threshold = targetThreshold

If the contract is in state 2 or 3, and the target threshold is manually set to a value which is lower than the current threshold, the function reconcileSignerCount fails to set the threshold correctly in subsequent calls. This is because this function uses branching if-statements, and none of them are entered in this particular case https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L198-L203

        if (validSignerCount <= target && validSignerCount != currentThreshold) {
            newThreshold = validSignerCount;
        } else if (validSignerCount > target && currentThreshold < target) {
            newThreshold = target;
        }
        if (newThreshold > 0) {

For the case validSignerCount > target, and currentThreshold > target, none of these statements are entered and the threshold is not set. This creates an issue when sending transactions from the safe. This is due to a check in the post-flight checkAfterExecution function https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L517-L519

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

The function _getCorrectThreshold calculates the threshold correctly. https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L533-L540

    function _getCorrectThreshold() internal view returns (uint256 _threshold) {
        uint256 count = _countValidSigners(safe.getOwners());
        uint256 min = minThreshold;
        uint256 max = targetThreshold;
        if (count < min) _threshold = min;
        else if (count > max) _threshold = max;
        else _threshold = count;
    }

However, since the reconcileSignerCount function didn't set the threshold correctly in the first place, the values wont match and this check will revert.

Impact

Bricked contract due to lowering targetThreshold

Code Snippet

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

Tool used

Manual Review

Recommendation

Use the same logic as in _getCorrectThreshold to set the threshold in reconcileSignerCount

Duplicate of #36