search

sherlock-audit / 2023-10-notional-judging

5 stars 5 forks source link

shealtielanz - `_mintVaultShares()` in `SingleSidedLPVaultBase.sol` can revert unexpectedly, causing DOS to deposits from notional. #28

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

shealtielanz

medium

_mintVaultShares() in SingleSidedLPVaultBase.sol can revert unexpectedly, causing DOS to deposits from notional.

Summary

the call to _mintVaultShares() can revert for a certain vault share greater than type(uint80).max on the call to .toUint80 due to overflow.

Vulnerability Detail

The SingleSidedLPVaultBase contract during the execution of _depositFromNotional() function calls the internal function _mintVaultShares(). Inside this function could be seen casting uint256 typed local variables to uint80 type: https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L229C1-L240C64

    function _mintVaultShares(uint256 lpTokens) internal returns (uint256 vaultShares) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            //@audit the percison used to divide is above the normal percios and would lead to low amounts
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim; //@audit in a situation we have a low enough ppoltoclaim the vault shares can go above uint80.
        }

        // Updates internal storage here
        state.totalPoolClaim += lpTokens;
        state.totalVaultSharesGlobal += vaultShares.toUint80();
        state.setStrategyVaultState();
..SNIP..

This casting could be considered safe only based on the assumption that value of vaultShares returned would always be less than uint80.maxValue. However, we could see that this assumption would be wrong in case of low enough totalPoolClaim amount in the pool.

        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            //@audit the percison used to divide is above the normal percios and would lead to low amounts
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim; //@audit in a situation we have a low enough ppoltoclaim the vault shares can go above uint80.
        }

With a small state.totalPoolClaim where state.totalPoolClaim != 0 , big lpTokens and state.totalVaultSharesGlobal values, the calculation could result in a vaultShares that is greater than uint80.maxValue. This will lead to an overflow on the line state.totalVaultSharesGlobal += vaultShares.toUint80(); causing a revert, resulting to DOS on the call to deposit from notional.

Impact

This would brick deposits from notional on the call to _depositFromNotional()

Code Snippet

Manual Review

Recommendation

Consider changing the state.totalVaultSharesGlobalfield type from uint80 to uint256.

nevillehuang commented 11 months ago

Invalid, insufficient proof. Would be helpful in the future to provide some scenarios (numerical examples) on how this could possibly overflow in production, given uint80 is a huge value especially for representing vault shares. Imo, if a judge needs to explicitly do that to prove your issue, the issue does not have sufficient proof. Additionally, even if it reaches that level of vaultShares to mint, the lp can always separately mint vaultshares via different deposit transactions with no loss of funds and DoS.