Skip to content

Latest commit

 

History

History
117 lines (88 loc) · 5.08 KB

027.md

File metadata and controls

117 lines (88 loc) · 5.08 KB
Raw

Great Syrup Sloth

High

Malicious user may permanently brick a safe or signers may abuse the system when it is migrating

Summary

When the owner wants to migrate, it is because a new version of hsg was released. The problem occurs because a user can frontrun migration to brick the safe completely. Additionally the new signers may use the safe without the module protections.

Root Cause

in HatsSingerGate.sol ln 357 https://github.com/sherlock-audit/2024-11-hats-protocol/blob/49de29508904e95b3cfaaf27d2e76c527429c019/hats-zodiac/src/HatsSignerGate.sol#L353

  function migrateToNewHSG(address _newHSG, uint256[] calldata _signerHatIds, address[] calldata _signersToMigrate)
    public
  {
    _checkUnlocked();
    _checkOwner();

    ISafe s = safe; // save SLOADS
    // remove existing HSG as guard
    s.execRemoveHSGAsGuard();
    // enable new HSG as module and guard
    s.execAttachNewHSG(_newHSG);
    // remove existing HSG as module
    s.execDisableHSGAsModule(_newHSG);

    // if _signersToMigrate is provided, migrate them to the new HSG by calling claimSignersFor()
    uint256 toMigrateCount = _signersToMigrate.length;
    if (toMigrateCount > 0) {
      // check that the arrays are the same length
      if (_signerHatIds.length != toMigrateCount) revert InvalidArrayLength();

      IHatsSignerGate(_newHSG).claimSignersFor(_signerHatIds, _signersToMigrate);
    }

in the migrateToNewHSG function we can see that first the function will remove the current hsg guard and attach a new guard. The problem occurs because the code attempts to remove the exisiting HSG as module but instead the code will disable the newly added module.

Secondly the code will migrate the signers to the new contract. The problem also occurs here that a malicious user can see what signers will be migrated and he can call claimSignersFor for one of the signers that is going to be claimed before this function is executed.

we can see from the snippet from the claimSignersFor function

s.execSafeTransactionFromHSG(addOwnerData);

there is a call to the function execSafeTransactionFromHSG.

  function execSafeTransactionFromHSG(ISafe _safe, bytes memory _data) internal {
    _safe.execTransactionFromModule({ to: address(_safe), value: 0, data: _data, operation: Enum.Operation.Call });
  }

as we can see the function execTransactionFromModule will be called with the addOwnerData which will be encodeAddOwnerWithThresholdAction. This will call the following logic in the safe.

    function execTransactionFromModule(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation
    ) external override returns (bool success) {
        (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation);
        success = execute(to, value, data, operation, type(uint256).max);
        postModuleExecution(guard, guardHash, success);
    }

This will call the following with the included data. Notice the success bool is returned but is never checked in the function that calls it.

    function addOwnerWithThreshold(address owner, uint256 _threshold) public override authorized {
        // Owner address cannot be null, the sentinel or the Safe itself.
        if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this)) revertWithError("GS203");
        // No duplicate owners allowed.
        if (owners[owner] != address(0)) revertWithError("GS204");
        owners[owner] = owners[SENTINEL_OWNERS];
        owners[SENTINEL_OWNERS] = owner;
        ownerCount++;
        emit AddedOwner(owner);
        // Change threshold if threshold was changed.
        if (threshold != _threshold) changeThreshold(_threshold);
    }

as we can see from the logic if one of the owners attempted to be added is already added the call will revert. But like i have showed that the call success boolean is never checked, the original call will proceed. The migration will be successful and only the owner added through frontrun will be added. And given that the enforced threshold will be the less of the threshold and the num of owners. The single owner can control the safe. Which can lead to losses of funds and abuse of the safe.

Internal pre-conditions

  1. malicious user frontruns migration
  2. migration succeeds but inner call fails to add all owners.

External pre-conditions

No response

Attack Path

  1. malicious user frontruns migration
  2. the frontrun will be a call with claimSignersFor with one of the signers to be added.
  3. the migration subcall will fail but the original migration call will succeed
  4. the only owner migrated will be the malicious owner who front ran the call
  5. the threshold of the safe will be set to the lesser value of the enforced threshold and the number of safe owners.
  6. given the number of safe owners will be 1 here, he can abuse the safe and cause loss of funds.

Impact

Abuse of the safe and can cause loss of funds if the safe is handling funds.

PoC

No response

Mitigation

check the success bool in the subcall to execTransactionFromModule