unforgiven - attacker can perform malicious transactions in the safe because reentrancy is not implemented correctly in the checkTransaction() and checkAfterExecution() function in HSG

[sherlock-admin]

unforgiven

high

attacker can perform malicious transactions in the safe because reentrancy is not implemented correctly in the checkTransaction() and checkAfterExecution() function in HSG

Summary

to prevent reentrancy during the safe's execTransaction() function call code use _guardEntries and increase it in the checkTransaction() and decrease it in the checkAfterExecution(). but the logic is wrong and code won't underflow in the checkAfterExecution() if attacker perform reentrancy during the execTransaction()

Vulnerability Detail

This is some part of the checkTransaction() and checkAfterExecution() code:

    function checkTransaction(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address payable refundReceiver,
        bytes memory signatures,
        address // msgSender
    ) external override {
        if (msg.sender != address(safe)) revert NotCalledFromSafe();

        uint256 safeOwnerCount = safe.getOwners().length;
        // uint256 validSignerCount = _countValidSigners(safe.getOwners());

        // ensure that safe threshold is correct
        reconcileSignerCount();

        if (safeOwnerCount < minThreshold) {
            revert BelowMinThreshold(minThreshold, safeOwnerCount);
        }

        // get the tx hash; view function
        bytes32 txHash = safe.getTransactionHash(
            // Transaction info
            to,
            value,
            data,
            operation,
            safeTxGas,
            // Payment info
            baseGas,
            gasPrice,
            gasToken,
            refundReceiver,
            // Signature info
            // We subtract 1 since nonce was just incremented in the parent function call
            safe.nonce() - 1 // view function
        );

        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);

        unchecked {
            ++_guardEntries;
        }
    }

    /// @notice Post-flight check to prevent `safe` signers from removing this contract guard, changing any modules, or changing the threshold
    /// @dev Modified from https://github.com/gnosis/zodiac-guard-mod/blob/988ebc7b71e352f121a0be5f6ae37e79e47a4541/contracts/ModGuard.sol#L86
    function checkAfterExecution(bytes32, bool) external override {
        if (msg.sender != address(safe)) revert NotCalledFromSafe();

        // leave checked to catch underflows triggered by re-erntry attempts
        --_guardEntries;
    }

as you can see code increase the value of the _guardEntries in the checkTransaction() which is called before the transaction execution and decrease its value in the checkAfterExecution which is called after transaction execution. this won't protect against reentrancy during the safe's execTransaction() call. attacker can perform this actions:

1. Transaction1 which has valid number of signers and set the value of the guard to 0x0. and call safe.execTransaction(Transaction2).
2. Transaction2 which reset the value of the guard to the HSG address.
3. now by calling Tsafe.execTransaction(Transaction1) code would first call checkTransaction() and would see the number of the signers is correct and then increase the value of the _guardEntiries to 1 and then code in safe would execute the Transaction1 which would set the guard to 0x0 and execute the Transaction2 in safe.
4. because guard is 0x0 code would execute the Transaction2 and then during that code would re-set the value of the guard to the HSG address.
5. now checkAfterExecution() would get exeucted and would see that guard value is correct and would decrease the _guardEntiries

the attack is possible by changing the value of the threshhold in the safe. because code would perform two increase and one decrease during the reentrancy so the underflow won't happen.

Impact

it's possible to set guard or threshold during the execTransaction() and execute another malicious transaction which resets guard and threshold

Code Snippet

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

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

https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/Safe.sol#L172-L174

Tool used

Manual Review

Recommendation

set the value of the guard to 1 and decrease in the checkTransaction() and increase in the checkAfterExecution().

[MLON33]

zobront clarified 124 was previously duped to 41, but was separated because it didn't accurately express the High severity exploit.

[MLON33]

zobront added "Fix Approved" label

[jacksanford1]

Fix added by spengrah for reference: https://github.com/Hats-Protocol/hats-zodiac/pull/13