An attacker can impose financial losses on other stakers by validating their validators prior to executing a slash.

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L168-L204
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L497-L505
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L299-L318

Vulnerability details

Impact

An attacker can impose financial losses on other stakers by validating their validators prior to executing a slash.

Proof of Concept

The validateWithdrawalCredentials() function lacks a caller check, allowing anyone to invoke it and validate other stakers' validators. This vulnerability can be exploited by attackers to cause financial losses to other stakers.

    function validateWithdrawalCredentials(
        address nodeOwner,
        BeaconProofs.BeaconStateRootProof calldata beaconStateRootProof,
        BeaconProofs.ValidatorFieldsProof[] calldata validatorFieldsProofs
    )
        external
        nonReentrant
        nodeExists(nodeOwner)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_WITHDRAW_CREDS)
    {
        NativeVaultLib.Storage storage self = _state();
        NativeVaultLib.NativeNode storage node = self.ownerToNode[nodeOwner];

        if (beaconStateRootProof.timestamp == block.timestamp) {
            revert BeaconTimestampIsCurrent();
        }
        if (
            beaconStateRootProof.timestamp < node.lastSnapshotTimestamp
                || beaconStateRootProof.timestamp < node.currentSnapshotTimestamp
        ) revert BeaconTimestampTooOld();

        uint256 totalRestakedWei;
        BeaconProofs.validateBeaconStateRootProof(
            _getParentBlockRoot(beaconStateRootProof.timestamp), beaconStateRootProof
        );

        for (uint256 i = 0; i < validatorFieldsProofs.length; i++) {
            totalRestakedWei += self.validateWithdrawalCredentials(
                nodeOwner,
                beaconStateRootProof.timestamp,
                beaconStateRootProof.beaconStateRoot,
                validatorFieldsProofs[i]
            );
        }

203     _increaseBalance(nodeOwner, totalRestakedWei);
    }

Consider the following scenario:

1. Current state of the NativeVault:
   • totalAssets: 64 ETH (Alice: 32 ETH, Bob: 32 ETH)
   • totalSupply: s + s = 2s (Alice's balance is s, and Bob's balance is s, too.)
2. Alice has an unvalidated validator with 32 ETH and there is a pending slashing of 3 ETH, which means Alice and Bob are supposed to be slashed equally by 1.5 ETH each.
3. Bob, a malicious staker, calls the validateWithdrawalCredentials() function to validate Alice's validator. This alters the state during the _increaseBalance() function call L203:

       function _increaseBalance(address _of, uint256 assets) internal {
           ...
   
           _mint(_of, shares);
           self.totalAssets += assets;
           ...

   Updated state:
   • totalAssets: 96 ETH (Alice: 64 ETH, Bob: 32 ETH)
   • totalSupply: 3s (Alice: 2s, Bob: s)
4. The pending slashing is executed:

       function slashAssets(uint256 totalAssetsToSlash, address slashingHandler)
           ...
   
   315     self.totalAssets -= totalAssetsToSlash;
           ...

   Final state:
   • totalAssets: 93 ETH
   • totalSupply: 3s (Alice: 2s, Bob: s)

As a result, Alice's assets are calculated as 93 ETH * 2s / 3s = 62 ETH, meaning she is slashed by 2 ETH(0.5 ETH more than expected).

In the above scenario, the calculations differ slightly from the actual values because the NativeVault inherits from ERC4626, which uses the exchange rate formula (totalAssets + 1) / (totalSupply + 1). However, the discrepancy is minimal and does not significantly impact the reported loss to Alice.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to restrict access to the validateWithdrawalCredentials() function, allowing only the owner of the validator or specific individuals with designated roles to call it.

Assessed type

Other

[c4-judge]

MiloTruck marked the issue as primary issue

[MiloTruck]

Will check with the sponsor, but I believe this is by design.

If Alice has an additional 32 ETH in a validator that points to her native node, this unvalidated 32 ETH should be considered part of her native node and should be slashable. It shouldn't matter if the withdrawal credentials have been validated yet.

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid

[karan-andalusia]

Yes this was by design since Alice might be aware that there is a pending slash and might wait to validate their validator to the node until after the slashing on Karak has happened. A third party can come in and validate their validator which is already pointing to their node for them hence including those shares in the slashing.