_guardEntries not protecting against re-entry

Summary

Safe users can forge a transaction to call checkTransaction() from within the safe, which will increase the value of _guardEntries by 1, rendering the underflow check in checkAfterExecution() useless.

Vulnerability Detail

The safe users can forge valid data and signatures for a valid safe transaction with currentNonce + 1, and use this data to make a valid safe transaction with currentNonce that executes this data.

The execution will go through checkTransaction() before execution, bumping _guardEntries to 1. It will then execute itself, entering checkTransaction() once again, bumping _guardEntries to 2.

After the transaction is executed, checkAfterExecution() will lower _guardEntries back to 1 and leave it at 1.

The transaction execution coming from the safe, the protective measure if (msg.sender != address(safe)) revert NotCalledFromSafe() is useless.

Impact

The _guardEntries value can be bumped to 1 by safe users. I am not sure why the protocol wants to prevent re-entry using this value on these functions, but if it is important to the protocol, it should know it does not work.

Code Snippet

checkTransaction bumps _guardEntries:

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L500-L502

checkAfterExecution lowers _guardEntries expecting to catch re-entry by having underflow errors:

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L527-L528

Tool used

Manual Review

Recommendation

Check that the value of _guardEntries is 1 before lowering it and revert if it is not.

Duplicate of #124

sherlock-audit / 2023-02-hats-judging

cducrest-brainbot - _guardEntries not protecting against re-entry #105