sherlock-audit / 2023-02-hats-judging

2 stars 0 forks source link

minhtrng - Owners can be swapped even though they still wear their signer hats #118

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

minhtrng

medium

Owners can be swapped even though they still wear their signer hats

Summary

HatsSignerGateBase does not check for a change of owners post-flight. This allows a group of actors to collude and replace opposing signers with cooperating signers, even though the replaced signers still wear their signer hats.

Vulnerability Detail

The HatsSignerGateBase performs various checks to prevent a multisig transaction to tamper with certain variables. Something that is currently not checked for in checkAfterExecution is a change of owners. A colluding group of malicious signers could abuse this to perform swaps of safe owners by using a delegate call to a corresponding malicious contract. This would bypass the requirement of only being able to replace an owner if he does not wear his signer hat anymore as used in _swapSigner:

for (uint256 i; i < _ownerCount - 1;) {
    ownerToCheck = _owners[i];

    if (!isValidSigner(ownerToCheck)) {
        // prep the swap
        data = abi.encodeWithSignature(
            "swapOwner(address,address,address)",
            ...

Impact

bypass restrictions and perform action that should be disallowed.

Code Snippet

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

Tool used

Manual Review

Recommendation

Perform a pre- and post-flight comparison on the safe owners, analogous to what is currently done with the modules.

spengrah commented 1 year ago

https://github.com/Hats-Protocol/hats-zodiac/pull/5

MLON33 commented 1 year ago

zobront added "Fix Approved" label