roguereddwarf - HatsSignerGateBase: signers can add / remove / swap signers which bypasses the HSG logic and can lead to multiple bad outcomes including DOS and increased control over Safe

[sherlock-admin]

roguereddwarf

medium

HatsSignerGateBase: signers can add / remove / swap signers which bypasses the HSG logic and can lead to multiple bad outcomes including DOS and increased control over Safe

Summary

This report deals with a similar issue as the TRST-M-6 Signers can backdoor the safe to execute any transaction in the future without consensus issue from the previous audit.

The previous issue dealt with the fact that signers can execute transactions that change the modules that are registered with the Safe.

This report is similar in the sense that signers can execute actions on the Safe that are unsafe and can break the assumptions made by the HSG logic.

Specifically, signers can execute the OwnerManager.addOwnerWithThreshold, OwnerManager.removeOwner and OwnerManager.swapOwner functions from the OwnerManager contract that the Safe inherits from.

The changes to the owners will not be detected in the HatsSignerGateBase.checkAfterExecution function which is executed after the transaction by the signers.

The checkAfterExecution function's purpose is explicitly to check that the signers did not perform any dangerous actions. However it lacks checks that the owners have not been messed with which as I will explain is such a "dangerous action".

Vulnerability Detail

The dangerous functions that the signers can execute on the Safe are the following:

OwnerManager.addOwnerWithThreshold

    function addOwnerWithThreshold(address owner, uint256 _threshold) public authorized {
        // Owner address cannot be null, the sentinel or the Safe itself.
        require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "GS203");
        // No duplicate owners allowed.
        require(owners[owner] == address(0), "GS204");
        owners[owner] = owners[SENTINEL_OWNERS];
        owners[SENTINEL_OWNERS] = owner;
        ownerCount++;
        emit AddedOwner(owner);
        // Change threshold if threshold was changed.
        if (threshold != _threshold) changeThreshold(_threshold);
    }

OwnerManager.removeOwner

    function removeOwner(address prevOwner, address owner, uint256 _threshold) public authorized {
        // Only allow to remove an owner, if threshold can still be reached.
        require(ownerCount - 1 >= _threshold, "GS201");
        // Validate owner address and check that it corresponds to owner index.
        require(owner != address(0) && owner != SENTINEL_OWNERS, "GS203");
        require(owners[prevOwner] == owner, "GS205");
        owners[prevOwner] = owners[owner];
        owners[owner] = address(0);
        ownerCount--;
        emit RemovedOwner(owner);
        // Change threshold if threshold was changed.
        if (threshold != _threshold) changeThreshold(_threshold);
    }

OwnerManager.swapOwner

    function swapOwner(address prevOwner, address oldOwner, address newOwner) public authorized {
        // Owner address cannot be null, the sentinel or the Safe itself.
        require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203");
        // No duplicate owners allowed.
        require(owners[newOwner] == address(0), "GS204");
        // Validate oldOwner address and check that it corresponds to owner index.
        require(oldOwner != address(0) && oldOwner != SENTINEL_OWNERS, "GS203");
        require(owners[prevOwner] == oldOwner, "GS205");
        owners[newOwner] = owners[oldOwner];
        owners[prevOwner] = newOwner;
        owners[oldOwner] = address(0);
        emit RemovedOwner(oldOwner);
        emit AddedOwner(newOwner);
    }

So let's look at a DOS scenario:

1. Assume the are 5 hats with the signersHatId. I.e. 5 hats that can potentially be signers.
2. Only 3 signers are allowed to be registered at a max. I.e. maxSigners=3
3. The signers can now call OwnerManager.addOwnerWithThreshold to add signers beyond maxSigners
4. This leads to a DOS in HatsSignerGateBase.reconcileSignerCount in the following lines:
   Link

    function reconcileSignerCount() public {
        address[] memory owners = safe.getOwners();
        uint256 validSignerCount = _countValidSigners(owners);


        if (validSignerCount > maxSigners) {
            revert MaxSignersReached();
        }

There are more valid signers than maxSigners so the revert will be reached.

Owners should only be added by the HatsSignerGate not by the signers themselves. The HatsSignerGate.claimSigner and MultiHatsSignerGate.claimSigner function ensure that maxSigners is not exceeded.

Let's look at another scenario how a bad signer can make use of this issue:

1. Assume the are 5 hats with the signersHatId. I.e. 5 hats that can potentially be signers.
2. Only 3 signers are allowed to be registered at a max. I.e. maxSigners=3
3. One of the 3 signers is more conservative in the transactions he signs. The other 2 signers want to replace him.
4. So say the threshold is 2
5. Now the other 2 signers can call OwnerManager.swapOwner and swap the third signer with another signer that suits their agenda.
6. Now even when the threshold increases to 3 the two signers can get through with their agenda.

So basically there starts a "race" among the signers to swap each other.

This should not be possible. Swaps should only be executed by the HatsSignerGateBase contract in case a signer becomes invalid Link.

Impact

I have shown in this report how it can lead to multiple bad outcomes when signers can influence the signers (owners) registered in the Safe. Managing signers should be solely the job of the HatsSignerGate, not of the signers.

The scenarios I showed are DOS and signers being able to increase their control over the Safe.

Code Snippet

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

https://github.com/safe-global/safe-contracts/blob/131f0d25135c1b98c185c940ae37fb0275ac4062/contracts/base/OwnerManager.sol#L58-L69

https://github.com/safe-global/safe-contracts/blob/131f0d25135c1b98c185c940ae37fb0275ac4062/contracts/base/OwnerManager.sol#L78-L90

https://github.com/safe-global/safe-contracts/blob/131f0d25135c1b98c185c940ae37fb0275ac4062/contracts/base/OwnerManager.sol#L99-L112

Tool used

Manual Review

Recommendation

The HatsSignerGateBase.checkAfterExecution function should check that the owners of the Safe are the same as before the execution.

Just like it checks that the registered modules are the same.

Duplicate of #48