The goal of checkAfterExecution() is to "prevent safe signers from removing this contract guard, changing any modules, or changing the threshold". However the way it is enforces allow for changing of threshold under certain situations and prevents certain transactions to be executed in a restrictive manner.
Vulnerability Detail
The check enforces: if (safe.getThreshold() != _getCorrectThreshold()) { revert ... }. Which means the safe user could have changed the safe's threshold to the value returned by _getCorrectThreshold() after execution and the transaction would not revert.
However, I believe the most note-worthy behaviour is that the safe cannot impact the value returned by _getCorrectThreshold() without updating its internal threshold otherwise their transaction will revert.
Impact
A multisig safe owned by a DAO will not be able to slash one of its owner for misbehaviour using the multisig because this will result in the previously valid user losing its hat, thus lowering the value of _countValidSigners(safe.owners) returned by _getCorrectThreshold() under certain threshold assumptions, making the checkAfterExecution() call revert.
Store the value of the underlying safe threshold locally in the before hook checkTransaction() and compare it in the after hook checkAfterExecution() if you want to enforce the safe's threshold does not change.
cducrest-brainbot
medium
checkAfterExecution threshold constraints incorrect
Summary
The goal of
checkAfterExecution()is to "preventsafesigners from removing this contract guard, changing any modules, or changing the threshold". However the way it is enforces allow for changing of threshold under certain situations and prevents certain transactions to be executed in a restrictive manner.Vulnerability Detail
The check enforces:
if (safe.getThreshold() != _getCorrectThreshold()) { revert ... }. Which means the safe user could have changed the safe's threshold to the value returned by_getCorrectThreshold()after execution and the transaction would not revert.However, I believe the most note-worthy behaviour is that the safe cannot impact the value returned by
_getCorrectThreshold()without updating its internal threshold otherwise their transaction will revert.Impact
A multisig safe owned by a DAO will not be able to slash one of its owner for misbehaviour using the multisig because this will result in the previously valid user losing its hat, thus lowering the value of
_countValidSigners(safe.owners)returned by_getCorrectThreshold()under certain threshold assumptions, making thecheckAfterExecution()call revert.Code Snippet
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
Store the value of the underlying safe threshold locally in the before hook
checkTransaction()and compare it in the after hookcheckAfterExecution()if you want to enforce the safe's threshold does not change.Duplicate of #52