search

code-423n4 / 2023-07-reserve-findings

0 stars 0 forks source link

`AppreciatingFiatCollateral::refresh` can revert due to implementations of `_underlyingRefPerTok` #3

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/AppreciatingFiatCollateral.sol#L80 https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/AppreciatingFiatCollateral.sol#L90

Vulnerability details

Description

According to the documentation and comments, the function refresh should never revert. However, AppreciatingFiatCollateral can revert due to the implementation of the _underlyingRefPerTok function. For instance, if an external call made in this function returns an error, then refresh will revert.

Impact

The implementation of function refresh in AppreciatingFiatCollateral can revert due to the implementation of the function _underlyingRefPerTok, contradicting the documentation, which states: Because refresh() is relied upon by so much of the protocol, it is important that it only reverts due to out-of-gas errors.

POC

wstETH.stEthPerToken() just do a call to stETH. This contract is upgradable, meaning that in a future any of his methods can change. If stETH.getPooledEthByShares(uint256) is switch for a function which introduce an error, then, wstETH.stEthPerToken() will revert, which means that LidoStakedEthCollateral::_underlyingRefPerTok will revert too, reverting AppreciatingFiatCollateral::refresh

Severity justification

Given external conditions, stated assumption, low likelihood and that this issue can break function refresh expected behaviour, which is critical for the correct behaviour of the whole protocol, the severity assigned is MEDIUM

Recommended mitigation

Option A

Clarify in documentation the implementation risks of AppreciatingFiatCollateral::_underlyingRefPerTok, as well as discouraging any external call to an upgradable contract inside this function.

Option B

Introduce error handling in this block to handle any error which could come from an external call of _underlyingRefPerTok.

Assessed type

Other

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #13

c4-judge commented 1 year ago

thereksfour marked the issue as satisfactory

c4-judge commented 1 year ago

thereksfour changed the severity to QA (Quality Assurance)