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.
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 thantargetThreshold
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)The
if
case works correctly.The issue is in the
else if
case.So imagine the following scenario:
validSignerCount=10
,target=9
andcurrentThreshold=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 encodevalidSignerCode
in the data which is 10. This is obviously wrong because it is above thetarget
. So we should use thenewThreshold
as the variable to encode since it is capped attarget
.Impact
A threshold greater than
targetThreshold
can be set in the Safe. This is wrong. The threshold should be capped attargetThreshold
.Code Snippet
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L183-L217
Tool used
Manual Review
Recommendation
Instead of
validSignerCount
, thenewThreshold
variable should be passed to the Safe.Fix:
Duplicate of #37