Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blackhole - Incorrect value calculation in _getValueOfSplitFinalizedWithdrawRequest due to missing decimals conversion #27

Closed
sherlock-admin2 opened this issue Jul 3, 2024 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Jul 3, 2024

blackhole

High

Incorrect value calculation in _getValueOfSplitFinalizedWithdrawRequest due to missing decimals conversion

Summary

If a withdraw request is split and finalized, _getValueOfSplitFinalizedWithdrawRequest will be invoked to calculate the value of the split request.
However, the function does not account for the decimals of the borrowToken, which can result in an incorrect value calculation when borrowToken is not redeemToken.

Vulnerability Detail

s.totalWithdraw represents the amount of redeemToken, so when the borrowToken is not equal to redeemToken, the decimals conversion is neccessary for calculating the correct value.

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

Impact

The incorrect value calculation can lead to an incorrect calculation of collateralRatio in notional VaultAcccountHealth, result in a loss of funds for the user.

Code Snippet

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

    function _getValueOfSplitFinalizedWithdrawRequest(
        WithdrawRequest memory w,
        SplitWithdrawRequest memory s,
        address borrowToken,
        address redeemToken
    ) internal virtual view returns (uint256) {
        // If the borrow token and the withdraw token match, then there is no need to apply
        // an exchange rate at this point.
        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); // @audit -high missing redeemDecimals to borrowDecimals conversion
        }
    }

Tool used

Manual Review

Recommendation

It's recommended to add the borrowToken decimals conversion to the _getValueOfSplitFinalizedWithdrawRequest function to calculate the correct value when borrowToken is not equal to redeemToken.

Duplicate of #60

@github-actions github-actions bot closed this as completed Jul 5, 2024
@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jul 5, 2024
@sherlock-admin4 sherlock-admin4 changed the title Fantastic Gunmetal Skunk - Incorrect value calculation in _getValueOfSplitFinalizedWithdrawRequest due to missing decimals conversion blackhole - Incorrect value calculation in _getValueOfSplitFinalizedWithdrawRequest due to missing decimals conversion Jul 11, 2024
@sherlock-admin4 sherlock-admin4 added the Reward A payout will be made for this issue label Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

2 participants