0x73696d616f - previewRedeem() is not ERC4626 compliant and might cause unexpected losses for users

[sherlock-admin2]

0x73696d616f

medium

previewRedeem() is not ERC4626 compliant and might cause unexpected losses for users

Summary

previewRedeem() is not ERC4626 compliant and might return more assets than redeem(). This could fish users into taking unexpected losses.

Vulnerability Detail

The EIP4626 states that 'redeem should return the same or more assets as previewRedeem if called in the same transaction'. However, in LMPVault, if there is a loss in one of the destination vaults, the actual redeem() amount would be smaller than previewRedeem(), depending on the underlying asset price. The amount would also differ if a rebalance call would shift the assets in a way that would lead to losses when withdrawing, but this scenario is out of scope.

Additionally, as stated in the EIP4626, the yield loss should be visible and expected on the difference between the previewRedeem() and convertToAssets() calls, which in the current implementation is not (they return the same value). Here is the statement: 'Note that any unfavorable discrepancy between convertToAssets and previewRedeem SHOULD be considered slippage in share price or some other type of condition, meaning the depositor will lose assets by redeeming.'

Add the following test to LMPVault-Withdraw.t.sol as a POC:

function test_POC_previewRedeem_NotERC4626Compliant() 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 1000 asset to DV1
    _underlyerOne.mint(address(this), 500);
    _underlyerOne.approve(address(_lmpVault), 500);
    _lmpVault.rebalance(
        address(_destVaultOne),
        address(_underlyerOne), // tokenIn
        500,
        address(0), // destinationOut, none when sending out baseAsset
        address(_asset), // baseAsset, tokenOut
        1000
    );
    
    // price decrease, users take losses when withdrawing
    _mockRootPrice(address(_underlyerOne), 5e17);

    assertEq(_lmpVault.previewRedeem(500), 500); // does not take into account losses, fishing users

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

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

Impact

Users lose unexpected funds, having called previewRedeem() before redeem() to know how much assets they would get, but then the actual amount could be lower (if there is a loss in one of the vaults).

Code Snippet

redeem() calls _withdraw() internally, while previewRedeem() does not:

function previewRedeem(uint256 shares) public view virtual override returns (uint256) {
    return _convertToAssets(shares, Math.Rounding.Down);
}
...
function redeem(
    uint256 shares,
    address receiver,
    address owner
) public virtual override nonReentrant noNavDecrease ensureNoNavOps returns (uint256 assets) {
    uint256 maxShares = maxRedeem(owner);
    if (shares > maxShares) {
        revert ERC4626ExceededMaxRedeem(owner, shares, maxShares);
    }
    uint256 possibleAssets = previewRedeem(shares);

    assets = _withdraw(possibleAssets, shares, receiver, owner); // TO SUBMIT: no slippage control, might make user lose many tokens
}

Tool used

Vscode
Foundry
Manual Review

Recommendation

Add a _simulateWithdraw() function to simulate withdrawing from the destination vaults and call it internally on previewRedeem(). Note that the destination vaults do not have a function to simulate withdrawBaseAsset(), so this should also be added.

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.