A snapshot may face a permanent DoS if both a slashing event occurs in the NativeVault and the staker's validator is penalized.

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L126-L162
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L476-L495
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L517-L525
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L507-L515

Vulnerability details

Impact

The staker can still receive rewards even if their validators are empty.

Proof of Concept

Let's consider the following scenario:

1. Alice has a validator whose balance is 32 ETH.

2. A slashing event occurs in the NativeVault, resulting in Alice being slashed by 2 ETH, reducing her withdrawable assets to 30 ETH.

3. Subsequently, Alice's validator loses all its funds (this can happen within the 7-day snapshot period, even within just 1 hour). A snapshot should then be taken to reduce Alice's assets by 32 ETH, which requires calling the validateSnapshotProofs() function for Alice's validator.

       function validateSnapshotProofs(
           address nodeOwner,
           BeaconProofs.BalanceProof[] calldata balanceProofs,
           BeaconProofs.BalanceContainer calldata balanceContainer
       )
           external
           nonReentrant
           nodeExists(nodeOwner)
           whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_SNAPSHOT)
       {
           NativeVaultLib.Storage storage self = _state();
           NativeVaultLib.NativeNode storage node = self.ownerToNode[nodeOwner];
           NativeVaultLib.Snapshot memory snapshot = node.currentSnapshot;
   
           if (node.currentSnapshotTimestamp == 0) revert NoActiveSnapshot();
   
           BeaconProofs.validateBalanceContainer(snapshot.parentBeaconBlockRoot, balanceContainer);
   
           for (uint256 i = 0; i < balanceProofs.length; i++) {
               NativeVaultLib.ValidatorDetails memory validatorDetails =
                   node.validatorPubkeyHashToDetails[balanceProofs[i].pubkeyHash];
   
               if (validatorDetails.status != NativeVaultLib.ValidatorStatus.ACTIVE) revert InactiveValidator();
               if (validatorDetails.lastBalanceUpdateTimestamp >= node.currentSnapshotTimestamp) {
                   revert ValidatorAlreadyProved();
               }
   
   153         int256 balanceDeltaWei = self.validateSnapshotProof(
                   nodeOwner, validatorDetails, balanceContainer.containerRoot, balanceProofs[i]
               );
   
               snapshot.remainingProofs--;
   158         snapshot.balanceDeltaWei += balanceDeltaWei;
           }
   
   161     _updateSnapshot(node, snapshot, nodeOwner);
       }

   In this call, snapshot.balanceDeltaWei is set to -32 ETH, and the _updateSnapshot() function is invoked at L161.

4. In the _updateSnapshot() function, the _updateBalance() function is invoked at L490, with totalDeltaWei = -32 ETH, as noted at L482.

       function _updateSnapshot(
           NativeVaultLib.NativeNode storage node,
           NativeVaultLib.Snapshot memory snapshot,
           address nodeOwner
       ) internal {
           if (snapshot.remainingProofs == 0) {
   482         int256 totalDeltaWei = int256(snapshot.nodeBalanceWei) + snapshot.balanceDeltaWei;
   
               node.withdrawableCreditedNodeETH += snapshot.nodeBalanceWei;
   
               node.lastSnapshotTimestamp = node.currentSnapshotTimestamp;
               delete node.currentSnapshotTimestamp;
               delete node.currentSnapshot;
   
   490         _updateBalance(nodeOwner, totalDeltaWei);
               emit SnapshotFinished(nodeOwner, node.nodeAddress, node.lastSnapshotTimestamp, totalDeltaWei);
           } else {
               node.currentSnapshot = snapshot;
           }
       }

5. Within the _updateBalance() function, the _decreaseBalance() function is called at L521 with a decrease amount of 32 ETH.

       function _updateBalance(address _of, int256 assets) internal {
           if (assets > 0) {
               _increaseBalance(_of, uint256(assets));
           } else if (assets < 0) {
   521         _decreaseBalance(_of, uint256(-assets));
           } else {
               return;
           }
       }

6. In the _decreaseBalance() function, shares are burned at L511.

       function _decreaseBalance(address _of, uint256 assets) internal {
           NativeVaultLib.Storage storage self = _state();
           uint256 shares = previewWithdraw(assets);
           _beforeWithdraw(assets, shares);
   511     _burn(_of, shares);
           self.totalAssets -= assets;
           self.ownerToNode[_of].totalRestakedETH -= assets;
           emit DecreasedBalance(self.ownerToNode[_of].totalRestakedETH);
       }

The problem arises in step 6. Here, the shares being burned correspond to 32 ETH, while Alice's shares only amount to 30 ETH. This discrepancy causes the burn amount to exceed the existing share amount, leading to a reversal of the transaction. As a result, the snapshot for Alice will be permanently DoSed, and her share amount will remain unchanged. Consequently, Alice can still receive rewards even though her validator is empty. In addition, nobody can call validateExpiredSnapshot() for Alice.

A malicious staker could intentionally orchestrate this scenario by creating a validator and engaging in malicious behavior. If the malicious action fails, they would lose all 32 ETH but could still receive rewards.

Tools Used

Manual review

Recommended Mitigation Steps

The _decreaseBalance() function should be enhanced to properly handle cases where the burning amount exceeds the existing share amount.

For example:

    function _decreaseBalance(address _of, uint256 assets) internal {
        NativeVaultLib.Storage storage self = _state();
-       uint256 shares = convertToShares(assets);
+       uint256 shares = Math.min(convertToShares(assets), balanceOf(nodeOwner));
        _beforeWithdraw(assets, shares);
        _burn(_of, shares);
        self.totalAssets -= assets;
        self.ownerToNode[_of].totalRestakedETH -= assets;
        emit DecreasedBalance(self.ownerToNode[_of].totalRestakedETH);
    }

Assessed type

DoS

[c4-judge]

MiloTruck marked the issue as primary issue

[MiloTruck]

Subsequently, Alice's validator loses all its funds (this can happen within the 7-day snapshot period, even within just 1 hour). A snapshot should then be taken to reduce Alice's assets by 32 ETH

A validator getting slashed for its entire effective balance is only possible due to the correlation penalty, which is extremely unlikely. Historically, most slashing penalties have been around 1 ETH.

The following conditions must be met for this issue to occur:

• Karak slashes an operator for X amount, followed by beacon chain slashing for Y amount.
• X +Y must exceed the node owner's total restaked balance.

I believe this is extremely unlikely to occur, and as such, medium severity is appropriate.

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid

[c4-judge]

MiloTruck removed the grade

[c4-judge]

MiloTruck changed the severity to 2 (Med Risk)

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[lanrebayode]

@MiloTruck I do not consider this valid for few reasons;

1. the likelihood of being slashed 100% on beacon is extremely unlikely. here

2. The warden comment below;

A malicious staker could intentionally orchestrate this scenario by creating a validator and engaging in malicious behaviour. If the malicious action fails, they would lose all 32 ETH but could still receive rewards.

It is not reasonable to lose 32ETH on purpose to get rewards that would definitely be lower.

Also, it is clearly stated that, DSS is responsible for reward distribution and it is not just based on shares check comment on #58. It is the DSS responsibility to tell if validator of node is ACTUALLY staked in beacon

3. Validating the snapshot of user is not useful for the protocol, since it has no balance in beacon/node, what will be the essence?

LIKELIHOOD: extremely low
IMPACT: NO impact on protocol since beacon is empty and nodeBalance would have been deducted(sent to slashStore) from the start of snapshot.

At best, this should count as QA.

[TradMod]

Agree with @lanrebayode

PLUS, the fact that the user is not gonna be able to withdraw his rewards in anyway, makes this issue totally invalid because when the snapshot gets expired, users cannot call startWithdrawal:

    function startWithdrawal(address to, uint256 weiAmount)
        external
        nonReentrant
        nodeExists(msg.sender)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_START_WITHDRAWAL)
        returns (bytes32 withdrawalKey)
    {
        // TODO: make more recent snapshot compulsory
        NativeVaultLib.Storage storage self = _state();
        NativeVaultLib.NativeNode storage node = self.ownerToNode[msg.sender];
        if (weiAmount > withdrawableWei(msg.sender) - self.nodeOwnerToWithdrawAmount[msg.sender]) {
            revert WithdrawMoreThanMax();
        }
        self.nodeOwnerToWithdrawAmount[msg.sender] += weiAmount;

@>      if (block.timestamp >= node.lastSnapshotTimestamp + Constants.SNAPSHOT_EXPIRY) {
            revert SnapshotExpired();
        }

        withdrawalKey = NativeVaultLib.calculateWithdrawKey(msg.sender, self.nodeOwnerToWithdrawNonce[msg.sender]++);
        address nodeOwner = msg.sender;

        self.withdrawalMap[withdrawalKey].to = to;
        self.withdrawalMap[withdrawalKey].assets = weiAmount;
        self.withdrawalMap[withdrawalKey].nodeOwner = nodeOwner;
        self.withdrawalMap[withdrawalKey].start = uint96(block.timestamp);

        emit StartedWithdraw(nodeOwner, _config().operator, withdrawalKey, weiAmount, to);

        return withdrawalKey;
    }

[KupiaSecAdmin]

@lanrebayode
You seem to have misunderstood my issue. You mentioned that:

NO impact on protocol since beacon is empty and nodeBalance would have been deducted(sent to slashStore) from the start of snapshot.

This statement is completely incorrect, as the account snapshot for the user is not available, which is the core of this issue and is explained in the report.

@ShaheenRehman
You mentioned that:

PLUS, the fact that the user is not gonna be able to withdraw his rewards in anyway, makes this issue totally invalid because when the snapshot gets expired, users cannot call startWithdrawal:

This statement is incorrect.

First, even though the user cannot currently withdraw their reward, granting rewards to one user results in a loss of funds for other users.

Second, the rewards will become withdrawable once a sufficient amount has been accumulated.

[TradMod]

Doesn't make any sense to me tbh. Rewards Distribution is handled by the DSS, If DSS notices anything malicious he will just jail the operator/validator. Rewards are not automatic.

Anyway, this was a good find ngl, but it have zero impact, infact its a total loss for the operator itself, losing 32 ETH to earn rewards which can be withdrawalable after a few months/years, absolutely makes no sense.

[KupiaSecAdmin]

@ShaheenRehman

You mentioned that:

Rewards Distribution is handled by the DSS, If DSS notices anything malicious he will just jail the operator/validator. Rewards are not automatic.

This is inaccurate because, while the DSS can jail an operator, it cannot target a specific staker. As a result, this could lead to the loss of funds for other stakers within the same nativeVault.

You also mentioned that:

infact its a total loss for the operator itself, losing 32 ETH to earn rewards which can be withdrawalable after a few months/years, absolutely makes no sense.

Losing 32 ETH is a possibility, but it is not the intention of the staker. The risk of losing ETH significantly increases when the staker is a malicious validator. They can, however, mitigate their losses from such malicious actions by staking into the nativeVault.

[MiloTruck]

According to the C4 severity categorization, the criteria for medium severity is:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

As stated in my initial comment, this requires both Karak and beacon chain slashing to occur. The upper limit of how much the DSS can slash an account for is 100%:

    uint256 public constant HUNDRED_PERCENT_WAD = 100e18;
    uint256 public constant MAX_SLASHING_PERCENT_WAD = HUNDRED_PERCENT_WAD;

Therefore, it is possible for the sum of Karak and beacon chain slashing to exceed 100%, however unlikely. I view this as "with stated assumptions, but external requirements".

Regarding impact, validateSnapshotProofs() and validateExpiredSnapshot() will permanently be DOSed for the user - they can't stake into the vault in the future even if they would like to. This fulfills "the function of the protocol or its availability could be impacted".

As such, this remains as medium.