Attackers can conduct a permanent DoS on others' snapshots.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L126-L162
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L448-L474
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L476-L495
https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/NativeVaultLib.sol#L110-L144

Vulnerability details

Impact

Attackers can conduct a permanent DoS on others' snapshots.

Proof of Concept

Let's consider the following scenario:

1. Alice last called the startSnapshot() function 8 days ago and is about to invoke it again.

2. Bob, the attacker, executes the following two functions in succession by front-running Alice within the same block:

   • The validateExpiredSnapshot() function, using nodeOwner set to Alice.
   • The validateSnapshotProofs() function, with nodeOwner as Alice and balanceProofs containing all of Alice's validators.

   As a result:

   • The snapshot started by Bob is finalized. At this point:

     validatorDetails.lastBalanceUpdateTimestamp is set to node.currentSnapshotTimestamp, which is block.timestamp.

     This occurs for all of Alice's active validators during the NativeVaultLib.validateSnapshotProof() function call within the validateSnapshotProofs() function.

     https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/NativeVaultLib.sol#L118-L125

             uint64 timestamp = self.ownerToNode[nodeOwner].currentSnapshotTimestamp;
                 [...]
             validatorDetails.lastBalanceUpdateTimestamp = timestamp;

     node.currentSnapshotTimestamp is set to 0 at L487(https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L487).

     https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L487

             delete node.currentSnapshotTimestamp;

     This happens after all validators are proven, during the _updateSnapshot() function call within the validateSnapshotProofs() function.

3. Finally, Alice's transaction to call the startSnapshot() function succeeds, as node.currentSnapshotTimestamp == 0. So, node.currentSnapshotTimestamp is set to block.timestamp at L470, which results in validatorDetails.lastBalanceUpdateTimestamp == node.currentSnapshotTimestamp == block.timestamp for all the active validators.

   https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L470

           node.currentSnapshotTimestamp = uint64(block.timestamp);

From this point forward, Alice's call to validateSnapshotProofs() will always revert at L149, as the values of validatorDetails.lastBalanceUpdateTimestamp and node.currentSnapshotTimestamp are now identical. As a result, the validators can never be proven and snapshot.remainingProofs is forever stuck at a non-zero value, leading to a permanent DoS on Alice's snapshot.

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L149-L151

    function validateSnapshotProofs(
        ...
            if (validatorDetails.lastBalanceUpdateTimestamp >= node.currentSnapshotTimestamp) {
                revert ValidatorAlreadyProved();
            }
        ...

If this occurs, stakers won't be able to withdraw all ETH staked to the validators. This is because the value of withdrawableCreditedNodeETH will never increase since it only increases by snapshot. As a result, the withdrawableCreditedNodeETH can't include all withdrawn ethers from validators to nodeAddress. So, the withdrawableWei() function doesn't return proper value.

    function withdrawableWei(address nodeOwner) public view nodeExists(nodeOwner) returns (uint256) {
        return
            Math.min(convertToAssets(balanceOf(nodeOwner)), _state().ownerToNode[nodeOwner].withdrawableCreditedNodeETH);
    }

This attack is available even when the nodeOwner calls startSnapshot() with the parameter revertIfNoBalanceChange as true. The attacker can circumvent a revert by donating 1 wei to nodeAddress between steps 2 and 3. (Donations are feasible because the NativeNode inherits from the Ownable contract, which includes the payable function cancelOwnershipHandover(), allowing anyone to call it. Even in the absence of the payable function cancelOwnershipHandover(), there are alternative methods available.)

Tools Used

Manual review

Recommended Mitigation Steps

There should be a mechanism in place to prevent double snapshots from occurring within the same block.

Assessed type

DoS

[c4-judge]

MiloTruck marked the issue as not a duplicate

[c4-judge]

MiloTruck marked the issue as primary issue

[MiloTruck]

The crux of this issue is an attacker front-running a user's call to startSnapshot() to call validateExpiredSnapshot() followed by validateSnapshotProofs(), causing two snapshots to be started in the same block:

Bob, the attacker, executes the following two functions in succession by front-running Alice within the same block:

• The validateExpiredSnapshot() function, using nodeOwner set to Alice.
• The validateSnapshotProofs() function, with nodeOwner as Alice and balanceProofs containing all of Alice's validators.

However, I don't believe it is possible to start a snapshot and call validateSnapshotProofs() to prove validators in the same block. This is because it should be impossible to generate proofs for a block root returned by _getParentBlockRoot() in a future block, so an attacker wouldn't know what proofs to call validateSnapshotProofs() with.

Will check with the sponsor though.

A simple fix would be to revert in _startSnapshot() if node.lastSnapshotTimestamp == block.timestamp.

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid

[karan-andalusia]

Fixed this by updating _startSnapshot to include the following check:

if (node.lastSnapshotTimestamp == uint64(block.timestamp)) revert AttemptedSnapshotInSameBlock();

[KupiaSecAdmin]

@MiloTruck

This issue, along with H-7 from the previous audit, presents identical possibilities and impacts.
It is difficult but possible to generate the proofs for a block root returned by _getParentBlockRoot() in a future block, particularly for miners.
It’s important to note that 'difficult' does not equate to 'unlikely' but reflects the computational complexity involved. The likelihood of this issue occurring cannot exceed that of a sandwich attack.
Moreover, attackers can explore numerous potential sets of proofs. If one of these proofs is valid, the attack can succeed.

Therefore, I think this issue is indeed valid.

[MiloTruck]

If you're referring to this line in H-7:

it is difficult to generate proofs for a block root returned by _getParentBlockRoot() in a future block.

It was written by me.

It's not possible because an attacker does not know which block his and the victim's transactions will get included in, and therefore, he will not know which parent block root to generate proofs for.

Even if the attacker was a validator, he would have to:

• Somehow be able to propose a block with his transaction and the victim's transaction.
• Compute proofs with the previous block's root.
• Submit his attack transaction.
• Build and propose his block.

All under 12 seconds.

Moreover, attackers can explore numerous potential sets of proofs. If one of these proofs is valid, the attack can succeed.

The number of potential sets of proofs is infinite, it's not possible for one of them to randomly be valid.

Given the numerous unlikely conditions required for this to ever occur, I believe QA is appropriate.

If you believe validators can pull this off, you need to be able to explain in detail what occurs on the beacon chain at the consensus level. Otherwise, this is just hypothetical speculation.

[c4-judge]

MiloTruck removed the grade

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[c4-judge]

MiloTruck marked the issue as grade-b