setTargetThreshold can set target below minThreshold
Summary
The setTargetThreshold() function can be used by owner to set the target threshold below minThreshold value, which is unexpected in the remainder of the contract.
Vulnerability Detail
The function setMinThreshold() calls _setMinThreshold() which revert if _minThreshold > targetThreshold. However setTargetThreshold() does not enforce such constraint and can result in a value of targetThreshold < minThreshold.
Impact
Before safe tx execution reconcileSignerCount() will set the safe thershold to targetThreshold if the number of valid signer count is above the threshold.
After safe tx execution checkAfterExecution() will call _getCorrectThreshold() which will return the value of minThreshold if the number of valid signers is below it (which can be the case if targetThreshold < minThreshold). The check if (safe.getThreshold() != _getCorrectThreshold()) { revert ... } will then revert the execution of the transaction.
Safe transactions will no longer be executable.
Code Snippet
setTargetThreshold does not check for minThreshold constraints:
cducrest-brainbot
medium
setTargetThreshold can set target below minThreshold
Summary
The
setTargetThreshold()function can be used by owner to set the target threshold belowminThresholdvalue, which is unexpected in the remainder of the contract.Vulnerability Detail
The function
setMinThreshold()calls_setMinThreshold()which revert if_minThreshold > targetThreshold. HoweversetTargetThreshold()does not enforce such constraint and can result in a value oftargetThreshold < minThreshold.Impact
Before safe tx execution
reconcileSignerCount()will set the safe thershold totargetThresholdif the number of valid signer count is above the threshold.After safe tx execution
checkAfterExecution()will call_getCorrectThreshold()which will return the value ofminThresholdif the number of valid signers is below it (which can be the case iftargetThreshold < minThreshold). The checkif (safe.getThreshold() != _getCorrectThreshold()) { revert ... }will then revert the execution of the transaction.Safe transactions will no longer be executable.
Code Snippet
setTargetThreshold does not check for minThreshold constraints:
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L95-L114
Check on safe / HSG threshold:
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L517-L519
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L533-L540
Tool used
Manual Review
Recommendation
Prevent the target threshold to be set lower than minThreshold.
Duplicate of #36