sherlock-audit / 2023-02-hats-judging

2 stars 0 forks source link

Dug - Valid signers can be forcibly removed from the safe #70

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Dug

medium

Valid signers can be forcibly removed from the safe

Summary

By calling removeOwner or swapOwner on the safe, signers can remove other valid signers from the safe.

Vulnerability Detail

The HSG is set up to prevent valid signers from being removed from the the safe. This is evident in the removeSigner function where a check is made with isValidSigner, reverting if the subject is wearing a signer hat.

    function removeSigner(address _signer) public virtual {
        if (isValidSigner(_signer)) {
            revert StillWearsSignerHat(_signer);
        }

        _removeSigner(_signer);
    }

However, a subset of signers can bypass these checks by approving a transaction that calls removeOwner or swapOwner on the safe.

Impact

This can have political consequences where, if a group of signers meets the threshold, they can remove opposing voices from voting power, potentially replacing them with more aligned signers.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add additional checks in checkAfterExecution that ensures signers we're not removed or swapped.

Duplicate of #118

spengrah commented 1 year ago

This is a duplicate of #118