code-423n4 / 2024-07-karak-validation

0 stars 0 forks source link

Potential Manipulation of Staking Rewards and Slash Avoidance in Vault Deployment #224

Closed c4-bot-4 closed 3 months ago

c4-bot-4 commented 3 months ago

Lines of code

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

Vulnerability details

A user can exploit the current implementation to bypass slashing penalties by manipulating the ETH balance of a node. This allows the user to maintain the same share value despite being slashed, enabling them to withdraw and reinvest the funds repeatedly to earn rewards.

Root Cause The issue lies in the calculation method used by the contract, which combines the node balance and the beacon chain balance. By manipulating the node balance, the user can maintain an inflated effective stake.

Impact

This vulnerability allows users to:

  1. Bypass slashing penalties.
  2. Withdraw and reinvest funds, earning rewards unfairly.
  3. Potentially destabilize the staking protocol by undermining the integrity of the slashing mechanism.

Proof of Concept

In the current implementation, a user can deploy a vault, prove that the vault address matches the withdrawal credentials of their staked ETH: validateWithdrawalCredentials(), and start providing snapshots. When the user (e.g., Alice) knows that their stake is about to be slashed, they can exploit the system to avoid the penalty and manipulate their staking rewards.

  1. Alice stakes 32 ETH in the beacon chain.
  2. She proves the stake and receives shares equivalent to 32 ETH.
  3. Alice's stake gets slashed by 2 ETH, reducing her effective stake to 30 ETH.
  4. Before starting a new snapshot, Alice sends 2 ETH to the node via a self-destruct function or a direct transfer, restoring the node's balance to 32 ETH.

This manipulation makes the system believe that the node still holds 32 ETH, bypassing the slashing penalty:

    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)); //@audit (30 + 2) - 32 = 0

        // 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;
        }
    }
  1. Alice withdraws the 2 ETH, potentially reinvesting it to earn additional rewards.
  2. She can repeat this process, maintaining her share value while avoiding penalties and maximizing rewards.

Calling validateSnapshotProofs():

    function validateSnapshotProofs(
        address nodeOwner,
        BeaconProofs.BalanceProof[] calldata balanceProofs,
        BeaconProofs.BalanceContainer calldata balanceContainer
    )
        external
        nonReentrant
        nodeExists(nodeOwner)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_SNAPSHOT)
    {
        NativeVaultLib.Storage storage self = _state();
        NativeVaultLib.NativeNode storage node = self.ownerToNode[nodeOwner];
        NativeVaultLib.Snapshot memory snapshot = node.currentSnapshot;

        if (node.currentSnapshotTimestamp == 0) revert NoActiveSnapshot();

        BeaconProofs.validateBalanceContainer(snapshot.parentBeaconBlockRoot, balanceContainer);

        for (uint256 i = 0; i < balanceProofs.length; i++) {
            NativeVaultLib.ValidatorDetails memory validatorDetails =
                node.validatorPubkeyHashToDetails[balanceProofs[i].pubkeyHash];

            if (validatorDetails.status != NativeVaultLib.ValidatorStatus.ACTIVE) revert InactiveValidator();
            if (validatorDetails.lastBalanceUpdateTimestamp >= node.currentSnapshotTimestamp) {
                revert ValidatorAlreadyProved();
            }

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

            snapshot.remainngProofs--;
            snapshot.balanceDeltaWei += balanceDeltaWei;
        }

        _updateSnapshot(node, snapshot, nodeOwner);
    }

This will return delta = -2, and call _updateSnapshot() with -2:

    function _updateSnapshot(
        NativeVaultLib.NativeNode storage node,
        NativeVaultLib.Snapshot memory snapshot,
        address nodeOwner
    ) internal {
        if (snapshot.remainingProofs == 0) {
            int256 totalDeltaWei = int256(snapshot.nodeBalanceWei) + snapshot.balanceDeltaWei;

            node.withdrawableCreditedNodeETH += snapshot.nodeBalanceWei;

            node.lastSnapshotTimestamp = node.currentSnapshotTimestamp;
            delete node.currentSnapshotTimestamp;
            delete node.currentSnapshot;

            _updateBalance(nodeOwner, totalDeltaWei);
            emit SnapshotFinished(nodeOwner, node.nodeAddress, node.lastSnapshotTimestamp, totalDeltaWei);
        } else {
            node.currentSnapshot = snapshot;
        }
    }

Here totalDeltaWei will be equal to 2 - 2 = 0 so it will return this function without any action:

    function _updateBalance(address _of, int256 assets) internal {
        if (assets > 0) {
            _increaseBalance(_of, uint256(assets));
        } else if (assets < 0) {
            _decreaseBalance(_of, uint256(-assets));
        } else {
            return;
        }
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Modify the contract to ensure that slashing penalties are correctly applied to the user's share value and ensure that any decrease in the staked ETH (due to slashing) is accurately reflected in the user's shares and rewards or another resolution can be to introduce checks to verify the balance of the node before starting a snapshot, ensuring it aligns with the expected value post-slashing.

Assessed type

Other

tpiliposian commented 3 months ago

Could someone please take a look at this?