DSS can force operator to pay much more for gas than necessary when calling DSS's hooks

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/CoreLib.sol#L56-L65
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#L48-L64
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/Operator.sol#L181-L203

Vulnerability details

Impact

Different DSSes can have different logics in their hooks. The hooks with complex necessary logics would require high hookCallGasLimit to be called while the hooks with simple necessary logics should only require low hookCallGasLimit to be called. To accommodate all DSSes' hooks, the following CoreLib.updateGasValues function would be called to set self.hookCallGasLimit to a value that is high enough for executing the most gas-intensive hook among all DSSes because every HookLib.callHookIfInterfaceImplemented function call uses self.hookCallGasLimit as the hookCallGasLimit input in this protocol.

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/CoreLib.sol#L56-L65

    function updateGasValues(
        Storage storage self,
        uint32 _hookCallGasLimit,
        uint32 _supportsInterfaceGasLimit,
        uint32 _hookGasBuffer
    ) internal {
        self.hookCallGasLimit = _hookCallGasLimit;
        self.hookGasBuffer = _hookGasBuffer;
        self.supportsInterfaceGasLimit = _supportsInterfaceGasLimit;
    }

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#L48-L64

    function callHook(
        address target,
        bytes memory data,
        bool ignoreFailure,
        uint32 hookCallGasLimit,
        uint32 hookGasBuffer
    ) internal returns (bool) {
        if (gasleft() < (hookCallGasLimit * 64 / 63 + hookGasBuffer)) revert NotEnoughGas();

        (bool success, bytes32 returnData) = performLowLevelCallAndLimitReturnData(target, data, hookCallGasLimit);

        if (!ignoreFailure && !success) revert DSSHookCallReverted(returnData);

        if (success) emit HookCallSucceeded(returnData);
        else emit HookCallFailed(returnData);
        return success;
    }

When self.hookCallGasLimit is high, an operator might feel safe to call a DSS's hook if such hook should have simple necessary logics since the unspent gas can be refunded to the operator. However, such DSS can conditionally execute unnecessary logics through its hook to spend gas up to the high self.hookCallGasLimit. This forces the operator to spend much more gas than necessary. For example, although the Operator.unregisterOperatorFromDSS function calls the HookLib.callHookIfInterfaceImplemented function with ignoreFailure being true so the DSS cannot prevent the operator from unregistering, this does not prevent the unnecessary logics through the DSS's hook from consuming gas up to the high self.hookCallGasLimit that is higher than the gas amount for only executing the hook's necessary logics. As a result, especially when the gas price is high, the DSS can force the operator to pay much more for gas than necessary.

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
        });
    }

Proof of Concept

The following steps can occur for the described scenario.

1. DSS A's unregistrationHook function's necessary logics are complex while DSS B's unregistrationHook function's necessary logics should be simple.
2. self.hookCallGasLimit in the protocol is set to a high value to ensure sufficient gas for calling DSS A's unregistrationHook function.
3. Operator A registers with DSS A, and Operator B registers with DSS B.
4. Operator B calls the Operator.unregisterOperatorFromDSS function to unregister from DSS B.
5. Although the Operator.unregisterOperatorFromDSS function, which calls the HookLib.callHookIfInterfaceImplemented function with ignoreFailure being true, does not allow DSS B to prevent Operator B from unregistering, DSS B can conditionally execute unnecessary logics through its unregistrationHook function to consume gas up to the high self.hookCallGasLimit that is higher than the gas amount for only executing DSS B's unregistrationHook function's necessary logics.
6. Operator B is forced to pay much more for gas than necessary, especially when the gas price is high.

Tools Used

Manual Review

Recommended Mitigation Steps

When a DSS registers in the Core contract, it can provide immutable gas limit values for each hook. Then, when an operator registers to a DSS, the operator can provide expected gas limit values for each of the DSS's hook; if at least one of the operator's expected hook gas limit values is lower than the corresponding hook gas limit value set by the DSS, then the operator's registration to the DSS reverts.

Assessed type

Other

[dewpe]

Reclassify as a non-issue

500K is the global gas limit that a hook call can use. This issue is about hook handlers in the DSS burning up to 500k in gas which is fine since the protocol allocates that to them. In practice, if a DSS has this on the registration hook, the operator would see the relatively high gas price and choose not to participate in the DSS because of that. If they have it in other places, like the two unregistration hooks, the operator can just exit and pay for the higher gasLimit twice max. This is ignoring the fact that no due diligence was done on the DSS prior to it being allocated and the DSS' intent is to be malicious which would stop people from allocating to it in the first place.

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

This is QA at best.

The impact here is that operators are forced to spend more gas, which does not meet the severity categorization for high/medium.

Additionally, operators specify which DSS to register with when calling registerOperatorToDSS() - if the DSS implementation intentionally burns gas, operators can simply choose not to register with it. Operators should also not expect the DSS hook to use less than hookCallGasLimit.

[c4-judge]

MiloTruck marked the issue as grade-b