Double Decrease of totalAssets During Slashing and Snapshot

[c4-bot-10]

Lines of code

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

Vulnerability details

The slashAssets() and _decreaseBalance() functions in the NativeVault.sol both reduce the totalAssets in the contract, causing an incorrect decrease in the total assets. This results in double-counting the slashed assets, leading to an incorrect final state of total assets and shares.

In the current implementation, when slashAssets is called, it decreases totalAssets by the slashed amount. Following this, and in case there is still no assets in the node to be withdrawn, during the snapshot process, at the end _decreaseBalance is called, which again decreases totalAssets by the same amount, resulting in a double reduction, but shares are being burnt once, only in the _decreaseBalance function.

Impact

This bug causes an incorrect final state of totalAssets and totalShares. For example, starting with 32 ETH assets and 32 shares, if 2 ETH is slashed, the system ends up with 28 ETH (32-2-2) and 30 (32-2) shares, leading to an inaccurate representation of the vault's state. This can affect user balances and protocol functionality, potentially causing financial discrepancies and loss of trust in the system.

Proof of Concept

Let's assume the user has 32 ETH assets staked in the beacon chain and 32 shares and is slashed 2 ETH:

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;
}

Here we have 30 ETH totalassets, and still 32 shares.

Then calling startSnapshot it started calculating which amount it can slash, but as there is no assets in the node, the function processes with 0s:

    function _transferToSlashStore(address nodeOwner) internal {
        NativeVaultLib.Storage storage self = _state();
        NativeVaultLib.NativeNode storage node = self.ownerToNode[nodeOwner];

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

        // 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);

        // update total restaked ETH available (node + beacon)
        node.totalRestakedETH -= slashedWithdrawable;

        // update withdrawable credited ETH available on the node only when node balance
        // decreases to less than that of the credited node ETH
        if (node.nodeAddress.balance < node.withdrawableCreditedNodeETH) {
            node.withdrawableCreditedNodeETH = node.nodeAddress.balance;
        }
    }

    function _startSnapshot(NativeVaultLib.NativeNode storage node, bool revertIfNoBalanceChange, address nodeOwner)
        internal
    {
        if (node.currentSnapshotTimestamp != 0) revert PendingIncompleteSnapshot();

        // 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,
            remainingProofs: node.activeValidatorCount
        });

        node.currentSnapshotTimestamp = uint64(block.timestamp);
        _updateSnapshot(node, snapshot, nodeOwner);

        emit SnapshotCreated(nodeOwner, node.nodeAddress, uint64(block.timestamp), snapshot.parentBeaconBlockRoot);
    }

After this the user calls validateSnapshotProofs(), which in turn calls _updateSnapshot() this in turn calls _updateBalance() with -2 delta, and this calls _decreaseBalance():

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

So now the shares will be 30 (30-2) and totalAssets decreased twice 28 (32-2-2) ETH.

Tools Used

Manual review.

Recommended Mitigation Steps

Ensure that totalAssets is only decreased once during the entire slashing and snapshot process.

Assessed type

Other

[tpiliposian]

Could someone please take a look at this?

[tpiliposian]

This one is similar to the code-423n4/2024-07-karak-findings#31

As far as I understood they can be duplicates.

@MiloTruck Can you please check it?

[MiloTruck]

This is invalid.

After this the user calls validateSnapshotProofs(), which in turn calls _updateSnapshot() this in turn calls _updateBalance() with -2 delta, and this calls _decreaseBalance()

This is incorrect. If Karak slashing occurs, the validator on the beacon chain will still have 32 ETH staked, so balanceDeltaWei is 0. nodeBalanceWei is also 0 since the native node holds no ETH. Therefore, _updateBalance() will be called with 0.

[tpiliposian]

Thank you for the answer, @MiloTruck.

To fully understand, I was thinking about cases where the user does not have 32 ETH in the Beacon Chain, such as in the case of an initial penalty (it is being issued immediately) In this case, the balance would be 31 ETH, so the delta -1 ETH, so at the end, the shares would be 31, but total assets would also be 30. Am I right?

https://eth2book.info/capella/part2/incentives/slashing/

[MiloTruck]

I think you're confusing between Karak slashing and beacon chain slashing. slashAssets() is called when a DSS for Karak performs a slash, the beacon chain balance doesn't change (or at least your report doesn't mention beacon chain slashing at all).

[tpiliposian]

Yeah, I thought the DSS should slash the user when they are penalized/slashed from the beacon chain.