WithdrawRequestBase::_getValueOfSplitFinalizedWithdrawRequest does not account for decimals when converting from redeem token to borrow token

Summary

WithdrawRequestBase::_getValueOfSplitFinalizedWithdrawRequest converts its return value from redeem token to borrow token (in case they are different) however the conversion does not account for the case where borrow token decimals are different than redeem token decimals.

Vulnerability Detail

The _getValueOfSplitFinalizedWithdrawRequest function calculates the value of a finalized split withdraw request (in borrow token units). The function calculates the value by taking the proportion of the splitWithdrawRequest totalWithdraw field, and, in case borrowToken is different than redeemToken, converts the value from redeemToken units to borrowToken units:

if (borrowToken == redeemToken) {
    return (s.totalWithdraw * w.vaultShares) / s.totalVaultShares;
} else {
    // Otherwise, apply the proper exchange rate
    (int256 rate, /* */) = Deployments.TRADING_MODULE.getOraclePrice(redeemToken, borrowToken);

    return (s.totalWithdraw * rate.toUint() * w.vaultShares) / 
        (s.totalVaultShares * Constants.EXCHANGE_RATE_PRECISION);
}

root cause

The formula above assumes that redemption token has the same decimals as borrow token, which may not be the case. The following is the decimals breakdown of the calculation:
totalWithdraw (redeem token decimals) * rate (exchange_rate_decimals) * vault shares (internal decimals) / totalVaultShares (internal decimals) * (exchange_rate_decimals) Note that the result is left with redeem token decimals but the value should be in borrow token decimals.

This results in the returned value potentially being orders of magnitude different than the real value (depending on the decimal difference between borrow token and redeem token).

Impact

_getValueOfSplitFinalizedWithdrawRequest is used as part of an account valuation that is later used in the Notional V3 framework for health checks and collateralization rate calculations (see the control flow here below):

_calculateValueOfWithdrawRequest

convertStrategyToUnderlying

VaultValuation::getPrimaryUnderlyingValueOfShare

VaultValuation::getCollateralRatioFactorsStateful

VaultAccountHealth ::checkVaultAccountCollateralRatio

VaultAccountAction

If redeem token decimal precision is considerably higher than borrow token, the account will be over-evaluated which will lead to liquidatable/unhealthy accounts avoiding liquidation and other system blocks. If the redeem token decimal precision is lower than borrow token, healthy accounts may be liquidated or blocked from operating on the system.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/common/WithdrawRequestBase.sol#L65

Tool used

Manual Review

Recommendation

in BaseStakingVault constructor, record the redeem token decimals (in the same way it's being done with staking and borrow tokens) and in _getValueOfSplitFinalizedWithdrawRequest correct the formula to account for redeem token decimals.

Duplicate of #60

sherlock-audit / 2024-06-leveraged-vaults-judging

nirohgo - WithdrawRequestBase::_getValueOfSplitFinalizedWithdrawRequest does not account for decimals when converting from redeem token to borrow token #93