search

sherlock-audit / 2023-02-hats-judging

2 stars 0 forks source link

obront - Other module can add owners to safe that push us above maxSigners, bricking safe #46

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

obront

high

Other module can add owners to safe that push us above maxSigners, bricking safe

Summary

If another module adds owners to the safe, these additions are not checked by our module or guard's logic. This can result in pushing us over maxSigners, which will cause all transactions to revert. In the case of an immutable hat, the only way to avoid the safe being locked permanently (with all funds frozen) may be to convince many hat wearers to renounce their hats.

Vulnerability Detail

When new owners are added to the contract through the claimSigner() function, the total number of owners is compared to maxSigners to ensure it doesn't exceed it.

However, if there are other modules on the safe, they are able to add additional owners without these checks.

In the case of HatsSignerGate.sol, there is no need to call claimSigner() to "activate" these owners. They will automatically be valid as long as they are a wearer of the correct hat.

This could lead to an issue where many (more than maxSigners) wearers of an immutable hat are added to the safe as owners. Now, each time a transaction is processed, checkTransaction() is called, which calls reconcileSignerCount(), which has the following check:

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

This will revert.

Worse, there is nothing the admin can do about it. If they don't have control over the eligibility address for the hat, they are not able to burn the hats or transfer them.

The safe will be permanently bricked and unable to perform transactions unless the hat wearers agree to renounce their hats.

Impact

The safe can be permanently bricked and unable to perform transactions unless the hat wearers agree to renounce their hats.

Code Snippet

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

Tool used

Manual Review

Recommendation

If validSignerCount > maxSigners, there should be some mechanism to reduce the number of signers rather than reverting.

Alternatively, as suggested in another issue, to get rid of all the potential risks of having other modules able to make changes outside of your module's logic, we should create the limit that the HatsSignerGate module can only exist on a safe with no other modules.

spengrah commented 1 year ago

I can see a few approaches here

  1. Add an onlyOwner function to change maxSigners. This opens up more surface area, but would enable the owner to unbrick the safe in this case.

  2. Add (and document) a trust assumption that other modules either a) will not add new safe owners, or b) can remove them if they accidentally brick the safe

  3. block any modules aside from HSG

cc @zobront

zobront commented 1 year ago

@spengrah I believe that allowing other modules aside from HSG adds a huge risk surface and is better not to. I know there are trade offs, but even if you manage to get everything right, you know there will be mistakes that lead to exploits, and in my view the best thing you can do for users is not allow it.

spengrah commented 1 year ago

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

zobront commented 1 year ago

Escalate for 10 USDC

All three dups of this issue (#51, #104, #130) describe the same issue, in which more than maxSigners can be added by (a) removing a hat, (b) reconciling, and (c) adding the hat back. This is a valid attack path.

This issue describes a separate issue, in which the extra signer can be added by an external module, which is a totally different attack with a different solution (note: @spengrah please make sure to review the other issues to ensure you have a fix that accounts for them).

This issue should be deduplicated from the other 3, since the attack is totally unrelated and simply results in the same outcome.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

All three dups of this issue (#51, #104, #130) describe the same issue, in which more than maxSigners can be added by (a) removing a hat, (b) reconciling, and (c) adding the hat back. This is a valid attack path.

This issue describes a separate issue, in which the extra signer can be added by an external module, which is a totally different attack with a different solution (note: @spengrah please make sure to review the other issues to ensure you have a fix that accounts for them).

This issue should be deduplicated from the other 3, since the attack is totally unrelated and simply results in the same outcome.

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 commented 1 year ago

Escalation accepted

Given although the outcome is similar the underlying issues are different Considering #51, #104, and #130 as a separate issue.

sherlock-admin commented 1 year ago

Escalation accepted

Given although the outcome is similar the underlying issues are different Considering #51, #104, and #130 as a separate issue.

This issue's escalations have been accepted!

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

MLON33 commented 1 year ago

zobront added "Fix Approved" label