Wrong input parameter passed to DSS hook function - msg.sender instead of operator

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/interfaces/IDSS.sol#L15
https://github.com/code-423n4/2024-07-karak/blob/main/src/Core.sol#L146
https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/Operator.sol#L126

Vulnerability details

Impact

The protocol defines an interface with optional hook functions for DSSes to implement. These functions take in the Operator as an input parameter. By design anyone can finalize a vault staking request by calling finalizeUpdateVaultStakeInDSS(). However, in the function flow, it calls validateAndUpdateVaultStakeInDSS() which passes in msg.sender, instead of the operator as the parameter to the low level call to the DSS.

This is not in line with the specified DSS interface by the protocol. The severity impact depends on the implementation of the DSS, however issues are certain to happen, as this is a stake update function and the DSS expects the operator as input.

Proof of Concept

The protocol specifies the following DSS interface:

https://github.com/code-423n4/2024-07-karak/blob/main/src/interfaces/IDSS.sol#L15

interface IDSS is IERC165 {
    // HOOKS

    function registrationHook(address operator, bytes memory extraData) external;
    function unregistrationHook(address operator, bytes memory extraData) external;

    function requestUpdateStakeHook(address operator, Operator.StakeUpdateRequest memory newStake) external;
    function cancelUpdateStakeHook(address operator, address vault) external;
    function finishUpdateStakeHook(address operator) external;
    function requestSlashingHook(address operator, uint256[] memory slashingPercentagesWad) external;
    function cancelSlashingHook(address operator) external;
    function finishSlashingHook(address operator) external;
}

As we can see, all hooks expect the operator as input. However, by design anyone can finalize a vault stake update request and the finalizeUpdateVaultStakeInDSS() function is callable by anyone:

https://github.com/code-423n4/2024-07-karak/blob/main/src/Core.sol#L146

    /// @notice Allows anyone to finish the queued request for an operator to update assets delegated to a DSS
    /// @dev Only operator can finish their queued request valid only after a
    /// minimum delay of `Constants.MIN_STAKE_UPDATE_DELAY` after starting the request
    function finalizeUpdateVaultStakeInDSS(Operator.QueuedStakeUpdate memory queuedStake)
        external
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_CORE_FINALIZE_STAKE_UPDATE)
    {
        _self().validateAndUpdateVaultStakeInDSS(queuedStake);
        emit FinishedStakeUpdate(queuedStake);
    }

This function in turn calls validateAndUpdateVaultStakeInDSS(), which makes the low level call to finishUpdateStakeHook() with msg.sender as parameter, and not the operator:

https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/Operator.sol#L126

    function validateAndUpdateVaultStakeInDSS(CoreLib.Storage storage self, QueuedStakeUpdate memory queuedStakeUpdate)
        external
    {
        State storage operatorState = self.operatorState[queuedStakeUpdate.operator];
        validateQueuedStakeUpdate(operatorState, queuedStakeUpdate);
        updateVaultStakeInDSS(
            operatorState,
            queuedStakeUpdate.updateRequest.vault,
            queuedStakeUpdate.updateRequest.dss,
            queuedStakeUpdate.updateRequest.toStake
        );
        delete operatorState.pendingStakeUpdates[queuedStakeUpdate.updateRequest.vault];
        IDSS dss = queuedStakeUpdate.updateRequest.dss;
        HookLib.callHookIfInterfaceImplemented({
            dss: dss,
            data: abi.encodeWithSelector(dss.finishUpdateStakeHook.selector, msg.sender),
            interfaceId: dss.finishUpdateStakeHook.selector,
            ignoreFailure: true,
            hookCallGasLimit: self.hookCallGasLimit,
            supportsInterfaceGasLimit: self.supportsInterfaceGasLimit,
            hookGasBuffer: self.hookGasBuffer
        });
    }

Tools Used

Manual review

Recommended Mitigation Steps

Apply this correction:

https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/Operator.sol#L126

    function validateAndUpdateVaultStakeInDSS(CoreLib.Storage storage self, QueuedStakeUpdate memory queuedStakeUpdate)
        external
    {
        State storage operatorState = self.operatorState[queuedStakeUpdate.operator];
        validateQueuedStakeUpdate(operatorState, queuedStakeUpdate);
        updateVaultStakeInDSS(
            operatorState,
            queuedStakeUpdate.updateRequest.vault,
            queuedStakeUpdate.updateRequest.dss,
            queuedStakeUpdate.updateRequest.toStake
        );
        delete operatorState.pendingStakeUpdates[queuedStakeUpdate.updateRequest.vault];
        IDSS dss = queuedStakeUpdate.updateRequest.dss;
        HookLib.callHookIfInterfaceImplemented({
            dss: dss,
+            data: abi.encodeWithSelector(dss.finishUpdateStakeHook.selector, queuedStakeUpdate.operator),
-            data: abi.encodeWithSelector(dss.finishUpdateStakeHook.selector, msg.sender),
            interfaceId: dss.finishUpdateStakeHook.selector,
            ignoreFailure: true,
            hookCallGasLimit: self.hookCallGasLimit,
            supportsInterfaceGasLimit: self.supportsInterfaceGasLimit,
            hookGasBuffer: self.hookGasBuffer
        });
    }

Assessed type

Error

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Out of scope

[MiloTruck]

Out-of-scope as it has been reported in a previous audit.

[c4-judge]

MiloTruck marked the issue as primary issue