psy4n0n - YearnProtocol’s pricePerShare can return wrong exchangeRate by the use of flash-loans

[sherlock-admin]

psy4n0n

high

YearnProtocol’s pricePerShare can return wrong exchangeRate by the use of flash-loans

Summary

YearnProtocol’s pricePerShare can be manipulated by directly depositing into the underlying pool which can significantly manipulate the price. This has been previously exploited using flash loan to exploit CREAM Finance (https://github.com/yearn/yearn-security/blob/master/disclosures/2021-10-27.md)

Vulnerability Detail

The value of pricePerShare can be significantly increased, the value is calculated using totalAssets / totalSupply . Here the totalAssets can be increased by directly depositing into the vault., hence the value of pricePerShare can be increased.

This will directly affect the functions exchangeRate (for the provider) and price (in Vault.sol), which will now return wrong price.

This can then affect the rewardPerLockedToken mapping due to the value of nominator being significantly larger.

function storePriceAndRewards(uint256 _totalUnderlying, uint256 _protocolId) internal {
    uint256 currentPrice = price(_protocolId); // calculated from pricePerShare
    if (lastPrices[_protocolId] == 0) {
      lastPrices[_protocolId] = currentPrice;
      return;
    }

    int256 priceDiff = int256(currentPrice - lastPrices[_protocolId]);
    int256 nominator = (int256(_totalUnderlying * performanceFee) * priceDiff);
    int256 totalAllocatedTokensRounded = totalAllocatedTokens / 1E18;
    int256 denominator = totalAllocatedTokensRounded * int256(lastPrices[_protocolId]) * 100; // * 100 cause perfFee is in percentages

    if (totalAllocatedTokensRounded == 0) {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = 0;
    } else {
      rewardPerLockedToken[rebalancingPeriod][_protocolId] = nominator / denominator;
    }

    lastPrices[_protocolId] = currentPrice;
  }

Impact

This might lead to fund loss depending on the liquidity present in the vault as the price will return different value than it should.

Code Snippet

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

Tool used

Manual Review

Recommendation

Here are a few comments/recommendation that can be taken in consideration:

1. Strictly check the price source for all tokens - you should be confident that no element in the formula can be manipulated. 
2. Use the Chainlink oracle. 
3. Use TWAP when the Chainlink price is not available (but it is bad advice in some cases, see the following chapter).

[mister0y]

The price of the vault or rewards are not directly impacted by the price of a yearn vault.

[fs0c-sh]

Escalate for 10 USDC A few things I wanted to mention about this :

1. In the above function the value of currentPrice would be changed in case of price manipulation in yearn vault, this would in fact change the priceDiff and thus the nominator . The value of denominator will remain the same as of the lastPrices when the price was not manipulated. This will in fact change the value of rewardPerLockedToken .
2. The value of exchangeRate from yearn provider also changes the returned value of balanceUnderlying which might affect the accounting of the protocol in case of attack as balanceUnderlying in Vaults directly call balanceUnderlying of the providers(in this case yearn provider), because this directly affects savedTotalUnderlying value in the vault.
3. The above attacks, might change the accounting of the protocol and the rewards distributed, I haven't written an exploit/POC to prove the same but reading the code, it seems like the accounting would definitely be disturbed in case of such flash-loan exploit.

[sherlock-admin]

Escalate for 10 USDC A few things I wanted to mention about this :

1. In the above function the value of currentPrice would be changed in case of price manipulation in yearn vault, this would in fact change the priceDiff and thus the nominator . The value of denominator will remain the same as of the lastPrices when the price was not manipulated. This will in fact change the value of rewardPerLockedToken .
2. The value of exchangeRate from yearn provider also changes the returned value of balanceUnderlying which might affect the accounting of the protocol in case of attack as balanceUnderlying in Vaults directly call balanceUnderlying of the providers(in this case yearn provider), because this directly affects savedTotalUnderlying value in the vault.
3. The above attacks, might change the accounting of the protocol and the rewards distributed, I haven't written an exploit/POC to prove the same but reading the code, it seems like the accounting would definitely be disturbed in case of such flash-loan exploit.

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 rejected

This is not a valid issue. Comment from lead Watson:

the manipulation of yearn vault is done via donation and is feasible only if there is leverage > 1 wrt it achieved via some other protocol. Here it is not the case.

[sherlock-admin]

Escalation rejected

This is not a valid issue. Comment from lead Watson:

the manipulation of yearn vault is done via donation and is feasible only if there is leverage > 1 wrt it achieved via some other protocol. Here it is not the case.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.