code-423n4 / 2024-09-karak-mitigation-findings

0 stars 0 forks source link

Node owners can circumvent slashing penalties. #33

Closed c4-bot-7 closed 3 weeks ago

c4-bot-7 commented 4 weeks ago

Lines of code

https://github.com/karak-network/karak-restaking/tree/v2/src/entities/NativeVault.sol#L509-L517

Vulnerability details

Impact

Node owners can circumvent slashing penalties.

Proof of Concept

When decreasing balance of a node owner, he may have fewer shares than what should be burned. In this case, all of his shares are burned. However, totalRestakedETH is still reduced by the original amount of assets, which can be exploited by malicious users to circumvent slashing.

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

This vulnerability can be exploited by malicious users to circumvent slashing. While the Merkle tree can reflect the change in the validator's balance immediately upon inclusion of the withdrawal transaction, the actual crediting of unstaked ethers to the withdrawal address may take additional time depending on the transaction's finality and processing.

Consider the following scenario:

  1. Both Alice and Bob each add a validator with 32 ETH and successfully complete a snapshot.

    Alice's totalRestakedETH : 32 ETHER Bob's totalRestakedETH : 32 ETHER totalAssets : 64

  2. Slashing occurs, causing totalAssets to decrease to 60 ETHER.

    totalAssets : 60 Alice's totalRestakedETH : 32 ETHER convertToAssets(balanceOf(Alice)) : 30 ETHER Bob's totalRestakedETH : 32 ETHER convertToAssets(balanceOf(Bob)) : 30 ETHER

  3. Alice requests a full withdrawal from her validator.

  4. The Merkle tree is updated; however, the unstaked ethers are not yet credited to the withdrawal address.

  5. Alice initiates a snapshot, completing it before the unstaked ethers are credited. (On Ethereum, it can typically take anywhere from a few seconds to several minutes. However, in some cases, it might take longer.) Her balance decreases by 32 ETHER. Since Alice does not have enough to burn, all her shares are burned (L511-L512).

    totalAssets : 60 - 32 = 28 Alice's totalRestakedETH: 32 ETHER - 32 ETHER = 0 (L515). Bob's totalRestakedETH : 32 ETHER convertToAssets(balanceOf(Bob)) : 28 ETHER (Bob has all remaining shares.)

  6. Once the unstaked ethers are credited to the withdrawal address, Alice starts another snapshot. Her withdrawable balance is now 32 ETHER.

    totalAssets : 60 Alice's totalRestakedETH : 32 ETHER convertToAssets(balanceOf(Alice)) : 32 ETHER Bob's totalRestakedETH : 32 ETHER convertToAssets(balanceOf(Bob)) : 28 ETHER

In the described scenario, Alice successfully avoids slashing, while Bob incurs an additional loss of 2 ETH.

Tools Used

Manual review

Recommended Mitigation Steps

Only the amount corresponding to burning shares should be subtracted from totalAssets and totalRestakedETH.

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

Assessed type

Other

MiloTruck commented 3 weeks ago
  1. The Merkle tree is updated; however, the unstaked ethers are not yet credited to the withdrawal address.

  2. Alice initiates a snapshot, completing it before the unstaked ethers are credited. (On Ethereum, it can typically take anywhere from a few seconds to several minutes. However, in some cases, it might take longer.)

I don't believe it is possible for the ETH balance of the withdrawal address to not have increased while the validator's balance has already decreased in the parent beacon block root. You're claiming that the execution layer can be outdated while the root of the previous block on the consensus layer has already been updated.

If you think so, please provide some form of proof (eg. from an EIP or the consensus specs).

c4-judge commented 3 weeks ago

MiloTruck marked the issue as unsatisfactory: Invalid

KupiaSecAdmin commented 3 weeks ago

While the Merkle tree can reflect the change in the validator's balance immediately upon inclusion of the withdrawal transaction, the actual crediting of unstaked ethers to the withdrawal address may take additional time depending on the transaction's finality and processing.

  1. Validator's Balance Decrease: When a withdrawal transaction is included in a block on the Beacon Chain, the validator's balance is immediately decreased in the consensus layer's state. This change is reflected in the Merkle tree of the Beacon Chain.

  2. ETH Balance Update Delay: However, the actual update to the ETH balance at the withdrawal address in the execution layer may not occur immediately. This discrepancy can arise due to several factors:

In summary, while the validator's balance is updated immediately upon transaction inclusion, the corresponding update to the withdrawal address's ETH balance may experience delays due to processing times and network conditions. This is an important distinction in understanding how the two layers interact.

MiloTruck commented 3 weeks ago

By "some form of proof", I mean a credible source such as EIPs, client implementations, Ethereum consensus specs, etc...

Your answer is not sufficient proof.