spyrosonic10 - Possible loss of fund in YearnProvider

[sherlock-admin]

spyrosonic10

high

Possible loss of fund in YearnProvider

Summary

There may be scenarios in Yearn where they don't have enough collateral to satisfy withdraw request and this will result in withdrawing less amount than requested amount i.e. partial withdraw.

Vulnerability Detail

Yearn deploy its capital/collateral to other protocols and there may be scenarios when Yearn is not able to withdraw requested amount from other protocols collectively. In this scenario, Yearn will calculate sharesToBurn from partial amount it has withdrawn and burn partial shares. Given partial withdraw is possible in withdraw() of YearnProvider, current logic is not dealing with remaining LP token in YearnProvider.sol

Impact

Unburnt/remaining shares (LP tokens) will remain in YearnProvider.sol contract and this amount will not be part of balanceUnderlying() hence will not be considered as amount under management. So it is clear to say that this scenario will results in loss of fund.

Code Snippet

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/YearnProvider.sol#L44-L66

function withdraw(
    uint256 _amount,
    address _yToken,
    address _uToken
  ) external override returns (uint256) {
    uint256 balanceBefore = IERC20(_uToken).balanceOf(msg.sender);

    require(
      IYearn(_yToken).transferFrom(msg.sender, address(this), _amount) == true,
      "Error transferFrom"
    );

    uint256 uAmountReceived = IYearn(_yToken).withdraw(_amount);
    IERC20(_uToken).safeTransfer(msg.sender, uAmountReceived);

    uint256 balanceAfter = IERC20(_uToken).balanceOf(msg.sender);
    require(
      (balanceAfter - balanceBefore - uAmountReceived) == 0,
      "Error Withdraw: under/overflow"
    );

    return uAmountReceived;
  }

Tool used

Manual Review

Recommendation

Transfer remaining _yTokens to msg.sender after withdraw is done from Yearn

    uint256 remainingLpBalance = IYearn(_yToken).balanceOf(address(this));
    if(remainingLpBalance != 0){
      IYearn(_yToken).transfer(msg.sender, remainingLpBalance)
    }

Duplicate of #355

[spyrosonic10]

Escalate for 10 USDC

This should be High. There can be huge fund loss for protocol when withdraw is partial. Provider's withdraw is routinely called by Vault managing the aggregated funds distribution, the freeze amount can be massive enough and will be translated to a loss for many users.

[sherlock-admin]

Escalate for 10 USDC

This should be High. There can be huge fund loss for protocol when withdraw is partial. Provider's withdraw is routinely called by Vault managing the aggregated funds distribution, the freeze amount can be massive enough and will be translated to a loss for many users.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Although there is a precondition that there should be partial withdrawals, considering that the code does not consider this scenario and the significant impact of the loss. to the user, this issue can be considered a valid high in this case.

[sherlock-admin]

Escalation accepted

Although there is a precondition that there should be partial withdrawals, considering that the code does not consider this scenario and the significant impact of the loss. to the user, this issue can be considered a valid high in this case.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.