Safe threshold can be set above target threshold, causing transactions to revert
If a `targetThreshold` is set below the safe's threshold, the `reconcileSignerCount()` function will fail to adjust the safe's threshold as it should, leading to a mismatch that causes all transactions to revert.\\nIt is possible and expected that the `targetThreshold` can be lowered, sometimes even lower than the current safe threshold.\\nIn the `setTargetThreshold()` function, there is an automatic update to lower the safe threshold accordingly. However, in the event that the `signerCount < 2`, it will not occur. This could easily happen if, for example, the hat is temporarily toggled off.\\nBut this should be fine! In this instance, when a new transaction is processed, `checkTransaction()` will be called, which calls `reconcileSignerCount()`. This should fix the problem by resetting the safe's threshold to be within the range of `minThreshold` to `targetThreshold`.\\nHowever, the logic to perform this update is faulty.\\n```\\nuint256 currentThreshold = safe.getThreshold();\\nuint256 newThreshold;\\nuint256 target = targetThreshold; // save SLOADs\\n\\nif (validSignerCount <= target && validSignerCount != currentThreshold) {\\n    newThreshold = validSignerCount;\\n} else if (validSignerCount > target && currentThreshold < target) {\\n    newThreshold = target;\\n}\\nif (newThreshold > 0) { // rest of code update safe threshold // rest of code }\\n```\\n\\nAs you can see, in the event that the `validSignerCount` is lower than the target threshold, we update the safe's threshold to `validSignerCount`. That is great.\\nIn the event that `validSignerCount` is greater than threshold, we should be setting the safe's threshold to `targetThreshold`. However, this only happens in the `else if` clause, when `currentThreshold < target`.\\nAs a result, in the situation where `target < current <= validSignerCount`, we will leave the current safe threshold as it is and not lower it. This results in a safe threshold that is greater than `targetThreshold`.\\nHere is a simple example:\\nvalid signers, target threshold, and safe's threshold are all 10\\nthe hat is toggled off\\nwe lower target threshold to 9\\nthe hat is toggled back on\\n`if` block above (validSignerCount <= target && validSignerCount != currentThreshold) fails because `validSignerCount > target`\\nelse `if` block above (validSignerCount > target && currentThreshold < target) fails because `currentThreshold > target`\\nas a result, `newThreshold == 0` and the safe isn't updated\\nthe safe's threshold remains at 10, which is greater than target threshold\\nIn the `checkAfterExecution()` function that is run after each transaction, there is a check that the threshold is valid:\\n```\\nif (safe.getThreshold() != _getCorrectThreshold()) {\\n    revert SignersCannotChangeThreshold();\\n}\\n```\\n\\nThe `_getCorrectThreshold()` function checks if the threshold is equal to the valid signer count, bounded by the `minThreshold` on the lower end, and the `targetThreshold` on the upper end:\\n```\\nfunction _getCorrectThreshold() internal view returns (uint256 _threshold) {\\n    uint256 count = _countValidSigners(safe.getOwners());\\n    uint256 min = minThreshold;\\n    uint256 max = targetThreshold;\\n    if (count < min) _threshold = min;\\n    else if (count > max) _threshold = max;\\n    else _threshold = count;\\n}\\n```\\n\\nSince our threshold is greater than `targetThreshold` this check will fail and all transactions will revert.
Edit the if statement in `reconcileSignerCount()` to always lower to the `targetThreshold` if it exceeds it:\\n```\\n// Remove the line below\\nif (validSignerCount <= target && validSignerCount != currentThreshold) {\\n// Add the line below\\nif (validSignerCount <= target) {\\n    newThreshold = validSignerCount;\\n// Remove the line below\\n} else if (validSignerCount > target && currentThreshold < target) {\\n// Add the line below\\n} else {\\n    newThreshold = target;\\n}\\n// Remove the line below\\nif (newThreshold > 0) { // rest of code update safe threshold // rest of code }\\n// Add the line below\\nif (newThreshold != currentThreshold) { // rest of code update safe threshold // rest of code }\\n```\\n
A simple change to the `targetThreshold` fails to propagate through to the safe's threshold, which causes all transactions to revert.
```\\nuint256 currentThreshold = safe.getThreshold();\\nuint256 newThreshold;\\nuint256 target = targetThreshold; // save SLOADs\\n\\nif (validSignerCount <= target && validSignerCount != currentThreshold) {\\n    newThreshold = validSignerCount;\\n} else if (validSignerCount > target && currentThreshold < target) {\\n    newThreshold = target;\\n}\\nif (newThreshold > 0) { // rest of code update safe threshold // rest of code }\\n```\\n