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

0 stars 0 forks source link

Front-running Vulnerability in NativeVault Snapshot Process #64

Closed howlbot-integration[bot] closed 2 months ago

howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Intro

The NativeVault contract's snapshot process is vulnerable to a front-running attack that could be exploited by a malicious node owner. This vulnerability stems from the ability of any node owner to initiate a snapshot and the subsequent ability for anyone to submit validator proofs for that snapshot. This will lead to a myriad of issues like denial of service and griefing attacks.

Details

Attack Vector

A malicious node owner can affect pending snapshots of other users through the following mechanism:

  1. Monitor the mempool for startSnapshot transactions from other node owners.
  2. Front-run these transactions with their own startSnapshot call.
  3. Quickly follow up with validateSnapshotProofs calls using manipulated proofs.

Impact

  1. Snapshot Overwriting:

    • When a malicious actor front-runs a legitimate startSnapshot call, it overwrites the currentSnapshotTimestamp and currentSnapshot in the contract's state.
    • This effectively cancels the pending snapshot of the legitimate user, as the contract now considers the malicious actor's snapshot as the current one.
  2. Proof Invalidation Dos:

    • The validateSnapshotProofs function checks against the currentSnapshotTimestamp:
if (validatorDetails.lastBalanceUpdateTimestamp >= node.currentSnapshotTimestamp) {
    revert ValidatorAlreadyProved();
}
  1. Balance Update Manipulation:

    • The attacker can submit manipulated proofs that misrepresent the balance changes of validators.
    • This affects the snapshot.balanceDeltaWei calculation, which directly impacts the balance updates for all users.
  2. Griefing through Delayed Balance Updates:

  1. Incorrect Balance Calculations:

    • If the attacker successfully submits manipulated proofs, it could lead to:
    • Overstating of the attacker's balance increases
    • Understating of the attacker's balance decreases
    • Potential understating of other users' balance increases
    • Possible overstating of other users' balance decreases
  2. Economic Losses:

    • Users might miss out on timely withdrawal opportunities due to understated balances.

Code Sections

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

function startSnapshot(bool revertIfNoBalanceChange)
    external
    nonReentrant
    nodeExists(msg.sender)
    whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_START_SNAPSHOT)
{
    _startSnapshot(_state().ownerToNode[msg.sender], revertIfNoBalanceChange, msg.sender);
}

function validateSnapshotProofs(
    address nodeOwner,
    BeaconProofs.BalanceProof[] calldata balanceProofs,
    BeaconProofs.BalanceContainer calldata balanceContainer
)
    external
    nonReentrant
    nodeExists(nodeOwner)
    whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_SNAPSHOT)
{

}

Recommendations

  1. Replace the current snapshot system with a queue, where snapshot requests are processed in order.
  2. Enforce a minimum time between snapshots for each node.
  3. Implement a commit-reveal scheme for snapshot initiation to prevent front-running.

Assessed type

Other

c4-judge commented 2 months ago

MiloTruck marked the issue as not a duplicate

c4-judge commented 2 months ago

MiloTruck marked the issue as unsatisfactory: Invalid

MiloTruck commented 2 months ago

Invalid.

There is no issue with anyone submitting proofs for other node owners; the proofs are validated against the beacon chain in validateSnapshotProofs() and cannot be falsified.

anthonyshiks commented 2 months ago

Hi @MiloTruck ,

Thank you for your review of our report. While we appreciate your insight that proofs cannot be falsified due to validation against the beacon chain, we believe the core of our concern remains valid. Let me elaborate:

  1. Front-running vulnerability: The startSnapshot function can be front-run, allowing an attacker to manipulate the order of snapshots:
function startSnapshot(bool revertIfNoBalanceChange)
    external
    nonReentrant
    nodeExists(msg.sender)
    whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_START_SNAPSHOT)
{
    _startSnapshot(_state().ownerToNode[msg.sender], revertIfNoBalanceChange, msg.sender);
}

There's no protection against a malicious actor front-running this call.

  1. Snapshot overwriting: The _startSnapshot function overwrites any existing snapshot without checks:
function _startSnapshot(NativeVaultLib.NativeNode storage node, bool revertIfNoBalanceChange, address nodeOwner)
    internal
{
    if (node.currentSnapshotTimestamp != 0) revert PendingIncompleteSnapshot();
    // ... (snapshot creation logic)
    node.currentSnapshotTimestamp = uint64(block.timestamp);
    _updateSnapshot(node, snapshot, nodeOwner);
}
  1. Proof submission by anyone: The validateSnapshotProofs function allows anyone to submit proofs for any node:
function validateSnapshotProofs(
    address nodeOwner,
    BeaconProofs.BalanceProof[] calldata balanceProofs,
    BeaconProofs.BalanceContainer calldata balanceContainer
)
    external
    nonReentrant
    nodeExists(nodeOwner)
    whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_VALIDATE_SNAPSHOT)
{
    // ... (proof validation logic)
}

While proofs are validated, this still allows an attacker to control the timing of balance updates.

The attack scenario:

  1. Attacker monitors the mempool for startSnapshot transactions.
  2. When a legitimate user attempts to start a snapshot, the attacker front-runs with their own startSnapshot.
  3. The attacker quickly follows up with validateSnapshotProofs calls.
  4. This prevents the legitimate user from starting their snapshot (due to PendingIncompleteSnapshot check).
  5. The attacker controls when balance updates occur, potentially delaying or rushing updates to their advantage.

Impact:

While balances can't be falsified, the ability to control snapshot timing and order is a significant vulnerability that could be exploited for various malicious purposes.

Proposed mitigations:

  1. Implement a queue system for snapshots.
  2. Enforce a minimum time between snapshots for each node.
  3. Use a commit-reveal scheme for snapshot initiation.

We believe this issue remains valid and significant, and should be at least medium severity based on impact.

Sincerely, NexusAudits

MiloTruck commented 1 month ago

This is still invalid.

A snapshot can only be started for a node by its node owner as startSnapshot() uses msg.sender:

    function startSnapshot(bool revertIfNoBalanceChange)
        external
        nonReentrant
        nodeExists(msg.sender)
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_START_SNAPSHOT)
    {
        _startSnapshot(_state().ownerToNode[msg.sender], revertIfNoBalanceChange, msg.sender);
    }