When malicious behavior occurs and DSS requests slashing against vault during 2 day period after SLASHING_WINDOW of 7 days is passed after staker initiates a withdrawal, token amount to be slashed is calculated to be higher than what it should be

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/interfaces/Constants.sol#L13-L16
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L94-L124
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L79-L92
https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Vault.sol#L157-L188
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/VaultLib.sol#L24-L38
https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Core.sol#L248-L256
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L126-L151

Vulnerability details

Impact

According to https://github.com/code-423n4/2024-07-karak?tab=readme-ov-file#stakers, Stakers can initiate a withdrawal, subject to a ``MIN_WITHDRAW_DELAY`` of 9 days, and DSS can slash any malicious behavior occurring before the withdrawal initiation for up to 7 days. Since MIN_WITHDRAW_DELAY equals SLASHING_WINDOW + SLASHING_VETO_WINDOW, the staker's withdrawal should be safe without being associated with any malicious behavior and hence not slashable when the DSS does not request any slashing against the corresponding vault within the SLASHING_WINDOW of 7 days after such withdrawal is initiated.

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/interfaces/Constants.sol#L13-L16

    uint256 public constant SLASHING_WINDOW = 7 days;
    uint256 public constant SLASHING_VETO_WINDOW = 2 days;
    uint256 public constant MIN_STAKE_UPDATE_DELAY = SLASHING_WINDOW + SLASHING_VETO_WINDOW;
    uint256 public constant MIN_WITHDRAWAL_DELAY = SLASHING_WINDOW + SLASHING_VETO_WINDOW;

During the 2 day period after the SLASHING_WINDOW of 7 days is passed after the staker's withdrawal is initiated, such as when just couple minutes are passed after such SLASHING_WINDOW is reached, the staker's withdrawal cannot be finished but a malicious behavior can occur and the DSS can make a slashing request against the corresponding vault; in this situation, the staker's withdrawal mentioned previously should not be subject to such slashing though. At that time, when the DSS calls the Core.requestSlashing function, which further calls the SlasherLib.requestSlashing and SlasherLib.fetchEarmarkedStakes functions, the token amount to be slashed is calculated as the DSS's allowed slashing percentage of the vault's totalAssets(), which includes the token amount corresponding to the staker's withdrawal. Because the staker's withdrawal should not be slashable, the token amount to be slashed actually should be the DSS's allowed slashing percentage of a value that equals the vault's totalAssets() minus the token amount corresponding to the staker's withdrawal. Thus, the DSS can unfairly slash more underlying token amount from the vault than it should be allowed.

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L94-L124

    function requestSlashing(
        CoreLib.Storage storage self,
        IDSS dss,
        SlashRequest memory slashingMetadata,
        uint256 nonce
    ) external returns (QueuedSlashing memory queuedSlashing) {
        validateRequestSlashingParams(self, slashingMetadata, dss);
        uint256[] memory earmarkedStakes = fetchEarmarkedStakes(slashingMetadata);
        queuedSlashing = QueuedSlashing({
            dss: dss,
            timestamp: uint96(block.timestamp),
            operator: slashingMetadata.operator,
            vaults: slashingMetadata.vaults,
            earmarkedStakes: earmarkedStakes,
            nonce: nonce
        });
        self.slashingRequests[calculateRoot(queuedSlashing)] = true;
        ...
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L79-L92

    function fetchEarmarkedStakes(SlashRequest memory slashingMetadata)
        internal
        view
        returns (uint256[] memory earmarkedStakes)
    {
        earmarkedStakes = new uint256[](slashingMetadata.vaults.length);
        for (uint256 i = 0; i < slashingMetadata.vaults.length; ++i) {
            earmarkedStakes[i] = Math.mulDiv(
                slashingMetadata.slashPercentagesWad[i],
                IKarakBaseVault(slashingMetadata.vaults[i]).totalAssets(),
                Constants.MAX_SLASHING_PERCENT_WAD
            );
        }
    }

Moreover, when the MIN_WITHDRAW_DELAY of 9 days is passed after the staker's withdrawal is initiated, the staker can call the Vault.finishRedeem function for finishing his withdrawal request. Since SLASHING_VETO_WINDOW is 2 days, the DSS can call the Core.finalizeSlashing function for finalizing its slashing request just after the MIN_WITHDRAW_DELAY of 9 days is passed after the initiation of the staker's withdrawal if the DSS's slashing request was made when just couple minutes were passed after the SLASHING_WINDOW for such withdrawal of the staker was reached. Because both the staker's Vault.finishRedeem transaction and the DSS's Core.finalizeSlashing transaction are sent at the similar time, a malicious miner can place and execute the DSS's Core.finalizeSlashing transaction before the staker's Vault.finishRedeem transaction. In this case, the staker's withdrawal can be unfairly slashed by the DSS even though it should not be slashed.

https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Vault.sol#L157-L188

    function finishRedeem(bytes32 withdrawalKey)
        external
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_VAULT_FINISH_REDEEM)
    {
        (VaultLib.State storage state, VaultLib.Config storage config) = _storage();

        WithdrawLib.QueuedWithdrawal memory startedWithdrawal = state.validateQueuedWithdrawal(withdrawalKey);
        ...
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/VaultLib.sol#L24-L38

    function validateQueuedWithdrawal(State storage self, bytes32 withdrawalKey)
        internal
        view
        returns (WithdrawLib.QueuedWithdrawal memory qdWithdrawal)
    {
        qdWithdrawal = self.withdrawalMap[withdrawalKey];
        ...
        if (qdWithdrawal.start + Constants.MIN_WITHDRAWAL_DELAY > block.timestamp) {
            revert MinWithdrawDelayNotPassed();
        }
    }

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

    function finalizeSlashing(SlasherLib.QueuedSlashing memory queuedSlashing)
        external
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_CORE_FINALIZE_SLASHING)
    {
        _self().finalizeSlashing(queuedSlashing);
        ...
    }

https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/SlasherLib.sol#L126-L151

    function finalizeSlashing(CoreLib.Storage storage self, QueuedSlashing memory queuedSlashing) external {
        ...
        if (queuedSlashing.timestamp + Constants.SLASHING_VETO_WINDOW > block.timestamp) {
            revert MinSlashingDelayNotPassed();
        }
        ...
    }

Proof of Concept

The following steps can occur for the first described scenario where the DSS unfairly slashes more underlying token amount from the vault than it should be allowed.

1. The vault holds 100 underlying tokens and has only one staker who owns 100 shares.
2. On Day 0, the staker initiates his withdrawal for redeeming 50 shares.
3. During the SLASHING_WINDOW of 7 days between Day 0 and Day 7, no malicious behavior occurs, and the DSS does not request any slashing against the vault.
   • Therefore, the staker's withdrawal is safe and should not be slashable, and the staker should receive 50 * 100 / 100 = 50 underlying tokens for his withdrawal.
4. On Day 8, a malicious behavior occurs, and the DSS with a 50% allowed slashing percentage requests to slash the vault.
   • The token amount to be slashed by the DSS is calculated to be 50% * 100 = 50 underlying tokens.
   • However, because the staker's withdrawal initiated on Day 0 should not be subject to such slashing, the token amount to be slashed by the DSS should equal 50% * (100 - 50) = 25 underlying tokens.
5. On Day 9, the MIN_WITHDRAW_DELAY of 9 days is passed after the initiation of the staker's withdrawal so the staker finishes his withdrawal and does receive 50 * 100 / 100 = 50 underlying tokens from the vault.
6. On Day 10, the DSS finalizes the slashing and slashes 50 underlying tokens calculated on Day 8, which is more than 25 underlying tokens that it should slash, from the vault. Therefore, the DSS unfairly slashes more underlying tokens from the vault than it should be allowed.

The following steps can occur for the second described scenario where the staker's withdrawal can be unfairly slashed by the DSS even though it should not be slashed.

1. The vault holds 150 underlying tokens and has two stakers in which Staker A owns 100 shares and Staker B owns 50 shares.
2. On Day 0, Staker A initiates his withdrawal for redeeming 100 shares.
3. During the SLASHING_WINDOW of 7 days between Day 0 and Day 7, no malicious behavior occurs, and the DSS does not request any slashing against the vault.
   • Therefore, Staker A's withdrawal is safe and should not be slashable, and Staker A should receive 100 * 150 / (100 + 50) = 100 underlying tokens for his withdrawal.
4. A malicious behavior occurs when 3 minutes are passed after Day 7 starts, and the DSS with a 50% allowed slashing percentage requests to slash the vault when 5 minutes are passed after Day 7 starts.
   • The token amount to be slashed by the DSS is calculated to be 50% * 150 = 75 underlying tokens.
   • However, because Staker A's withdrawal initiated on Day 0 should not be subject to such slashing, the token amount to be slashed by the DSS should equal 50% * (150 - 100) = 25 underlying tokens.
5. On Day 9, the MIN_WITHDRAW_DELAY of 9 days is passed after the initiation of Staker A's withdrawal so the staker calls the Vault.finishRedeem function for finishing his withdrawal.
6. When 5 minutes are passed after Day 9 starts, the DSS calls the Core.finalizeSlashing function for finalizing the slashing.
7. Both Staker A's Vault.finishRedeem transaction and the DSS's Core.finalizeSlashing transaction are sent at the similar time, a malicious miner places and executes the DSS's Core.finalizeSlashing transaction before Staker A's Vault.finishRedeem transaction.
8. When the DSS's Core.finalizeSlashing transaction is executed, the DSS slashes 75 underlying tokens calculated in Step 4, which is more than 25 underlying tokens that it should slash, from the vault.
9. When Staker A's Vault.finishRedeem transaction is executed, Staker A receives 100 * (150 - 75) / (100 + 50) = 50 underlying tokens, which is less than 100 underlying tokens that it should receive for his withdrawal, from the vault. Therefore, Staker A's withdrawal is unfairly slashed by the DSS even though it should not be slashed.

Tools Used

Manual Review

Recommended Mitigation Steps

One way to mitigate this issue is to update the Vault.finishRedeem function to allow the staker's withdrawal to be finished after the SLASHING_WINDOW of 7 days is passed after such withdrawal is initiated if the DSS does not request any slashing against the corresponding vault within such SLASHING_WINDOW and only enforce the MIN_WITHDRAW_DELAY of 9 days on the withdrawal if the DSS has requested to slash the corresponding vault within such SLASHING_WINDOW.

Assessed type

Timing

[devdks25]

Ideally the protocol doesn't provides any financial advantage (to any individual) for slashing any operator. So if a staker provides a quite low priority gas fees then it might stay in the mempool for long enough to be slashed for activity which it wasn't responsible for.
So imo the staker's should provide enough priority gas fees for finishRedeem to prevent the aforementioned. @dewpe

[dewpe]

Would re-rate to a non-issue

From a technical standpoint, the user can withdraw on the exact second that the 9th day hits but they could delay it forever if they really wanted. It's ultimately up to them to finish it in a timely manner. On the frontend we can state "hey a slashing has started so withdraw your funds right when the 9th day hits". In normal operations, a DSS would be written so that it slashes users from only the active operator set and that code can be reviewed by stakers and operators before depositing and delegating. If you let them out at 7 days if there isn't a slashing request then you may run into some timing games if there is a slashing request occurs then.

From a philosophical standpoint, DSS contracts would be audited and agreeing to allocate assets to them or to an operator that allocates to them means you agree to whatever slashing conditions the DSS implements. In theory the DSS can slash you for 100% right when you register if it wanted.

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[c4-judge]

MiloTruck changed the severity to 2 (Med Risk)

[c4-judge]

MiloTruck marked the issue as not selected for report

[c4-judge]

MiloTruck marked the issue as duplicate of #7

[c4-judge]

MiloTruck marked the issue as not a duplicate

[MiloTruck]

Agree that the second scenario is unrealistic - it is entirely the staker's responsibility to ensure they withdraw their funds on time. If the staker calls finishRedeem() with a priority fee so low that his transaction remains in the mempool for an extended period of time, it is considered a user mistake.

However, the warden does make a valid point that calculating the amount of assets to slash based on totalAssets() will include assets queued for withdrawal.

@dewpe Isn't it an issue if the DSS ends up slashing a higher percentage of the remaining assets? For example:

• Vault has 100 tokens.
• User requests a withdrawal of 50 tokens.
• DSS calls requestSlashing() to slash 50% of assets, which is calculated as 50 tokens.
• User withdraws 50 tokens.
• finalizeSlashing() slashes the remaining 50 tokens, leaving 0 tokens in the vault.

Shouldn't the calculation in requestSlashing() exclude assets in pending withdrawals that have passed SLASHING_WINDOW? Although in practice this isn't easy to implement.

[c4-judge]

MiloTruck marked the issue as primary issue

[c4-judge]

MiloTruck marked the issue as selected for report

[devdks25]

@MiloTruck, the example seems apt and to mitigate it we are thinking of computing earmarkedStakes during finalizeSlashing, this way all the stakers that are staked will only be slashed.

[devdks25]

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