roguereddwarf - HatsSignerGateBase: _removeSigner function may revert so it is not possible to remove a signer

[sherlock-admin]

roguereddwarf

medium

HatsSignerGateBase: _removeSigner function may revert so it is not possible to remove a signer

Summary

If the HatsSignerGateBase._removeSigner function is called under certain conditions, it will be attempted to set the threshold in the Safe to zero which reverts.

So it is not possible to remove a signer that is invalid.

Vulnerability Detail

The scenario is the following:

The _removeSigner function is called under these conditions: currentSignerCount=1, owners.length=2, validSignerCount=0, targetTreshold=1

This means the else block is entered. Link

} else {
    uint256 currentThreshold = safe.getThreshold();
    uint256 newThreshold = currentThreshold;
    uint256 validSignerCount = _countValidSigners(owners);

    if (validSignerCount == currentSignerCount) {
        newSignerCount = currentSignerCount;
    } else {
        newSignerCount = currentSignerCount - 1;
    }

    // ensure that txs can't execute if fewer signers than target threshold
    if (newSignerCount <= targetThreshold) {
        newThreshold = newSignerCount;
    }

    removeOwnerData = abi.encodeWithSignature(
        "removeOwner(address,address,uint256)", _findPrevOwner(owners, _signer), _signer, newThreshold
    );
}

We execute newSignerCount = currentSignerCount - 1. So newSignerCount = 0.

Also, newSignerCount <= targetThreshold, so newThreshold = newSignerCount.

So we set the threshold in the Safe to 0.

We can see in the code of the Safe that this will eventually revert: Link

require(_threshold >= 1, "GS202");

Impact

Under certain conditions the _removeSigner function reverts. So it is not possible to remove a signer that has become invalid. So the signer can persist in the contract and possibly become valid again later even though he should have been removed.

Code Snippet

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

    function _removeSigner(address _signer) internal {
        bytes memory removeOwnerData;
        address[] memory owners = safe.getOwners();
        uint256 currentSignerCount = signerCount; // save an SLOAD
        uint256 newSignerCount;

        if (currentSignerCount < 2 && owners.length == 1) {
            // signerCount could be 0 after reconcileSignerCount
            // make address(this) the only owner
            removeOwnerData = abi.encodeWithSignature(
                "swapOwner(address,address,address)",
                SENTINEL_OWNERS, // prevOwner
                _signer, // oldOwner
                address(this) // newOwner
            );

            // newSignerCount is already 0
        } else {
            uint256 currentThreshold = safe.getThreshold();
            uint256 newThreshold = currentThreshold;
            uint256 validSignerCount = _countValidSigners(owners);

            if (validSignerCount == currentSignerCount) {
                newSignerCount = currentSignerCount;
            } else {
                newSignerCount = currentSignerCount - 1;
            }

            // ensure that txs can't execute if fewer signers than target threshold
            if (newSignerCount <= targetThreshold) {
                newThreshold = newSignerCount;
            }

            removeOwnerData = abi.encodeWithSignature(
                "removeOwner(address,address,uint256)", _findPrevOwner(owners, _signer), _signer, newThreshold
            );
        }

        // update signerCount
        signerCount = newSignerCount;

        bool success = safe.execTransactionFromModule(
            address(safe), // to
            0, // value
            removeOwnerData, // data
            Enum.Operation.Call // operation
        );

        if (!success) {
            revert FailedExecRemoveSigner();
        }
    }

Tool used

Manual Review

Recommendation

The threshold should not be changed to 0. So I recommend implementing a check to ensure newThreshold is above 0.

Fix:

diff --git a/src/HatsSignerGateBase.sol b/src/HatsSignerGateBase.sol
index 3e8bb5f..432011d 100644
--- a/src/HatsSignerGateBase.sol
+++ b/src/HatsSignerGateBase.sol
@@ -395,6 +395,8 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
                 newThreshold = newSignerCount;
             }

+            if (newThreshold == 0) newThreshold = 1;
+
             removeOwnerData = abi.encodeWithSignature(
                 "removeOwner(address,address,uint256)", _findPrevOwner(owners, _signer), _signer, newThreshold
             );

[roguereddwarf]

Escalate for 10 USDC

This is not a duplicate of #107 .

107 describes an issue when currentSignerCount < validSignerCount.

When you look at my scenario you can see it is a different one: currentSignerCount=1, owners.length=2, validSignerCount=0, targetTreshold=1. In fact currentSignerCount > validSignerCount.

107 deals with how his issue can cause an underflow in the signer count.

On the other hand I show how this check fails: https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/base/OwnerManager.sol#L123

Also my issue cannot be solved by just calling reconcileSignerCount so it is not just a bad UX experience.

[sherlock-admin]

Escalate for 10 USDC

This is not a duplicate of #107 .

107 describes an issue when currentSignerCount < validSignerCount.

When you look at my scenario you can see it is a different one: currentSignerCount=1, owners.length=2, validSignerCount=0, targetTreshold=1. In fact currentSignerCount > validSignerCount.

107 deals with how his issue can cause an underflow in the signer count.

On the other hand I show how this check fails: https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/base/OwnerManager.sol#L123

Also my issue cannot be solved by just calling reconcileSignerCount so it is not just a bad UX experience.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Not a duplicate of #107 But not a valid high/medium on its own, as when validSignerCount == 0 & the number of owners is equal to or greater than the threshold, claimSigner would swap the invalidsigner, or claimSigner would add one valid one, and then the invalid one could be removed with removeSigner.

[sherlock-admin]

Escalation accepted

Not a duplicate of #107 But not a valid high/medium on its own, as when validSignerCount == 0 & the number of owners is equal to or greater than the threshold, claimSigner would swap the invalidsigner, or claimSigner would add one valid one, and then the invalid one could be removed with removeSigner.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.