cducrest-brainbot - checkAfterExecution threshold constraints incorrect

[sherlock-admin]

cducrest-brainbot

medium

checkAfterExecution threshold constraints incorrect

Summary

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.

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 hook checkAfterExecution() if you want to enforce the safe's threshold does not change.

Duplicate of #52