Skip to content

Latest commit

 

History

History
102 lines (76 loc) · 4.28 KB

016.md

File metadata and controls

102 lines (76 loc) · 4.28 KB

Wobbly Chrome Cuckoo

High

Signers and HSG Modules can add a Module Guard to the Safe via delegatecall to brick all Safe-calling functions of the HSG and break protocol invariant

Summary

Both Safe owners/signers and HSG Modules can add a Module Guard to the Safe, which will then be able to block any transaction from the HSG to the Safe.

Root Cause

HSG uses _checkSafeState() to make sure that the Safe's storage is not changed by signers or HSG modules. It checks the guard, threshold, owners, fallback handler, and module invariants. However, it doesn't check the Module Guard.

  function _checkSafeState(ISafe _safe) internal view {
    if (_safe.getSafeGuard() != address(this)) revert CannotDisableThisGuard();

    // prevent signers from changing the threshold
    if (_safe.getThreshold() != _existingThreshold) revert CannotChangeThreshold();

    // prevent signers from changing the owners
    if (keccak256(abi.encode(_safe.getOwners())) != _existingOwnersHash) revert CannotChangeOwners();

    // prevent changes to the fallback handler
    if (_safe.getSafeFallbackHandler() != _existingFallbackHandler) revert CannotChangeFallbackHandler();

    // prevent signers from removing this module or adding any other modules
    (address[] memory modulesWith1, address next) = _safe.getModulesWith1();

    // ensure that there is only one module...
    // if the length is 0, we know this module has been removed
    // per Safe ModuleManager.sol#137, "If all entries fit into a single page, the next pointer will be 0x1", ie
    // SENTINELS. Therefore, if `next` is not SENTINELS, we know another module has been added.
    // We also check that the only module is this contract
    if (modulesWith1.length == 0 || next != SafeManagerLib.SENTINELS || modulesWith1[0] != address(this)) {
      revert CannotChangeModules();
    }
  }

Therefore, the Module Guard can be changed by either the signers or a HSG module via delegatecall.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

  1. Signers set a Module Guard in the Safe via delegatecall.
  2. The module guard can be set up to do arbitrary actions, for example block all transactions.
  3. The HSG cannot do anything to the safe, because all transactions from the HSG are sent via SafeManagerLib.execSafeTransactionFromHSG() or directly by calling execTransactionFromModule().
  function execSafeTransactionFromHSG(ISafe _safe, bytes memory _data) internal {
    _safe.execTransactionFromModule({ to: address(_safe), value: 0, data: _data, operation: Enum.Operation.Call });
  }

Note that execTransactionFromModule() always calls the Module Guard to check the transaction.

    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);//Module Guard call
        success = execute(to, value, data, operation, type(uint256).max);
        postModuleExecution(guard, guardHash, success);//another Module Guard call
    }
...
    function preModuleExecution(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation
    ) internal returns (address guard, bytes32 guardHash) {
        onBeforeExecTransactionFromModule(to, value, data, operation);
        guard = getModuleGuard();

        // Only whitelisted modules are allowed.
        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");

        if (guard != address(0)) {
            guardHash = IModuleGuard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);//Module Guard call
        }
    }

Impact

Signers can block all actions taken by the HSG and retain control of the Safe forever since the HSG can't remove them. DoS of all HSG calls to the Safe. Protocol invariant "(HSG) Modules should never be able to change any values in Safe storage" is broken.

PoC

No response

Mitigation

Add a check to _checkSafeState() to prevent changes to the module guard.