hyh - Any tokens can be stolen via withdraw from YearnProvider and AaveProvider balances

[sherlock-admin]

hyh

high

Any tokens can be stolen via withdraw from YearnProvider and AaveProvider balances

Summary

In both cases the amount to be returned to a user is determined by responses of another user-supplied contract.

Vulnerability Detail

Attacker calling YearnProvider and AaveProvider withdraw() can supply real _uToken and precooked _yToken / _aToken that will report the numbers needed to fully extract _uToken balance of the Provider. _uToken can be any, there looks to be no preconditions.

Simplest example is rewards supplied by yield market via any LP initiative program can be stolen by a third party. Say, for example, such reward funds can be sent to some Providers on Vault withdrawals via them and are left on their balances.

Impact

Any underlying tokens can be stolen fully from YearnProvider and AaveProvider balances.

Even if all these contracts aren't supposed to hold balances, there are a spectre of cases when they end up possessing some meaningful funds (accumulated residuals, additional rewards supplied from the markets, user operational mistakes), which are attributed to protocol users, but can be stolen this way.

Code Snippet

Fake _yToken supplied to YearnProvider's withdraw() reports back current real _uToken balance of YearnProvider via IYearn(_yToken).withdraw(_amount), which is then send to msg.sender:

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;
  }

Fake _aToken supplied to AaveProvider's withdraw() reports real pool and _uToken addresses, reports balances so that _amount is the difference. As result current real_aToken balance of AaveProvider will be withdrawn to msg.sender:

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Providers/AaveProvider.sol#L49-L74

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

    require(
      IAToken(_aToken).transferFrom(msg.sender, address(this), _amount) == true,
      "Error: transferFrom"
    );
    uint256 uTokensReceived = IALendingPool(IAToken(_aToken).POOL()).withdraw(
      IAToken(_aToken).UNDERLYING_ASSET_ADDRESS(),
      _amount,
      msg.sender
    );

    uint256 balanceAfter = IERC20(_uToken).balanceOf(msg.sender);

    require(
      (balanceAfter - balanceBefore - uTokensReceived) == 0,
      "Error Withdraw: under/overflow"
    );

    return uTokensReceived;
  }

Tool used

Manual Review

Recommendation

One way is maintaining a whitelist mapping {underlying token -> yield bearing token -> acceptance flag}. The flag for the pair used in a call is then required for withdraw to proceed.

Also, a balance check for token that is sent to a user can be useful: for attacker to benefit the token that is sent to them has to be real, so another approach is controlling its balance of the contract before and after the operation, and require that no loss of the initial balance took place.

Duplicate of #411

[dmitriia]

Escalate for 10 USDC Similarly to 411, https://github.com/sherlock-audit/2023-01-derby-judging/issues/411#issuecomment-1503876217, yield might include reward tokens, that will be sent to YearnProvider or AaveProvider by the pool on withdrawal and then be stolen from Provider balance as described in the issue.

For example, Aave currently have pools with reward tokens being part of the yield in addition to underlying:

https://app.aave.com/reserve-overview/?underlyingAsset=0x3a58a54c066fdc0f2d55fc9c89f0415c92ebf3c4&marketName=proto_polygon_v3

This looks like valid high severity and needs to be fixed.

[sherlock-admin]

Escalate for 10 USDC Similarly to 411, https://github.com/sherlock-audit/2023-01-derby-judging/issues/411#issuecomment-1503876217, yield might include reward tokens, that will be sent to YearnProvider or AaveProvider by the pool on withdrawal and then be stolen from Provider balance as described in the issue.

For example, Aave currently have pools with reward tokens being part of the yield in addition to underlying:

https://app.aave.com/reserve-overview/?underlyingAsset=0x3a58a54c066fdc0f2d55fc9c89f0415c92ebf3c4&marketName=proto_polygon_v3

This looks like valid high severity and needs to be fixed.

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

Considering this issue a duplicate of #411

[sherlock-admin]

Escalation accepted

Considering this issue a duplicate of #411

This issue's escalations have been accepted!

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