053.md

roguereddwarf

medium

HatsSignerGateBase: reconcileSignerCount function might set threshold too high

Summary

There is a simple mistake in the HatsSignerGateBase.reconcileSignerCount function.

The wrong variable is used to set the threshold in the Safe.

In some cases a threshold can be set that is too large such that more valid signatures than targetThreshold are required to execute a transaction.

Vulnerability Detail

Let's have a look at the issue in the reconcileSignerCount function. (See the full code of the function in the "Code Snippet" section)

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);

The if case works correctly.

The issue is in the else if case.

So imagine the following scenario: validSignerCount=10, target=9 and currentThreshold=8. It is possible for the variables to have these values if for example there were 8 valid signers + 2 invalid signers and these invalid signers have become valid again.

So the else if case applies.

So we set newThreshold=target which is 9. And then we encode validSignerCode in the data which is 10. This is obviously wrong because it is above the target. So we should use the newThreshold as the variable to encode since it is capped at target.

Impact

A threshold greater than targetThreshold can be set in the Safe. This is wrong. The threshold should be capped at targetThreshold.

Code Snippet

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

    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);


            bool success = safe.execTransactionFromModule(
                address(safe), // to
                0, // value
                data, // data
                Enum.Operation.Call // operation
            );


            if (!success) {
                revert FailedExecChangeThreshold();
            }
        }
    }

Tool used

Manual Review

Recommendation

Instead of validSignerCount, the newThreshold variable should be passed to the Safe.

Fix:

diff --git a/src/HatsSignerGateBase.sol b/src/HatsSignerGateBase.sol
index 3e8bb5f..2651b2c 100644
--- a/src/HatsSignerGateBase.sol
+++ b/src/HatsSignerGateBase.sol
@@ -201,7 +201,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
             newThreshold = target;
         }
         if (newThreshold > 0) {
-            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
+            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);
 
             bool success = safe.execTransactionFromModule(