Operator can DOS DSS's unregistrationHook function by specifying arbitrary unregistrationHookData while not reverting its own Core.unregisterOperatorFromDSS function call

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Core.sol#L113-L124
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/Operator.sol#L181-L203
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L78-L103
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L16-L39

Vulnerability details

Impact

When calling the following Core.unregisterOperatorFromDSS function, the specified unregistrationHookData is used as an input for calling the DSS's unregistrationHook function. When the DSS needs unregistrationHookData to contain certain content or be encoded for certain types, the operator can specify unregistrationHookData in which the content or types resulted from decoding such unregistrationHookData in the DSS's unregistrationHook function do not match the DSS's expected content or types, causing the DSS's unregistrationHook function to revert. Alternatively, the operator can specify unregistrationHookData to be memory-intensive enough to cause the processing of the specified unregistrationHookData to consume more gas than self.hookCallGasLimit, which also reverts the DSS's unregistrationHook function.

Since the Core.unregisterOperatorFromDSS function calls the HookLib.callHookIfInterfaceImplemented function with the ignoreFailure input being true, reverting the DSS's unregistrationHook function call does not revert the operator's Core.unregisterOperatorFromDSS function call. However, the DSS's unregistrationHook function's logics can be important to such DSS, such as for keeping correct accounting of the operators that have been unregistered from such DSS. As a result, the operator is able to DOS the DSS's unregistrationHook function by specifying arbitrary unregistrationHookData while not reverting its own Core.unregisterOperatorFromDSS function call.

https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Core.sol#L113-L124

    function unregisterOperatorFromDSS(IDSS dss, bytes memory unregistrationHookData)
        external
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_CORE_UNREGISTER_FROM_DSS)
    {
        address operator = msg.sender;
        CoreLib.Storage storage self = _self();
        self.checkIfOperatorIsRegInRegDSS(operator, dss);
        self.unregisterOperatorFromDSS(dss, operator, unregistrationHookData);

        emit UnregisteredOperatorToDSS(operator, address(dss));
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/Operator.sol#L181-L203

    function unregisterOperatorFromDSS(
        CoreLib.Storage storage self,
        IDSS dss,
        address operator,
        bytes memory unregistrationHookData
    ) external {
        State storage operatorState = self.operatorState[operator];
        // Checks if all operator delegations are zero
        address[] memory vaults = getVaultsStakedToDSS(operatorState, dss);
        if (vaults.length != 0) revert AllVaultsNotUnstakedFromDSS();
        if (!isOperatorRegisteredToDSS(self, operator, dss)) revert OperatorNotValidatingForDSS();

        self.operatorState[operator].dssMap.remove(address(dss));
        HookLib.callHookIfInterfaceImplemented({
            dss: dss,
            data: abi.encodeWithSelector(dss.unregistrationHook.selector, operator, unregistrationHookData),
            interfaceId: dss.unregistrationHook.selector,
            ignoreFailure: true, // So it can't prevent the operator from unregistering
            hookCallGasLimit: self.hookCallGasLimit,
            supportsInterfaceGasLimit: self.supportsInterfaceGasLimit,
            hookGasBuffer: self.hookGasBuffer
        });
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L78-L103

    function callHookIfInterfaceImplemented(
        IERC165 dss,
        bytes memory data,
        bytes4 interfaceId,
        bool ignoreFailure,
        uint32 hookCallGasLimit,
        uint32 supportsInterfaceGasLimit,
        uint32 hookGasBuffer
    ) internal returns (bool) {
        if (gasleft() < (supportsInterfaceGasLimit * 64 / 63 + hookGasBuffer)) {
            revert NotEnoughGas();
        }

        (bool success, bytes32 result) = performLowLevelCallAndLimitReturnData(
            address(dss),
            abi.encodeWithSelector(IERC165.supportsInterface.selector, interfaceId),
            supportsInterfaceGasLimit
        );

        if (!success || result == bytes32(0)) {
            // Either call failed or interface isn't implemented
            emit InterfaceNotSupported();
            return false;
        }
        return callHook(address(dss), data, ignoreFailure, hookCallGasLimit, hookGasBuffer);
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L16-L39

    function performLowLevelCallAndLimitReturnData(address target, bytes memory data, uint256 gasLimit)
        internal
        returns (bool success, bytes32 returnVal)
    {
        bytes32[1] memory returnData;

        assembly {
            // pointer(data) + 0x20 is where actual data is available
            // pointer(data) contains the size of the data in bytes
            // returnData is where the return value is written to
            // we limit size of return value to 32 bytes (same as the size of `returnData` above)
            success :=
                call(
                    gasLimit, // gas available to the inner call
                    target, // address of contract being called
                    0, // ETH (denominated in WEI) being transferred in this call
                    add(data, 0x20), // Pointer to actual data (i.e. 32 bytes offset from `data`)
                    mload(data), // Size of actual data (i.e. the value stored in the first 32 bytes at `data`)
                    returnData, // Free pointer as a buffer for the inner call to write the return value
                    32 // 32 bytes size limit for the return value
                )
        }
        returnVal = returnData[0];
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Operator A is registered to DSS A.
2. DSS A's unregistrationHook function contains logics for keeping track of the operators that have been unregistered from DSS A and decoding the received unregistrationHookData into separate variables for emitting events.
3. Operator A uses the abi.encodePacked function to create the unregistrationHookData and provides such unregistrationHookData when calling the Core.unregisterOperatorFromDSS function.
4. When Operator A's Core.unregisterOperatorFromDSS transaction calls DSS A's unregistrationHook function, decoding such unregistrationHookData reverts, which also reverts DSS A's unregistrationHook function.
5. Since the Core.unregisterOperatorFromDSS function calls the HookLib.callHookIfInterfaceImplemented function with the ignoreFailure input being true, reverting DSS A's unregistrationHook function call does not revert Operator A's Core.unregisterOperatorFromDSS transaction.
6. As a result, DSS A fails to record Operator A as one of its unregistered operators but Operator A's Core.unregisterOperatorFromDSS transaction succeeds.

Tools Used

Manual Review

Recommended Mitigation Steps

The Core.unregisterOperatorFromDSS function can be updated to disallow the operator from specifying arbitrary unregistrationHookData; instead, the Core.unregisterOperatorFromDSS function can specify a determined set of input variables for calling the DSS's unregistrationHook function so the DSS would implement its unregistrationHook function in a way that accommodates such input variables.

Assessed type

DoS

[devdks25]

Ideally that's bestowed upon the DSS on how to handle it, the reason for ignoring the failure is to prevent a malicious DSS from blocking unregistering/unstaking the operator from DSS.
@dewpe

[dewpe]

Would reclassify as low because the unregistration hook itself has no guarantee of running successfully hence the allow revert flag being enabled. Ultimately the implementation of the hooks is up to the DSS and the best we can provide is a recommendation to try-catch inside the handler for this specific hook when they are taking in custom data from the unregistrationHookData param just incase junk data is passed in

[c4-sponsor]

@devdks25 Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[devdks25]

Fix: https://github.com/karak-network/karak-restaking/pull/373

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

Agree with the sponsor.

This issue and its duplicates rely on speculation on future code - the DSS implementation is not in-scope. It is the responsibility of the DSS to handle the case where finalizeUpdateVaultStakeInDSS() does not revert even when finishUpdateStakeHook() does.

Therefore, I believe this is QA at best.

[c4-judge]

MiloTruck marked the issue as grade-b