Incorrect Slashing Mechanism Leading to Unintended Penalties for Unaffected Users

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L430
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L299-L318

Vulnerability details

The current slashing mechanism does not decrease user shares proportionally to the slashed assets, which is crucial because of the convertToAssets() calculations, and this leads to unintended penalties for users who are not affected by the slashing event. This can result in users being unfairly penalized, impacting their staking rewards.

Impact

This vulnerability results in:

1. Unintended penalties for users not affected by the slashing event.
2. Reduced staking rewards for unaffected users.
3. Potential loss of confidence in the staking protocol due to unfair penalties.

Proof of Concept

Let's suppose there are 3 stakers in the vault, and three users have staked ETH with the following details:

User 1: 32 ETH
User 2: 32 ETH
User 3: 32 ETH
Total Restaked ETH: 96 ETH
Total Assets: 96 ETH
Shares: 32, 32, 32
Total Shares: 96

User 1 gets slashed by 4 ETH.
The system updates the total assets but not the shares:

    function slashAssets(uint256 totalAssetsToSlash, address slashingHandler)
        external
        onlyOwner
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_SLASHER)
        returns (uint256 transferAmount)
    {
        NativeVaultLib.Storage storage self = _state();

        if (slashingHandler != self.slashStore) revert NotSlashStore();

        // avoid negative totalAssets if slashing amount is greater than totalAssets
        if (totalAssetsToSlash > self.totalAssets) {
            totalAssetsToSlash = self.totalAssets;
        }

        self.totalAssets -= totalAssetsToSlash;
        emit Slashed(totalAssetsToSlash);
        return totalAssetsToSlash;
    }

Total Restaked ETH: 32, 32, 32
Total Assets: 92 ETH
Shares: 32, 32, 32
Total Shares: 96

User 2, who has not been slashed, calculates their effective assets: function _transferToSlashStore():

        // slashed ETH = total restaked ETH (node + beacon) - share price equivalent ETH
        uint256 slashedAssets = node.totalRestakedETH - convertToAssets(balanceOf(nodeOwner));

This will be 32 - 32 * 92 / 96 = 32 - 30 = 2, so the slashedAssets is equal to 2, which then will be slashed from the contract:

        // sweepable ETH = min(ETH available on node address, total slashed ETH)
        uint256 slashedWithdrawable = Math.min(node.nodeAddress.balance, slashedAssets);

        // withdraw sweepable ETH to slashStore
        INativeNode(node.nodeAddress).withdraw(self.slashStore, slashedWithdrawable);

Root Cause
The slashing function reduces the total assets but does not proportionally adjust the shares of the users. This imbalance causes unaffected users to experience a reduction in their effective assets during snapshots.

Tools Used

Manual review.

Recommended Mitigation Steps

Modify the slashing function to reduce the shares of the affected user proportionally to the slashed amount.

Assessed type

Other

[tpiliposian]

Could someone please take a look at this?

[tpiliposian]

I believe this issue is a duplicate with:
code-423n4/2024-07-karak-findings#29

The same problem is described in different scenarios.
@MiloTruck Can you please review it once again?

[MiloTruck]

This is not the same issue as code-423n4/2024-07-karak-findings#29, which highlights how the accounting for nodeBalanceWei in _startSnapshot() is incorrect when a slash occurs.

The root cause you have presented is completely different:

The slashing function reduces the total assets but does not proportionally adjust the shares of the users. This imbalance causes unaffected users to experience a reduction in their effective assets during snapshots.

This is invalid as a slash occurs for all users staked in the native vault, not individual users. If a vault is slashed, all users in that vault should lose a portion of their assets.

[tpiliposian]

Ah okay, thanks so much.