Stakers unfairly forfeit their unattributed node balance included in the slashed assets.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L425-L446
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L482
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L448-L474

Vulnerability details

Impact

Stakers may lose unattributed node balance included in the slashed assets.

Proof of Concept

Assuming slashing occurs while a staker has an unattributed node balance (or some Ether is withdrawn to nodeAddress after slashing), the slashed assets will be transferred from nodeAddress to slashStore at L436. This may include some unattributed node balance, as slashedWithdrawable is only restricted to node.nodeAddress.balance at L433.

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L425-L446

425:     function _transferToSlashStore(address nodeOwner) internal {
426:         NativeVaultLib.Storage storage self = _state();
427:         NativeVaultLib.NativeNode storage node = self.ownerToNode[nodeOwner];
428: 
429:         // slashed ETH = total restaked ETH (node + beacon) - share price equivalent ETH
430:@>       uint256 slashedAssets = node.totalRestakedETH - convertToAssets(balanceOf(nodeOwner));
431: 
432:         // sweepable ETH = min(ETH available on node address, total slashed ETH)
433:@>       uint256 slashedWithdrawable = Math.min(node.nodeAddress.balance, slashedAssets);
434: 
435:         // withdraw sweepable ETH to slashStore
436:@>       INativeNode(node.nodeAddress).withdraw(self.slashStore, slashedWithdrawable);
437: 
438:         // update total restaked ETH available (node + beacon)
439:@>       node.totalRestakedETH -= slashedWithdrawable;
440: 
441:         // update withdrawable credited ETH available on the node only when node balance
442:         // decreases to less than that of the credited node ETH
443:         if (node.nodeAddress.balance < node.withdrawableCreditedNodeETH) {
444:             node.withdrawableCreditedNodeETH = node.nodeAddress.balance;
445:         }
446:     }

The unattributed node balance included in the slashed assets signifies an additional loss of Ether for the staker due to the following two factors:

Slashing has already reduced the total assets of the NativeVault and node.totalRestakedETH is also decreased in L439.

The unattributed node balance included in the slashed assets is not added to the totalDeltaWei at L482, because the nodeBalanceWei is calculated at L459 after the nodeAddress.balance is already decreased at L456.

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L482

482:        int256 totalDeltaWei = int256(snapshot.nodeBalanceWei) + snapshot.balanceDeltaWei;

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L448-L474

448:     function _startSnapshot(NativeVaultLib.NativeNode storage node, bool revertIfNoBalanceChange, address nodeOwner)
449:         internal
450:     {
                [...]
456:@>       _transferToSlashStore(nodeOwner);
457: 
458:         // Calculate unattributed node balance
459:@>       uint256 nodeBalanceWei = node.nodeAddress.balance - node.withdrawableCreditedNodeETH;
                [...]
474:     }

As a result, the staker unfairly forfeits his unallocated node balance included in the slashed assets.

Suppose the following scenario:

1. Alice is a staker and he has a node N.
   The state of node N is:
   totalRestakedETH is 32 ether,
   nodeAddress.balance is 1ether,
   withdrawableCreditedNodeETH is 0.
   (The unattributed node balance is 1-0=1 ether)
2. Slashing occurs and the slashedAssets in L430 is calculated as 1 ether. So all the unattributed node balance is included in the slashed assets.
3. Alice unfairly forfeits 1 ether:
   1 ether is transferred from his nodeAddress to slashStore.
   Another 1 ether is lost, since the totalRestakedETH and convertToAssets(balanceOf(nodeOwner)) is decreased from 32 ether to 31 ethers.
   Now, Alice can withdraw as most 31 ethers, which is less than the expected value 32 ethers.

Tools Used

Manual review

Recommended Mitigation Steps

There are two recommendations.

Recommendation 1:
The node balance change should be calculated before calling _transferToSlashStore().

    function _startSnapshot(NativeVaultLib.NativeNode storage node, bool revertIfNoBalanceChange, address nodeOwner)
        internal
    {
        if (node.currentSnapshotTimestamp != 0) revert PendingIncompleteSnapshot();
+       uint256 nodeBalanceDeltaWei = node.nodeAddress.balance - node.withdrawableCreditedNodeETH;
        // Sweep slashed ETH from node balance before locking in snapshot node balance.
        // This allows all the ETH that was theoritically slashed in `slashAssets` function call can be moved to a slash store,
        // which can decide what to do with this slashed ETH.
        _transferToSlashStore(nodeOwner);

        // Calculate unattributed node balance
        uint256 nodeBalanceWei = node.nodeAddress.balance - node.withdrawableCreditedNodeETH;

        if (revertIfNoBalanceChange && nodeBalanceWei == 0) revert NoBalanceUpdateToSnapshot();

        NativeVaultLib.Snapshot memory snapshot = NativeVaultLib.Snapshot({
            parentBeaconBlockRoot: _getParentBlockRoot(uint64(block.timestamp)),
            nodeBalanceWei: nodeBalanceWei,
-           balanceDeltaWei: 0,
+           balanceDeltaWei: nodeBalanceDeltaWei,
            remainingProofs: node.activeValidatorCount
        });
                [...]
    }

    function _updateSnapshot(
                [...]
-           int256 totalDeltaWei = int256(snapshot.nodeBalanceWei) + snapshot.balanceDeltaWei;
+           int256 totalDeltaWei = snapshot.balanceDeltaWei;
                [...]
    }

Recommendation 2:
slashedWithdrawable can be limited not to be greater than node.withdrawableCreditedNodeETH.

-       uint256 slashedWithdrawable = Math.min(node.nodeAddress.balance, slashedAssets);
+       uint256 slashedWithdrawable = Math.min(node.withdrawableCreditedNodeETH, slashedAssets);
+       node.withdrawableCreditedNodeETH -= slashedWithdrawable;

Assessed type

Other

[dewpe]

We think is a high, as it is marked

[MiloTruck]

Great find!

The crux of this issue is that nodeBalanceWei is calculated after _transferToSlashStore() is called, so node.nodeAddress.balance will have already decreased before the unattributed balance can be added to totalRestakedETH.

This causes a loss of funds for the node owner as he does not receive shares for the unattributed balance that was lost, as such, I believe high severity is appropriate.

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[c4-judge]

MiloTruck marked issue #102 as primary and marked this issue as a duplicate of 102