duc - When all signers of the gate lose their hats, `reconcileSignerCount` will not update threshold, then `targetThreshold` can be updated to be lower than the current threshold during here, leads to freeze safe's actions.

[sherlock-admin]

duc

medium

When all signers of the gate lose their hats, reconcileSignerCount will not update threshold, then targetThreshold can be updated to be lower than the current threshold during here, leads to freeze safe's actions.

Summary

In contract HatsSignerGateBase.sol, function reconcileSignerCount only updates the threshold if newThreshold > 0. When all signers of the gate lose their hats, reconcileSignerCount updates validSignerCount = 0, but safe's threshold doesn't change. During this time, targetThreshold can be updated without _setSafeThreshold, then targetThreshold can be

Vulnerability Detail

In function reconcileSignerCount, if validSignerCount == 0, newThreshold will be 0 and safe's threshold will not be updated:

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

Function setTargetThreshold doesn't update threshold when signerCount <= 1 :

function setTargetThreshold(uint256 _targetThreshold) public onlyOwner {
    if (_targetThreshold != targetThreshold) {
        _setTargetThreshold(_targetThreshold);

        if (signerCount > 1) _setSafeThreshold(_targetThreshold);
        emit HSGLib.TargetThresholdSet(_targetThreshold);
    }
}

If the new targetThreshold is lower than current threshold, safe's actions will be freezed because function checkAfterExecution always revert.

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

An example scenerio is as follows:

1. Assume validSignerCount = safe's threshold = targetThreshold = 3
2. All of signers lose their hats, reconcileSignerCount updates validSignerCount = 0, safe's threshold doesn't change (still = 3)
3. Owner updates targetThreshold to be 2
4. All of signers regain their hats, reconcileSignerCount updates validSignerCount = 3, safe's threshold doesn't change (still = 3) because current threshold == validSignerCount
5. After that, safe's actions can not be executed because function checkAfterExecution always revert (cause safe's threshold > targetThreshold)

   Impact

   If owner updates targetThreshold (decrease) during the time validSignerCount == 0, safe's actions can be freezed when the signers regain their hats.

Code Snippet

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

Tool used

Manual review

Recommendation

Should still call _setSafeThreshold when signerCount <= 1 in function setTargetThreshold:

function setTargetThreshold(uint256 _targetThreshold) public onlyOwner {
    if (_targetThreshold != targetThreshold) {
        _setTargetThreshold(_targetThreshold);

        _setSafeThreshold(_targetThreshold);
        emit HSGLib.TargetThresholdSet(_targetThreshold);
    }
}

Duplicate of #44