NativeVault.sol :: validateExpiredSnapshot() will always revert, making it impossible for users from initiating a new snapshot.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

validateExpiredSnapshot() is intended to be callable by anyone if the node owner delays starting a new snapshot beyond a specified period, specifically 7 days. However, the function is rendered ineffective because it checks if node.currentSnapshotTimestamp != 0, which always causes the transaction to revert. This issue prevents the initiation of a new snapshot, rendering the function useless.

Proof of Concept

validateExpiredSnapshot() is implemented as follows:

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) {
            revert SnapshotNotExpired();
        }

        _startSnapshot(node, false, nodeOwner);
    }

As you can see, the function checks if the snapshot has expired and, if so, calls _startSnapshot() to initiate a new snapshot.

function _startSnapshot(NativeVaultLib.NativeNode storage node, bool revertIfNoBalanceChange, address nodeOwner)
        internal
    {
        if (node.currentSnapshotTimestamp != 0) revert PendingIncompleteSnapshot();

///...
}

As you can see, for a new snapshot to be created, the previous one must be completed; otherwise, the transaction will revert. This makes the snapshot expiry time ineffective, as users cannot invoke validateExpiredSnapshot(). The only way to initiate a new snapshot is to wait for the current one to finish and then use startSnapshot().

The following POC demonstrates the issue. To run it, copy the code into nativeVault.t.sol.

function test_validateExpiredSnapshotAlwaysRervert(bytes32 parentRoot) public {
        vm.assume(parentRoot != bytes32(0));
        test_validateWithdrawalCredentials();

        address node = nativeVault.createNode();
        assertEq(nativeVault.getNodeOwner(node), address(this));

        vm.store(Constants.BEACON_ROOTS_ADDRESS, timestamp_idx(block.timestamp), bytes32(uint256(block.timestamp)));
        vm.store(Constants.BEACON_ROOTS_ADDRESS, root_idx(block.timestamp), parentRoot);

        //do a snapshot
        nativeVault.startSnapshot(false);

        assertEq(nativeVault.currentSnapshotTimestamp(address(this)), block.timestamp);
        assertEq(nativeVault.currentSnapshot(address(this)).parentBeaconBlockRoot, parentRoot);

        //Increment time to Constants.SNAPSHOT_EXPIRY
        vm.warp(block.timestamp + 7 days);
        vm.expectRevert(PendingIncompleteSnapshot.selector);
        nativeVault.validateExpiredSnapshot(address(this));
    }

Tools Used

Manual review.

Recommended Mitigation Steps

To address the issue, update validateExpiredSnapshot() with the following code:

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) {
            revert SnapshotNotExpired();
        }

+       delete node.currentSnapshotTimestamp;
+       delete node.currentSnapshot;

        _startSnapshot(node, false, nodeOwner);
    }

Assessed type

Other

[dewpe]

would reclassify as a non-issue, comments on why should come soon from @karan-andalusia

[karan-andalusia]

This is by design since only one snapshot can be ongoing at the time. The user won't have any incentive to not complete a snapshot since they will only get the shares minted for their balance changes once their snapshot has finalized. Deleting an ongoing snapshot can be used as a dos vector in this suggestion – lets say a user has an expired snapshot and wants to come in and start a new snapshot themselves. They start their snapshot and then another person comes in and midway between their snapshot calls validateExpiredSnapshot which would delete the users' ongoing snapshot and waste their proven validators since it never finalized. This can be done by the attacker again and again not allowing the user to ever finish the snapshot.

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Invalid

[MiloTruck]

Invalid.

As stated by the sponsor, this is by design. If a current snapshot is ongoing but it is past the 7-day expiry period, anyone can resolve his ongoing snapshot by submitting proofs on his behalf through validateSnapshotProofs().