Delayed Slashing Window and Lack of Transparency for Pending Slashes Could Lead to Loss of Funds

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/Core.sol#L248
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/SlashingHandler.sol#L52
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/entities/SlasherLib.sol#L79

Vulnerability details

Vulnerability Details:

Stakers can provide liquidity by choosing an operator and vault to deposit their assets with. In return, they receive shares based on their deposited amount relative to the total assets in the vault.

DSSs have the right to slash vaults if it feels that an operator has failed to perform its tasks adequately. DSSs are subjected to a delay of 2 days (represented by VETO_WINDOW) before a slashing can be finalized. This allows the slashing committee to veto a slashing event if it feels that the slashing was unfair.

The handleSlashing function transfers the slashed assets from the vault, effectively reducing the total assets in the vault, which slashes each shareholder relative to the number of shares they own.

    function handleSlashing(IERC20 token, uint256 amount) external {
        if (amount == 0) revert ZeroAmount();
        if (!_config().supportedAssets[token]) revert UnsupportedAsset();

        SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount);
        // Below is where custom logic for each asset lives
        SafeTransferLib.safeTransfer(address(token), address(0), amount);
    }

The problem with the current system is that the two-day veto window introduces a time gap between when a slash is confirmed and when it is finalized. The slash request records the amount that should be slashed depending on the slash percentage and total assets in the fetchEarmarkedStakes function. Therefore, it is fair to assume that the shareholders at that timestamp should be slashed. However, currently, users who deposit between the slash request and when it is executed will also be slashed and lose value.

Furthermore, the protocol currently lacks any getter methods to warn users of pending slashes for vaults, meaning users could unknowingly deposit into such vaults and lose value in a short period.

In the worst-case scenario, if a vault is fully slashed and the total assets become zero, since the total supply will still be non-zero and previous users will still own shares, any new deposits will instantly lose value, and some funds will be incorrectly allocated to previous users.

Impact:

The current implementation can lead to unfair slashing of users who deposit between the slashing request and its finalization. Users unknowingly depositing into vaults with pending slashes will lose value. With no getter methods available, there is no way for users to identify such vaults and avoid potential losses.

Tools Used:

• Manual analysis

Recommendation:

• Implement getter methods to allow users to check for pending slashes on vaults before making deposits.
• If a vault is fully slashed, consider disabling deposits to avoid users from losing value. If disabling deposits is not feasible, make sure users are warned of a slashing event or that the vault is empty with shares still in it, which could cause losses.

Assessed type

Other

[devdks25]

We plan on running indexers for operator related metrics.

If a vault is fully slashed, consider disabling deposits to avoid users from losing value. If disabling deposits is not feasible, make sure users are warned of a slashing event or that the vault is empty with shares still in it, which could cause losses.

Ideally no staker should deposit in such vault, but still the vault's deposit can be paused.
@dewpe

[dewpe]

Would reclassify to a non-issue

This is more of a frontend and indexer issue instead of a contract/protocol issue. Operators are already performing due diligence prior to delegating to an DSS. Part of their DD would be if there is any pending slashings. The frontend would make it obvious that there are pending slashings that could affect a users share price.

We could add helper functions on the querier to help protocols building on top have the same observability.

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[MiloTruck]

The warden has demonstrated how stakers that deposit while a slash is ongoing will lose funds.

Although it is technically the responsibility of stakers to ensure the vault's current state does not cause them to lose funds when depositing, it isn't apparent to the regular user that an ongoing slash will cause a loss of funds for new deposits. I also believe it isn't documented anywhere that users must check if a slash is ongoing before depositing into vaults prior to the contest.

As such, I am inclined to award this as medium severity.

[devdks25]

To mitigate this the vault deposits will be paused by the core during a queued slashing event and will be unpaused post finalizeSlashing/cancelSlashing.
Additionally a deposit_OVERRIDE_SLASH_PROTECTION method will be exposed to prevent griefing.

[devdks25]

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