CurveVolatileCollateral._underlyingRefPerTok() Possible manipulation

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/curve/CurveStableCollateral.sol#L146

Vulnerability details

Impact

curvePool.get_virtual_price() May be manipulated to cause malicious entry DISABLED

Proof of Concept

CurveVolatileCollateral._underlyingRefPerTok() return curvePool.get_virtual_price()

    function _underlyingRefPerTok() internal view virtual override returns (uint192) {
@>      return _safeWrap(curvePool.get_virtual_price());
    }

If curvePool contains ETH, it can be manipulated by the price.
For more information:

https://chainsecurity.com/curve-lp-oracle-manipulation-post-mortem/

_underlyingRefPerTok() Being manipulated could lead to entry DISABLED

Tools Used

Recommended Mitigation Steps

Check for reentrancy as described in the article.

Since reentrancy locks were not publicly viewable by smart contracts, the reentrancy lock had to be triggered differently— by calling a state-altering function. The criteria for such a function were not to transfer funds from or to oracles and to be relatively cheap in terms of gas overhead. The solution identified most suitable was calling the pools’ withdraw_admin_fees function to trigger the reentrancy lock. Most protocols followed that protection pattern.


## Assessed type

Context

[c4-judge]

thereksfour marked the issue as primary issue

[tbrent]

We have considered this very issue before and have decided as a solution to avoid raw ETH and ERC777's entirely, and not try to detect reentrancy in the way described in the article. This is for a few reasons:

1. It cannot be implemented uniformly. withdraw_admin_fees is not on all Curve pools, and in particular not on Tricrypto. https://etherscan.io/address/0xd51a44d3fae010294c616388b506acda1bfaae46
2. Raw ETH presents other challenges. Even if we detect reentrancy in the way suggested, the assetRegistry making multiple asset refresh() calls means an attacker could gain execution under a different asset's refresh() and use that to manipulate refPerTok().

Also related: Our target unit system does not work well with volatile pools. Each pool would need its own target unit and therefore could not be backed up with any other collateral except identically/distributed pools. We plan to remove CurveVolatileCollateral entirely.

[tbrent]

There is documentation on the website indicating to avoid ERC777 tokens, but this should probably be updated to include forbidding LP tokens that contain raw ETH. Though, it is a bit more complicated than that since some LP tokens offer withdrawal functions that automate the unwrapping of WETH into ETH.

https://reserve.org/protocol/rtokens/#non-compatible-erc20-assets

[c4-sponsor]

pmckelvy1 marked the issue as sponsor confirmed

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report

[5z1punch]

Dup of https://github.com/code-423n4/2023-07-reserve-findings/blob/main/data/ronnyx2017-Q.md#7-curve-read-only-reentrancy-can-increase-the-price-of-some-curvestablecollateral . In fact The price manipulated mentioned in the #14 is Curve Read-Only Reentrancy attack. And I explained this attack in more detail in my QA report.

[c4-judge]

thereksfour marked the issue as not selected for report

[c4-judge]

thereksfour marked the issue as duplicate of #45