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

0 stars 0 forks source link

NodeOwners can evade DSS slash in NativeVault. #83

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/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L310-L313 https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/NativeVault.sol#L512

Vulnerability details

Impact

Vaults are liable to slashing once the vault operator acts maliciously. DSS initiates a slash process by first calling Core.requestSlashing(), which must be followed by a veto window period to allow veto committee justify slash request or cancel.

If veto committee finds the request justifiable, anyone can call Core.finalizeSlashing() to finalize the process.

However, it is worth noting that slashing works differently in NativeVault. In a regular vault, if a user tries to escape slash requires by redeeming, it takes 7 days for the user to finalise withdraw(also, shares would have been transferred to vault), if slashing is justified, user cannot escape the slash.

In NativeVault, since funds are on Beacon, finalizing a slash process involves reducing totalAsset by the slash amount, so that node owners' share valuations are reduced in the ratio of slash. (32e18 shares now worth 16e18_ETH after a 50% slash).

The issue with this method is the validation check done in NativeVault.slashAssets() to prevent negative totalAsset.

  function slashAssets(uint256 totalAssetsToSlash, address slashingHandler)
        external
        onlyOwner
        nonReentrant
        whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_SLASHER)
        returns (uint256 transferAmount)
    {
        NativeVaultLib.Storage storage self = _state();

        if (slashingHandler != self.slashStore) revert NotSlashStore();

        // avoid negative totalAssets if slashing amount is greater than totalAssets
@>>>      if (totalAssetsToSlash > self.totalAssets) {
@>>>          totalAssetsToSlash = self.totalAssets;
        }

        self.totalAssets -= totalAssetsToSlash;
        emit Slashed(totalAssetsToSlash);
        return totalAssetsToSlash;
    }

NativeVault uses a virtual asset, it increases when node owners validate a withdrawal credential to the native node address and decreases in any snapshot update that returns a negative change in the validator beacon balance.

This means a validator can monitor a slash request on NativeVault and instantly initiate exit on Beacon before calling validateSnapshotProofs () to burn shares and reduce total assets.

Now, node owner shares have been burnt before the arrival of Eth, and total assets have also been reduced, the node owner then calls startSnapshot() to record the node balance difference and mint shares which can then be used to initiate withdrawal.

Proof of Concept

  1. DSS initiates a slash request on a NativeVault.(48hrs wait!)
  2. Validator(s) instantly initiated exit on Beacon(27hrs wait before processing withdrawal to node).
  3. Node owner(s) calls startSnapshot() before any increament in node balance.
  4. 27hrs later after Beacon have processed withdrawal, node owner calls validateSnapshotProofs() to so that balanceDeltaWei returns -32_ETH(beacon balance change).
  5. Remember node balance is zero, in _updateSnapshot(), ```totalDeltaWei = 0 + (-32_ETH)
  6. Balance is updated with -32_ETH, which leads to burning all shares of node owner and reducing total asset
    function _decreaseBalance(address _of, uint256 assets) internal {
        NativeVaultLib.Storage storage self = _state();
    @>>     uint256 shares = previewWithdraw(assets);
        _beforeWithdraw(assets, shares);
    @>>     _burn(_of, shares);
    @>>     self.totalAssets -= assets; // now zero
    @>>     self.ownerToNode[_of].totalRestakedETH -= assets;
        emit DecreasedBalance(self.ownerToNode[_of].totalRestakedETH);
    }
  7. On finalization of the slash request, total asset is empty, so nothing to remove

    
        // avoid negative totalAssets if slashing amount is greater than totalAssets
        if (totalAssetsToSlash > self.totalAssets) {
            totalAssetsToSlash = self.totalAssets; //@audit nothing is slashed
        }
  8. Node owner then waits till node address have been credited with ETH, then calls startSnapshot()
  9. Nothing happens in _transferToSlashStore() since user restaked and shares = 0.
  10. flow is transferred to _updateSnapshot() with node's new balance, and since remaining proofs = 0(after exit node.validatorCount is reduced), snapshot is finalized with positive deltawei int256 totalDeltaWei = 32 + 0 = 32
  11. _increaseBalance() mints shares worth 32_ETH to node owner, increases total asset and totalRestakedETH of the node.
  12. With the shares, node.lastSnapshotTimestamp up to date, withdraw can be initiated.

Tools Used

Manual review.

Recommended Mitigation Steps

Redesign of NativeVault slashing mechanism.

Assessed type

Invalid Validation

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