020.md

Short Obsidian Hyena

Medium

HatsSignerGate contract might still be a signer even if the signers have been claimed.

Summary

When calling the claimSignersFor function in HatsSignerGate.sol to claim signers, SafeManagerLib.encodeSwapOwnerAction could be skipped if the first signer already existed beforehand.

Root Cause

No response

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. A malicious user monitors the network for interactions with the Hats protocol.
2. Any user calls the claimSignersFor function to claim signers.
3. The malicious user intercepts the message and adds a valid signer as the first element in the message, effectively front-running the original transaction.
4. The HatsSignerGate contract remains the owner.

Why? The first element is skipped because the signer is already the owner, and the index (i) increases, causing the check for i == 0 && isInitialOwnersState to be bypassed.

Impact

No response

PoC

No response

Mitigation

      for (uint256 i; i < toClaimCount; ++i) {
        uint256 hatId = _hatIds[i];
        address signer = _signers[i];

        // register the signer, reverting if invalid or already registered
        _registerSigner({ _hatToRegister: hatId, _signer: signer, _allowReregistration: false });

        // if the signer is not an owner, add them
        if (!s.isOwner(signer)) {
          // initiate the addOwnerData, to be conditionally set below
          bytes memory addOwnerData;

          // for the first signer, check if the only owner is this contract and swap it out if so
-         if (i == 0 && isInitialOwnersState) {
            addOwnerData = SafeManagerLib.encodeSwapOwnerAction(SafeManagerLib.SENTINELS, address(this), signer);
          } else {
            // otherwise, add the claimer as a new owner
            addOwnerData = SafeManagerLib.encodeAddOwnerWithThresholdAction(signer, threshold);
            newNumOwners++;
          }

          // execute the call
          s.execSafeTransactionFromHSG(addOwnerData);
        }
      }

      // update the threshold if necessary
      uint256 newThreshold = _getNewThreshold(newNumOwners);
      if (newThreshold != threshold) {
        safe.execChangeThreshold(newThreshold);
      }
    }

+     bool firstOwner = true;

      for (uint256 i; i < toClaimCount; ++i) {
        uint256 hatId = _hatIds[i];
        address signer = _signers[i];

        // register the signer, reverting if invalid or already registered
        _registerSigner({ _hatToRegister: hatId, _signer: signer, _allowReregistration: false });

        // if the signer is not an owner, add them
        if (!s.isOwner(signer)) {
          // initiate the addOwnerData, to be conditionally set below
          bytes memory addOwnerData;

          // for the first signer, check if the only owner is this contract and swap it out if so
+         if (firstOwner && isInitialOwnersState) {
            addOwnerData = SafeManagerLib.encodeSwapOwnerAction(SafeManagerLib.SENTINELS, address(this), signer);
          } else {
            // otherwise, add the claimer as a new owner
            addOwnerData = SafeManagerLib.encodeAddOwnerWithThresholdAction(signer, threshold);
            newNumOwners++;
          }
+         firstOwner = false;

          // execute the call
          s.execSafeTransactionFromHSG(addOwnerData);
        }
      }

      // update the threshold if necessary
      uint256 newThreshold = _getNewThreshold(newNumOwners);
      if (newThreshold != threshold) {
        safe.execChangeThreshold(newThreshold);
      }
    }