There is no guarantee that the signerCount value of HSG is in sync with the number of valid owners of the safe when owner calls setTargetThreshold() to change the target threshold of the HSG. This will be the case if the owner does not call reconcileSignerCount() beforehand or during set up of HSG that is being wired to an underlying safe.
This means the function _setSafeThreshold() may not be called and the value of threshold on the underlying safe remains higher than targetThreshold on the HSG.
Vulnerability Detail
This problem is made worse by the behaviour of reconcileSignerCount() which does not update the safe threshold if validSignerCount > targetThreshold && safe.getThreshold() > targetThresold.
Impact
The value of threshold on the safe may remain higher than targetThreshold on the HSG for a long period of time and go unnoticed to users / owner. This may result in safe tx execution failing unexpectedly.
After safe tx execution checkAfterExecution() will call _getCorrectThreshold() which will return the value of targetThreshold if the number of valid signers is equal to or higher than targetThreshold. The check if (safe.getThreshold() != _getCorrectThreshold()) { revert ... } will then revert the execution of the transaction.
I believe the impact is high as this issue is likely to arise without being caught and create confusion to users when they try to execute their transaction.
Code Snippet
setTargetThreshold only sets the safe threshold if signerCount > 1:
cducrest-brainbot
high
Fail to set safe threshold to targetThreshold
Summary
There is no guarantee that the
signerCount
value of HSG is in sync with the number of valid owners of the safe when owner callssetTargetThreshold()
to change the target threshold of the HSG. This will be the case if the owner does not callreconcileSignerCount()
beforehand or during set up of HSG that is being wired to an underlying safe.This means the function
_setSafeThreshold()
may not be called and the value ofthreshold
on the underlying safe remains higher thantargetThreshold
on the HSG.Vulnerability Detail
This problem is made worse by the behaviour of
reconcileSignerCount()
which does not update the safe threshold ifvalidSignerCount > targetThreshold && safe.getThreshold() > targetThresold
.Impact
The value of
threshold
on the safe may remain higher thantargetThreshold
on the HSG for a long period of time and go unnoticed to users / owner. This may result in safe tx execution failing unexpectedly.After safe tx execution
checkAfterExecution()
will call_getCorrectThreshold()
which will return the value oftargetThreshold
if the number of valid signers is equal to or higher thantargetThreshold
. The checkif (safe.getThreshold() != _getCorrectThreshold()) { revert ... }
will then revert the execution of the transaction.I believe the impact is high as this issue is likely to arise without being caught and create confusion to users when they try to execute their transaction.
Code Snippet
setTargetThreshold only sets the safe threshold if
signerCount > 1
:https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L95-L103
reconcileSignerCount does not update the safe threshold if
validSignerCount > targetThreshold && safe.getThreshold() > targetThresold
:https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L194-L202
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
Update the safe threshold under less strict conditions.
Duplicate of #44