talfao - LMPVault is not entirely compliant with the ERC4626 standard.

[sherlock-admin]

talfao

medium

LMPVault is not entirely compliant with the ERC4626 standard.

Summary

In the current design, the withdraw function of LMPVault will never work if slippage happens, as the previewWithdraw() function is not considering any slippage.

Vulnerability Detail

After a discussion with the protocol team, they declared that their function previewWithdraw() is not implemented correctly because it does not consider a possible negative slippage during withdrawal.
Note: Slippage can happen if different assets are in destination vaults, and they need to be swapped to provide WETH for LMPVault as it is implemented in the withdrawBaseAsset function.

Scenario

The code itself would be really long since you need to modify a lot of lines in LMPVault-Withdraw.t.sol to simulate slippage. However:

1. Bob deposits 6000 assets.
2. Rebalance happens. Assets are put into destination vaults and staked.
3. Bob wants to withdraw 5000 assets.
4. Slippage happens (he will get -1 tokens for swap).
5. Function reverts, user can never withdraw using this function if slippage happens.

Important info:
The previewWithdraw() function should return more shares to be burned in order to get a specific number of assets, in the scenario above he would require 5002 shares to get 5000 assets. However, nothing like this occurs. In the current design, the function does not consider a possible slippage, hence it would always revert as it returns the number of shares just based on the function _convertToShares. We can see inside the LMPVaultRouter.sol in the withdraw function that the user is able to control slippage by providing an argument maxSharesOut.
(https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L73-L90)
However the shares to burn are calculated just upon the amount argument. The maxSharesOut argument is not considered in the underlying function to make the proper withdrawal for the user. => As a result of this information we can declare that the contract does not fully comply with the ERC4626 standard. We can find it out in the following statement from the ERC4626 description itself:

"Note that any unfavorable discrepancy between convertToShares and previewWithdraw SHOULD be considered slippage in share price or some other type of condition, meaning the depositor will lose assets by depositing." (https://eips.ethereum.org/EIPS/eip-4626)

Impact

The current design does not comply with the ERC4626 standard. Furthermore, if slippage happens, it prevents users from withdrawing assets via the withdraw function, as they do not have any option to influence slippage tolerance in that case.

Code Snippet

• https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L367-L369 -> missing slippage tolerance
• In current model, if slippage happens this will revert -> https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L416-L418

Tool used

Manual Review

Recommendation

Implement slippage tolerance in the previewWithdraw function, since it could return more shares which are needed for withdrawing the specific number of assets.
This should work correctly together with the design of LMPVaultRouter.

Duplicate of #577

[sherlock-admin2]

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

Trumpero commented:

invalid, user can use redeem function to dodge the check of withdrawn assets. LMPVault.redeem()