New NodeOwners can be griefed by forcing them to provide proof for an empty snapshot without any shares increase/decrease on their node

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L218-L220

Vulnerability details

Impact

New NodeOwners can be griefed by forcing them to provide proof for an empty snapshot without any shares increase/decrease on their node

The startSnapshot() on a node can only be called by the owner to prevent griefing of NodeOwners, this is to give liberty to nodeOwners on when to updateSnapshot to increase or decrease vault shares.

this is to prevent grievance to a node owner. Lets say you are a node owner at t1 - you finished your snapshot at t2 - without any yields I come in and start a snapshot for your node but don't submit any proofs at t3 - when you have significant yields to create a new snapshot, you realise that an existing snapshot needs to be finalised. So you will have to pay a lot of gas fees (merkle proofs are heavy) to finalize that snapshot without gaining any shares. Then start a new snapshot and submit all the proofs again. Paying double in gas fees.

Also, to prevent NodeOwner from stalling vault share update when they have a slashed validator, they ought to reduce node shares, there is validateExpiredSnapshot() method that allows anyone to start a snapshot for any node that has an expired snapshot.

However, due to missing zero check on node.lastSnapshotTimestamp, a griefer can keep calling validateExpiredSnapshot(address nodeOwner) on every newly deployed node that has validated a withdrawal credential.

Proof of Concept

When nodes are newly deployed, a griefer can call validateExpiredSnapshot(address nodeOwner) immediately after the node owner calls validateWithdrawalCredentials()(to mint initial node shares) that increases node.activeValidatorCount before any award share increase.

This action will force the node owner to spend more on gas to finalize the initial snapshot before starting and finalizing another snapshot.

    function validateExpiredSnapshot(address nodeOwner)
        external
        nonReentrant
        nodeExists(nodeOwner)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_EXPIRED_SNAPSHOT)
    {
        NativeVaultLib.NativeNode storage node = _state().ownerToNode[nodeOwner];
        // Only check if the last snapshot has expired or not
        if (node.lastSnapshotTimestamp + Constants.SNAPSHOT_EXPIRY > block.timestamp) { //@audit check will always pass for newly created nodes
            revert SnapshotNotExpired();
        }

        _startSnapshot(node, false, nodeOwner);
    }

This action will force the node owner to spend more on gas to finalize the initial snapshot before starting and finalizing another snapshot.

Instance

1. NativeNode was deployed and 10-20 validator credentials were verified to it, to increase active validator count and update shares.
2. Node owner expects to initiate snapshot when there's been a substantial increase in beacon actual balance(bal - 32_ETH) so more shares could be minted in NativeVault.....since there is a liberty of 7 days, this is legal
3. A malicious actor initiates snapshot with node owner address to trigger if (node.currentSnapshotTimestamp != 0) revert PendingIncompleteSnapshot(); when node owner tries to start snapshot.
4. The Node owner must now call validateSnapshotProofs() to finalize snapshot for 20! validators, restart again and revalidate to update shares(to avoid losing rewards from DSS)
5. The attacker does the same for multiple node owners, causing severe gas burdens for node owners with high amount of validators.

Tools Used

Manual review

Recommended Mitigation Steps

    function validateExpiredSnapshot(address nodeOwner)
        external
        nonReentrant
        nodeExists(nodeOwner)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_EXPIRED_SNAPSHOT)
    {
        NativeVaultLib.NativeNode storage node = _state().ownerToNode[nodeOwner];
        // Only check if the last snapshot has expired or not
+++ if ( node.lastSnapshotTimestamp == 0 ) revert; 
        if (node.lastSnapshotTimestamp + Constants.SNAPSHOT_EXPIRY > block.timestamp) {
            revert SnapshotNotExpired();
        }

        _startSnapshot(node, false, nodeOwner);
    }

Also, to prevent instances where node owners after increasing validator count and got minted shares stalls shares update due to slashing, initiate node.lastSnapshotTimestamp in createNode()

    function createNode()
        external
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_CREATE_NODE)
        returns (address)
    {
        NativeVaultLib.Storage storage self = _state();
        address newNodeAddr = self.deployNode(_config(), msg.sender);
        self.ownerToNode[msg.sender].nodeAddress = newNodeAddr;
        self.nodeToOwner[newNodeAddr] = msg.sender;
        return newNodeAddr;
+++     self.ownerToNode[msg.sender].lastSnapshotTimestamp =  uint64(block.timestamp);
    }

After creating a node, owners are expected to validate withdrawal credentials. Active Validators are expected to get reward credit in 5days or might have gotten slashed due to inactivity withing 7days period, so node shares should get updated!

Assessed type

Invalid Validation

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

This action will force the node owner to spend more on gas to finalize the initial snapshot before starting and finalizing another snapshot.

This is a good find, but I don't see this issue as high or medium severity - the maximum impact here is causing users to spend more gas, which doesn't fulfil the severity categorization:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

The function of the protocol or its availability is not impacted here as users can still start a new snapshot. Furthermore, the "DOS" can only be performed once on each native node and an attacker has no incentive to pull this off.

[c4-judge]

MiloTruck marked the issue as grade-b

[lanrebayode]

@MiloTruck I think this should pass as medium for few reasons;

1. The likelihood of this issue is high
2. It leads to temporal grieving of new node owners
3. New node owners are forced to incur extra operational cost in form of gas which count as indirect loss of funds(the gas is not as a result of protocol code/implementation, it is because of a malicious actor which could be a owner of another vault). If a nodeOwner had plan to updat snapshot with $200 reward from beacon spending just $50, user will have to spend $100, which obviously has reduced overall profitability of the noweOwner at that point in time. Imagine have to cause 100 new node owner spend extra $50, that amount to $5,000 for no reason!
4. The DOS can be performed once on every nodeOwner of the vault(the cummulative overhead cost on all vault users could be huge)
5. Also, if nodeOwner is not motivated to record snapshot due to slashing, protocol or whoever intends to update snapshot pays double in fees.
6. The ease of action, it is very easy with very little cost.
7. Other vault owners can be incentivised to pull this off on each other, users are left to pay!
8. There is actual leak in value!

[MiloTruck]

The likelihood of this issue is high

Disagree, there's no motivation for an attacker to perform this attack.

If a nodeOwner had plan to updat snapshot with $200 reward from beacon spending just $50, user will have to spend $100, which obviously has reduced overall profitability of the noweOwner at that point in time. Imagine have to cause 100 new node owner spend extra $50, that amount to $5,000 for no reason!

How did you arrive at $50 in gas costs? If you're trying to prove this could cause significant financial losses, please provide evidence and non-hypothetical calculations.

Think my previous comment still stands, I don't believe this meets the severity categorization for medium.