shaka - redeem function can return less assets than expected

[sherlock-admin]

shaka

high

redeem function can return less assets than expected

Summary

The redeem function of the LMPVault contract can return less assets than the value returned by previewRedeem, which breaks the ERC-4626 standard compatibility.

Vulnerability Detail

The LMPVault contract is a tokenized vaults that according to the README file should comply with the ERC-4626 standard.

As so, it has a redeem function that burns the shares from owner and sends the corresponding amount of assets of underlying tokens to the receiver.

422    function redeem(
423        uint256 shares,
424        address receiver,
425        address owner
426    ) public virtual override nonReentrant noNavDecrease ensureNoNavOps returns (uint256 assets) {
427        uint256 maxShares = maxRedeem(owner);
428        if (shares > maxShares) {
429            revert ERC4626ExceededMaxRedeem(owner, shares, maxShares);
430        }
431        uint256 possibleAssets = previewRedeem(shares);
432
433        assets = _withdraw(possibleAssets, shares, receiver, owner);
434    }

After checking that the number of shares does not exceed the maximum allowed, the previewRedeem is called to calculate the expected amount of assets that will be sent to the receiver.
Then _withdraw is called to calculate and send the actual amount of assets. The issue here is that this amount can be less than the expected amount, and the function will not revert. This is taken into account in the withdraw function, that performs the following check.

416        if (actualAssets < assets) {
417            revert TooFewAssets(assets, actualAssets);
418        }

However, this check is not performed in the redeem function, which means that the receiver can receive less assets than expected, and the owner will still be charged the full amount of shares. This breaks the ERC-4626 standard compatibility, which regarding the previewRedeem function states that:

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the same transaction.

In the documentation and more explicitly in a Discord comment from the protocol team it is stated that

user should interact just with router. He can also interact directly with Vault but he has to handle any potential issues from this on his own.

However, this does not change the fact that if the user decides to interact directly with the vault he might receive less assets than expected, as the LMPVault contract is supposed to follow the ERC-4626 standard, which is not the case. There is also the possibility that other protocols want to interact with Tokemak vaults and they only support interaction with ERC-4626 vaults, so they are not able to interact with the router, as its functions are not compatible with the ERC-4626 standard. This is probably the reason why the calls to the redeem and other functions of the LMPVault contract are not restricted to be called only by the router contract, but open to any address.

Impact

A user or a protocol that interacts with the LMPVault directly might receive less assets than expected, as the redeem can return less assets than the value returned by previewRedeem, because the contract is not following the ERC-4626 standard. In the case of protocol integrations, they might run into accounting issues.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L422-L434

Tool used

Manual Review

Recommendation

File: src/vault/LMPVault.sol

-       uint256 possibleAssets = previewRedeem(shares);
+       uint256 assets = previewRedeem(shares);

-       uint256 possibleAssets = previewRedeem(shares);
+       actualAssets = _withdraw(assets, shares, receiver, owner);
+       if (actualAssets < assets) {
+           revert TooFewAssets(assets, actualAssets);
+       }

Duplicate of #577

[sherlock-admin2]

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

Trumpero commented:

invalid, uses should interact with the router to avoid unexpected scenario like sharesOut = 0, redeemAmount < expected, ...
Users can also interact directly with Vault but he has to handle any potential issues from this on his own.