obront - LP tokens are not sent back to withdrawing user

[github-actions]

obront

high

LP tokens are not sent back to withdrawing user

Summary

When users withdraw their assets from IchiVaultSpell.sol, the function unwinds their position and sends them back their assets, but it never sends them back the amount they requested to withdraw, leaving the tokens stuck in the Spell contract.

Vulnerability Detail

When a user withdraws from IchiVaultSpell.sol, they either call closePosition() or closePositionFarm(), both of which make an internal call to withdrawInternal().

The following arguments are passed to the function:

• strategyId: an index into the strategies array, which specifies the Ichi vault in question
• collToken: the underlying token, which is withdrawn from Compound
• amountShareWithdraw: the number of underlying tokens to withdraw from Compound
• borrowToken: the token that was borrowed from Compound to create the position, one of the underlying tokens of the vault
• amountRepay: the amount of the borrow token to repay to Compound
• amountLpWithdraw: the amount of the LP token to withdraw, rather than trade back into borrow tokens

In order to accomplish these goals, the contract does the following...

1. Removes the LP tokens from the ERC1155 holding them for collateral.

doTakeCollateral(strategies[strategyId].vault, lpTakeAmt);

2. Calculates the number of LP tokens to withdraw from the vault.

uint256 amtLPToRemove = vault.balanceOf(address(this)) - amountLpWithdraw;
vault.withdraw(amtLPToRemove, address(this));

3. Converts the non-borrowed token that was withdrawn in the borrowed token (not copying the code in, as it's not relevant to this issue).

4. Withdraw the underlying token from Compound.

doWithdraw(collToken, amountShareWithdraw);

5. Pay back the borrowed token to Compound.

doRepay(borrowToken, amountRepay);

6. Validate that this situation does not put us above the maxLTV for our loans.

_validateMaxLTV(strategyId);

7. Sends the remaining borrow token that weren't paid back and withdrawn underlying tokens to the user.

doRefund(borrowToken);
doRefund(collToken);

Crucially, the step of sending the remaining LP tokens to the user is skipped, even though the function specifically does the calculations to ensure that amountLpWithdraw is held back from being taken out of the vault.

Impact

Users who close their positions and choose to keep LP tokens (rather than unwinding the position for the constituent tokens) will have their LP tokens stuck permanently in the IchiVaultSpell contract.

Code Snippet

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/spell/IchiVaultSpell.sol#L276-L330

Tool used

Manual Review

Recommendation

Add an additional line to the withdrawInternal() function to refund all LP tokens as well:

  doRefund(borrowToken);
  doRefund(collToken);
+ doRefund(address(vault));

[Gornutz]

duplicate of 34