Operators can finalize vault stake updates even when unregistered from DSS, disrupting DSS slashing ruquests

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/Core.sol#L146 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/entities/Operator.sol#L126 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/entities/Operator.sol#L1 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/Core.sol#L228

Vulnerability details

Impact

Anyone can finalize an operator's stake vault update request, even if the operator is unregistered from a given DSS, which later prevents the DSS from slashing these updates. An operator can initiate a Core::requestUpdateVaultStakeInDSS() call, then unregister from the DSS and still call Core::finalizeUpdateVaultStakeInDSS() successfully updating their stake records, signaling that there are staked funds towards the DSS for a particular vault belonging to the operator. From this setup, the assumption is probably that the hook calls made in Operator::validateAndUpdateVaultStakeInDSS() will validate the operator status, however, the call to the hook is sent with ignoreFailure: true, meaning that even if HookLib::callHookIfInterfaceImplemented() fails, a revert won't happen, allowing the update on the operatorState.vaultStakedInDssMap data to propagate through. Because of this, any slashing requests made by the DSS towards this status update will revert, as Core::requestSlashing() requires the slashing request's operator to be registered to the DSS.

This issue was reported in a previous audit of Karak, made by Renascence. Both the auditing team and the protocol have confirmed that it should be fixed, however, it appears to be still valid.

Proof of Concept

https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/Core.sol#L146 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/entities/Operator.sol#L126 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/entities/Operator.sol#L128 https://github.com/code-423n4/2024-07-karak/blob/a9658bccc884eb72538875c3940c6b507e06d4ca/src/Core.sol#L228

The following test case can be added in operatorDSS.t.sol and run with forge test --mt test_DSS_cannot_slash_unregistered_operator_stake:

PoC

```solidity function test_DSS_cannot_slash_unregistered_operator_stake() public { deposit_into_vaults(); address requestedVault = address(vaults[0]); address requestedVault1 = address(vaults[1]); Operator.StakeUpdateRequest memory stakeUpdate = Operator.StakeUpdateRequest({vault: requestedVault, dss: dss, toStake: true}); Operator.StakeUpdateRequest memory stakeUpdate1 = Operator.StakeUpdateRequest({vault: requestedVault1, dss: dss, toStake: true}); // Operator requests to stake into DSS vm.startPrank(operator); Operator.QueuedStakeUpdate memory queuedStakeUpdate = core.requestUpdateVaultStakeInDSS(stakeUpdate); Operator.QueuedStakeUpdate memory queuedStakeUpdate1 = core.requestUpdateVaultStakeInDSS(stakeUpdate1); bytes memory unregisterData = abi.encode("unregister", operator); // Operator unregisters from DSS core.unregisterOperatorFromDSS(dss, unregisterData); vm.stopPrank(); address alice = address(1); vm.warp(block.timestamp + Constants.MIN_STAKE_UPDATE_DELAY); // Arbitrary user finalizes the stake update // No reverts happen even if the operator is unregistered vm.startPrank(alice); core.finalizeUpdateVaultStakeInDSS(queuedStakeUpdate); core.finalizeUpdateVaultStakeInDSS(queuedStakeUpdate1); vm.stopPrank(); // The core system signals for valid vault changes towards the unregistered operator assertEq(core.fetchVaultsStakedInDSS(operator, dss)[0], address(vaults[0])); assertEq(core.fetchVaultsStakedInDSS(operator, dss)[1], address(vaults[1])); uint96 slashPercentageWad = uint96(Constants.HUNDRED_PERCENT_WAD); uint96[] memory slashPercentagesWad = new uint96[](vaults.length); slashPercentagesWad[0] = slashPercentageWad; slashPercentagesWad[1] = slashPercentageWad; (address[] memory operatorVaults) = core.fetchVaultsStakedInDSS(operator, dss); SlasherLib.SlashRequest memory slashingReq = SlasherLib.SlashRequest({ operator: operator, slashPercentagesWad: slashPercentagesWad, vaults: operatorVaults }); // DSS requests slashing but operator is not registered to DSS so it fails vm.expectRevert(OperatorNotValidatingForDSS.selector); vm.prank(address(dss)); core.requestSlashing(slashingReq); } ```

Tools Used

Manual review

Recommended Mitigation Steps

There are two approaches to fixing this issue:

1. If the Core is supposed to validate operator validity, then add the self.checkIfOperatorIsRegInRegDSS(queuedStake.operator, queuedStake.updateRequest.dss); to the Core::finalizeUpdateVaultStakeInDSS() function, which will allow registered users to have their update requests finalized.

@@ -148,6 +150,7 @@ contract Core is IBeacon, ICore, OwnableRoles, Initializable, ReentrancyGuard, P
         nonReentrant
         whenFunctionNotPaused(Constants.PAUSE_CORE_FINALIZE_STAKE_UPDATE)
     {
+        self.checkIfOperatorIsRegInRegDSS(queuedStake.operator, queuedStake.updateRequest.dss);
         _self().validateAndUpdateVaultStakeInDSS(queuedStake);
         emit FinishedStakeUpdate(queuedStake);
     }

2. If the hook calls are supposed to handle operator validation, then send ignoreFailure: false in Operator::validateAndUpdateVaultStakeInDSS() so that if there are operator discrepancies, the request will revert.

diff --git a/src/entities/Operator.sol b/src/entities/Operator.sol
index 31a3964..b40f9f1 100644
--- a/src/entities/Operator.sol
+++ b/src/entities/Operator.sol
@@ -125,7 +125,7 @@ library Operator {
             dss: dss,
             data: abi.encodeWithSelector(dss.finishUpdateStakeHook.selector, msg.sender),
             interfaceId: dss.finishUpdateStakeHook.selector,
-            ignoreFailure: true,
+            ignoreFailure: false,
             hookCallGasLimit: self.hookCallGasLimit,
             supportsInterfaceGasLimit: self.supportsInterfaceGasLimit,
             hookGasBuffer: self.hookGasBuffer

Assessed type

Access Control

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Out of scope

[Welith]

@MiloTruck Hello! Why is this marked as out-of-scope? The alleviation of the issue was marked both as resolved by the previous auditing team and the protocol, but the problem is still present. Where is it stated that previous audit findings are not valid for submission? Thanks!

[MiloTruck]

For this contest, issues that have been reported in previous audits before were publicly announced to be OOS:

https://discord.com/channels/810916927919620096/1260968277035843607/1267792210863067221

To be fair to everyone who did not submit after seeing this announcement, I'm abiding by this and invalidating all previously reported issues.

Feel free to open an issue in the org repo if you think previously reported issues should be considered in-scope, but unfortunately, I can't change a decision that was already made in the past.