Slashing NativeVault will lead to locked ETH for the users

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L238
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L277
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L348
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L512

Vulnerability details

Impact

The Karak protocol includes a slashing mechanism that allows the contract owner to penalize stakers by reducing their staked assets in the event of malicious activity by the operator. If a user with a staked balance of 32 ETH is subject to a 3 ETH slashing, they should ideally be able to withdraw the remaining 29 ETH. However, due to a flaw in the implementation, when the user attempts to fully withdraw their ETH, they are only able to withdraw less than the actual remaining amount, with some ETH becoming permanently locked in the protocol.

Specifically, if all share tokens are burned during the withdrawal process, the user cannot access the remaining locked ETH. This results in users receiving fewer ETH than they are entitled to, with the excess ETH becoming inaccessible and permanently locked within the protocol.

Proof of Concept

Consider the following simplified scenario, where Alice want to restake her 32 ETH into Karak protocol, using 1 validator and no rewards acumulated for simplicity. In this example, although Alice initially had 32 ETH staked and should be able to withdraw 29 ETH (after a 3 ETH slashing), she ends up receiving only 26 ETH. The remaining 3 ETH becomes permanently locked in the contract.
The steps she need to perform are the following:

1. Staking and Initial Setup:

   • Alice call createNode to point her withdrawal credentials to it
   • She then calls validateWithdrawalCredentials followed by startSnapshot and validateSnapshotProofs.
     After these actions, the state will look like this:
     totalAssets = 32e18
totalSupply = 32e18
totalRestakedETH = 32e18 (in her Node struct)

2. Slashing:

   • a slashing event occurs for 3 ETH, redusing the totalAssets to 29e18.

3. Withdraw from Beacon Chain:

   • Alice decides to withdraw all her ETH from the Beacon Chain, resulting in her node balance being 32 ETH.
   • To withdraw her ETH from the node, she performs the following operations: startSnapshot, validateSnapshotProofs, startWithdraw, and finishWithdraw.

4. Starting Snapshot

   • When startSnapshot is executed, _transferToSlashStore is called, slashing 3 ETH from her node and transferring them to SlashStore. This reduces totalRestakedETH to 29e18 in her Node struct and saves the new snapshot with nodeBalanceWei = 29e18.

5. Validate snapshot proofs

   • Alice calls validateSnapshotProofs to validate her balance proofs:

            int256 balanceDeltaWei = self.validateSnapshotProof(
                nodeOwner, validatorDetails, balanceContainer.containerRoot, balanceProofs[i]
            );

• balanceDeltaWei = -32e18 (the difference between newBalanceWei of 0 and prevBalanceWei of 32e18)
• snapshot.remainigProofs = 0 since Alice has only one validator
• After validation _updateSnapshot finalize snapshot due to no remaining active validators, where the balance of the user will be decreased due to the following calculation:

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

• This reuslt in 29e18 - 32e18 = -3e18, reducing Alice's shares by 3e18. The state after execution will be:
  node.totalRestakedETH: 26e18
node.withdrawableCreditedNodeETH: 29e18
balanceOf(alice) = 28689655172413793104
totalAssets = 26e18
totalSupply = 28689655172413793104

6. Starting Withdrawal
   • Alice calls startWithdraw. There is a check to prevent a user from withdrawing more than they actually can, but in Alice's situation, it will be less than what she should withdraw:

if (weiAmount > withdrawableWei(msg.sender) - self.nodeOwnerToWithdrawAmount[msg.sender]) {
            revert WithdrawMoreThanMax();
        }

• withdrawableWei(msg.sender) will return the minimum between:

function withdrawableWei(address nodeOwner) public view nodeExists(nodeOwner) returns (uint256) {
        return
            Math.min(convertToAssets(balanceOf(nodeOwner)), _state().ownerToNode[nodeOwner].withdrawableCreditedNodeETH);
    }

• convertToAssets is calculated as:
  (shares * (totalAssets + 1)) / (totalSupply + 1) which is:
  (28689655172413793104 * (26e18 + 1)) / (28689655172413793104 + 1) = 26e18 while
  _state().ownerToNode[nodeOwner].withdrawableCreditedNodeETH is 29e18 (the actual withdrawable wei after slashing)

7. Finishing Withdrawal:
   • finishWithdrawal sends the maximum withdrawableWei (26e18) to Alice, burning all her tokens and causing 3 ETH to remain stuck in the node.

Recommended Mitigation Steps

Due to the complexity and the current limitations in testing the implementation of the slashing mechanism, a precise fix is difficult to pinpoint. In the example provided above, the problem occurs at step 5 where _decreaseBalance is called again reducing the totalAssets by another 3e18 (the slashed amount). If totalAssets are not decreased again and stays 29e18 in the next step , withdrawableWei(msg.sender) would correctly return the minimum between 28999999999999999999 and 29000000000000000000.
However, to address the root cause of the issue, a better mechanism for handling slashing should be implemented and tested.

Assessed type

Other

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[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.