search

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

0 stars 0 forks source link

Stakers in the NativeVault could be unfairly slashed. #16

Open c4-bot-3 opened 4 weeks ago

c4-bot-3 commented 4 weeks ago

Lines of code

https://github.com/karak-network/karak-arena-mitigations/blob/475cfd73744cabe239720feec4a227a739910119/src/NativeVault.sol#L509-L517

Vulnerability details

Bug description

Note: Even though the root cause of the issue was not discovered in the previous audit, the presented scenario is the same as in M-02.

Consider a scenario where both Alice and Bob each have 32 ETH restaked into the NativeVault. NativeVault's totalAssets equals 64 ETH. A slashing event occurs in the vault, resulting in the NativeVault being slashed by 2 ETH, reducing totalAssets to 62 ETH.

NativeVault.sol#L312

self.totalAssets -= totalAssetsToSlash;

Now 32 shares of both users amount to 32 * 62 / 64 = 31 ETH, meaning that both Alice and Bob lost 1 ETH due to the slashing. After a slashing event has occurred in the NativeVault, Alice's validator looses all of its funds. Snapshot is started for Alice to reduce her assets by 32 ETH. validateSnapshotProofs() will calculate balanceDeltaWei as -32, subsequently calling _updateSnapshot() function.

NativeVault.sol#L151-L159

    int256 balanceDeltaWei = self.validateSnapshotProof(
        nodeOwner,
        validatorDetails,
        balanceContainer.containerRoot,
        balanceProofs[i]
    );
    snapshot.remainingProofs--;
    snapshot.balanceDeltaWei += balanceDeltaWei;
}
_updateSnapshot(node, snapshot, nodeOwner);

_updateSnapshot() calls _updateBalance(), where _decreaseBalance() function is invoked. _decreaseBalance() will burn all of Alice's shares and reduce totalAssets of the NativeVault by 32 ETH.

NativeVault.sol#L511-L515

uint256 shares = Math.min(convertToShares(assets), balanceOf(_of));
_beforeWithdraw(assets, shares);
_burn(_of, shares);
self.totalAssets -= assets;
self.ownerToNode[_of].totalRestakedETH -= assets;

After transaction has been completed, totalAssets are reduced to 30 ETH, leaving Bob only being able to withdraw 30 ETH, even though he should be able to withdraw 31.

Impact

Loss of funds for a user.

Recommended Mitigation

_decreaseBalance() function should calculate the amount of assets to slash based on the amount of shares of a user.

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

Assessed type

Other

c4-judge commented 3 weeks ago

MiloTruck marked the issue as primary issue

c4-judge commented 3 weeks ago

MiloTruck marked the issue as satisfactory

c4-judge commented 3 weeks ago

MiloTruck marked the issue as selected for report

c4-judge commented 3 weeks ago

MiloTruck marked the issue as new finding

karan-andalusia commented 3 weeks ago

This was identified internally and fixed but the pr was not merged in yet so it didn't make it to the mitigation review: https://github.com/karak-network/karak-restaking/pull/439/files