code423n4 commented 1 year ago

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 commented 1 year ago

thereksfour marked the issue as primary issue

tbrent commented 1 year ago

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:

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
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 commented 1 year ago

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