Lines of code

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

Vulnerability details

Impact

The NativeVault::_increaseBalance and NativeVault::_decreaseBalance functions, as currently implemented, could potentially lead to loss of funds due to unchecked slippage. Here are the functions:

function _increaseBalance(address _of, uint256 assets) internal {
    NativeVaultLib.Storage storage self = _state();
    if (assets + self.totalAssets > maxDeposit(_of)) revert DepositMoreThanMax(); 
    uint256 shares = convertToShares(assets);
    _mint(_of, shares);
    self.totalAssets += assets;
    self.ownerToNode[_of].totalRestakedETH += assets;
    ...
}

function _decreaseBalance(address _of, uint256 assets) internal {
    NativeVaultLib.Storage storage self = _state();
    uint256 shares = previewWithdraw(assets); //@audit inconsistent, why isn't this convertToAssets since it's up there using convertToShares?
    _beforeWithdraw(assets, shares);
    _burn(_of, shares);
    self.totalAssets -= assets;
    self.ownerToNode[_of].totalRestakedETH -= assets;
    ...
}

There is several issue with these code snippets:

Inconsistency in share calculation: _increaseBalance uses ERC4626::convertToShares while _decreaseBalance uses ERC4626::previewWithdraw.
_beforeWithdraw is called when decreasing balance which is virtual and internal yet we don't find it overriden to be external, but there's no corresponding call to _afterDeposit after increasing balance.
Quoting Solady implementation for previewMint:

Any unfavorable discrepancy between ERC4626::convertToAssets and ERC4626::previewMint SHOULD be considered slippage in share price or some other type of condition, meaning the depositor will lose assets by minting. Same goes for convertToShares and previewWithdraw See here and here.

Note that while previewRedeem and previewDeposit are overriden to revert with NotImplemented(), previewWithdraw is not which implies the intention for using it to account for slippage losses which is not the case yet.

We understand that code is loosely compliant with ERC4626, but it shouldn't be too loose to the point of loosing funds.

Tools Used

Manual review.

Recommended Mitigation Steps

Refactor the code to account for slippage at least. One possible solution is to revert if previewMint != convertToShares(assets) but this would be too strict for slippage and may always revert. Consider a sweet spot with some tolerated slippage between the 2 functions. Refactor the code to addresses the other inconsistencies as well.

Assessed type

ERC4626

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