BPZ - Users will experience a DOS when depositing to a LMPVault after rebalance.

[sherlock-admin2]

BPZ

medium

Users will experience a DOS when depositing to a LMPVault after rebalance.

Summary

Users will experience a DOS when trying to deposit to a LMPVault after rebalance.

Vulnerability Detail

When user deposits its checked that assets is less than maxDeposit. However, because the _maxMint function will return the type(uint256).max value when both the supplyLimit andwalletLimit are set to type(uint256).max value, this assertion will almost always revert when the totalAssets() / supply is greater than 1 because the type(uint256).max will be provided to the _convertToAssets function as the shares parameter, leading to an over-flow error on the assets calculation.

Impact

Users are no longer able to deposit funds on LMPVault contract.

Code Snippet

This test case was extracted from the LMPVault-Withdraw.t.sol. After this
test_withdraw_ReceivesNoMoreThanCachedIfPriceIncreases() it has been added the user deposit code & eventually it DOS due to overflow.

function test_withdraw_ReceivesNoMoreThanCachedIfPriceIncreases() public {
        _accessController.grantRole(Roles.SOLVER_ROLE, address(this));
        _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));

        // User is going to deposit 1000 asset
        _asset.mint(address(this), 1000);
        _asset.approve(address(_lmpVault), 1000);
        _lmpVault.deposit(1000, address(this));
      
  
        // Deployed 200 asset to DV1
        _underlyerOne.mint(address(this), 100);
        _underlyerOne.approve(address(_lmpVault), 100);
        _lmpVault.rebalance(
            address(_destVaultOne),
            address(_underlyerOne), // tokenIn
            100,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            200
        );


        // Deploy 800 asset to DV2
        _underlyerTwo.mint(address(this), 800);
        _underlyerTwo.approve(address(_lmpVault), 800);
        _lmpVault.rebalance(
            address(_destVaultTwo),
            address(_underlyerTwo), // tokenIn
            800,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            800
        );

       

        // Price of DV1 doubled
        _mockRootPrice(address(_underlyerOne), 4e18);

        // Cashing in 900 shares, which means we're entitled to at most 900 assets
        // We can get 400 from DV1, and we'll get the remaining 500 from DV2
        // Should leave us with no shares of DV1 and 300 of DV2

        uint256 balBefore = _asset.balanceOf(address(this));
        // vm.expectEmit(true, true, true, true);
        // emit Withdraw(address(this), address(this), address(this), 425, 500);
        uint256 assets = _lmpVault.redeem(900, address(this), address(this));
        uint256 balAfter = _asset.balanceOf(address(this));

        assertEq(assets, 900, "returned");
        assertEq(balAfter - balBefore, 900, "actual");

 
// User deposit here, DOS due to overflow. 
        address user1 = address(0x1);
        _asset.mint(user1, 700);

        vm.startPrank(user1);
        _asset.approve(address(_lmpVault), 700);
        _lmpVault.deposit(700, user1);

        vm.stopPrank();

    }

Also used console.log to check _convertToAssets function . Also final output values as in terminal as shown below.

  function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
        uint256 supply = totalSupply();
        console.log("supply:", supply);
        console.log("shares:" , shares);
        console.log("totalAssets():" , totalAssets());
        assets = (supply == 0) ? shares : shares.mulDiv(totalAssets(), supply, rounding);
       // assets = (supply == 0) ? shares : shares.mulDiv(1, supply, rounding);
        console.log("assets" , assets);
    }

So here assets = shares x totalAssets() / supply
= 115792089237316195423570985008687907853269984665640564039457584007913129639935 x 300 / 100 ---> Overflow & reverted

Tool used

Foundry

Recommendation

It is recommended to refactor the maxDeposit function to return the type(uint256).max value if the _maxMint function returns this value. This can be implemented by refactoring the maxDeposit function as follows:

    function maxDeposit(address wallet) public view virtual override returns (uint256 maxAssets) {
        // @audit recommended mitigation
        uint256 maxMintAmount = _maxMint(wallet);
        maxAssets = maxMintAmount == type(uint256).max ? maxMintAmount : convertToAssets(maxMintAmount);
    }

Duplicate of #577

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

low, when totalSupplyLimit == perWalletLimit == uint256.max, it's correct that function deposit() is unable to use but the function mint() can be used instead by the users.
Furthermore, when this case happens, the admin can set the limit to another value to mitigate the issue --> No fund are at risk, and temporary DOS