obront - Changing hat toggle address can lead to unexpected changes in status

[sherlock-admin]

obront

medium

Changing hat toggle address can lead to unexpected changes in status

Summary

Changing the toggle address should not change the current status unless intended to. However, in the event that a contract's toggle status hasn't been synced to local state, this change can accidentally toggle the hat back on when it isn't intended.

Vulnerability Detail

When an admin for a hat calls changeHatToggle(), the toggle address is updated to a new address they entered:

function changeHatToggle(uint256 _hatId, address _newToggle) external {
    if (_newToggle == address(0)) revert ZeroAddress();

    _checkAdmin(_hatId);
    Hat storage hat = _hats[_hatId];

    if (!_isMutable(hat)) {
        revert Immutable();
    }

    hat.toggle = _newToggle;

    emit HatToggleChanged(_hatId, _newToggle);
}

Toggle addresses can be either EOAs (who must call setHatStatus() to change the local config) or contracts (who must implement the getHatStatus() function and return the value).

The challenge comes if a hat has a toggle address that is a contract. The contract changes its toggle value to false but is never checked (which would push the update to the local state). The admin thus expects that the hat is turned off.

Then, the toggle is changed to an EOA. One would expect that, until a change is made, the hat would remain in the same state, but in this case, the hat defaults back to its local storage state, which has not yet been updated and is therefore set to true.

Even in the event that the admin knows this and tries to immediately toggle the status back to false, it is possible for a malicious user to sandwich their transaction between the change to the EOA and the transaction to toggle the hat off, making use of a hat that should be off. This could have dramatic consequences when hats are used for purposes such as multisig signing.

Impact

Hats may unexpectedly be toggled from off to on during toggle address transfer, reactivating hats that are intended to be turned off.

Code Snippet

https://github.com/Hats-Protocol/hats-protocol/blob/fafcfdf046c0369c1f9e077eacd94a328f9d7af0/src/Hats.sol#L623-L636

Tool used

Manual Review

Recommendation

The changeHatToggle() function needs to call checkHatToggle() before changing over to the new toggle address, to ensure that the latest status is synced up.

[spengrah]

The risk here is limited since toggle modules are treated as the source of truth for hat status and so admins should be aware that changing the toggle also changes the source of truth. But the recommended fix is simple and cheap enough that it makes sense to apply.

@zobront, does this same concept apply to eligibility?

[zobront]

@spengrah Agreed. The only situation I think would break a user's assumption is a change from contract => EOA, because in most cases the toggle will be saved in hat.config (so they won't need to take any action), but in this edge case it will be reset to an old version. That inconsistent behavior is the root of the problem.

The same is true with eligibility, but not sure if there's anything you can do about it, because it's on an individual hat by hat basis. If the eligibility contract is changed, you can't go through all the hats and update them. So my advice is just to be sure to document it very clearly in this case.

[spengrah]

Hats-Protocol/hats-protocol#116

[cducrest]

Escalate for 10 USDC

Disagree with being sever enough for being a medium. The Sherlock doc state "The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." However, as was explained, this problem is present both for status and eligibility, and a fix is possible and implemented in Hats-Protocol/hats-protocol#116 only for status, proving that the risk is acceptable for the eligibility part thus acceptable for the status part.

As noted in the comment by spengrah, the fix was implemented mostly because it is not expensive to do so, even though the risk is limited.

[sherlock-admin]

Escalate for 10 USDC

Disagree with being sever enough for being a medium. The Sherlock doc state "The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." However, as was explained, this problem is present both for status and eligibility, and a fix is possible and implemented in Hats-Protocol/hats-protocol#116 only for status, proving that the risk is acceptable for the eligibility part thus acceptable for the status part.

As noted in the comment by spengrah, the fix was implemented mostly because it is not expensive to do so, even though the risk is limited.

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 rejected

The issue is a valid medium
Given the edge case that the attack is still possible and can cause a hat to be activated which should not be and vice versa.

[sherlock-admin]

Escalation rejected

The issue is a valid medium
Given the edge case that the attack is still possible and can cause a hat to be activated which should not be and vice versa.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[MLON33]

zobront added "Fix Approved" label