The getOUSGPrice() function in rOUSG.sol has no protection against oracle malfunction

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L378-L380

Vulnerability details

Impact

In rOUSG.sol#L378-L380 is defined the getOUSGPrice() function for rOUSG contract. This function is quite similar to the ousgInstantManager one. However, while the latter has protection against oracle malfunction (that can be too stringent, as we reported in another issue), the former does not. This means that when the ousgInstantManager functionalities are blocked because the value returned by the oracle is too low, the functionalities of rOUSG keep working. Moreover, without this protection, in case of oracle malfunction many functionalities of the rOUSG contract would work badly. In particular, the getOUSGPrice() is used into:

• rOUSG.sol#L190 - rOUSG.totalSupply()
• rOUSG.sol#L201 - rOUSG.balanceOf()
• rOUSG.sol#L367 - rOUSG.getSharesByROUSG()
• rOUSG.sol#L375 - rOUSG.getROUSGByShares()

Those functions can heavily impact the behavior of the rOUSG contract.

Proof of concept

In rOUSG.sol#L378-L380 is defined the getOUSGPrice() function for rOUSG contract:

378     function getOUSGPrice() public view returns (uint256 price) {
379         (price, ) = oracle.getPriceData();
380     }

while in ousgInstantManager.sol#L479-L485 is defined the getOUSGPrice() function for ousgInstantManager contract:

479    function getOUSGPrice() public view returns (uint256 price) {
480      (price, ) = oracle.getPriceData();
481      require(
482        price > MINIMUM_OUSG_PRICE,
483        "OUSGInstantManager::getOUSGPrice: Price unexpectedly low"
484      );
485    }

where the MINIMUM_OUSG_PRICE constant:

63    uint256 public constant MINIMUM_OUSG_PRICE = 105e18;

Consistently to the ousgInstantManager contract, the rOUSG contract should revert when the oracle returns a price that is too low. Moreover, as we proposed in another issue, this protection should be flexible, as we propose in our mitigation proposal.

Tools Used

Visual inspection

Recommended Mitigation Steps

contract ROUSG is
  Initializable,
  ContextUpgradeable,
  PausableUpgradeable,
  AccessControlEnumerableUpgradeable,
  KYCRegistryClientUpgradeable,
  IERC20Upgradeable,
  IERC20MetadataUpgradeable
{
[...]
+     uint256 public minimum_ousg_price = 105e18;
[...]
+      function set_new_minimum_oug_price(uint256 new_minimum_oug_price) public onlyRole(DEFAULT_ADMIN_ROLE){
+           require(new_minimum_oug_price < MINIMUM_OUSG_PRICE);
+           minimum_ousg_price = new_minimum_oug_price;
+      }
+
378     function getOUSGPrice() public view returns (uint256 price) {
379         (price, ) = oracle.getPriceData();
+           require(
+               price > minimum_ousg_price,
+               "OUSGInstantManager::getOUSGPrice: Price unexpectedly low"
+           );
380     }

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #41

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #144

[c4-judge]

3docSec marked the issue as unsatisfactory: Out of scope

[c4-judge]

3docSec marked the issue as unsatisfactory: Out of scope

[niser93]

Hi @3docSec! I left a similar comment in #251. Also, this finding was judged Out of scope because of this:

We are aware that the SHV price could differ from the OUSG portfolio, so any findings
related to this price discrepancy is out of scope.

I want to underline that #251 and this finding are quite different. The former is on the usage of old OUSG prices, this one is on the lack of protection against too low price. So, while the former is invariant for the protocol, the latter depends on the developers' purpose.

Developers inserted the same protection against oracle malfunction I proposed in ousgInstantManager.sol#L479-L485. This protection is not used against the fact that that the SHV price could differ from the OUSG portfolio. Developers state in #245 that:

There would not be a situation in which we would want to allow instant mints or 
redemptoins at a price lower than MINIMUM_OUSG_PRICE.

So, even if we don't agree with this statement, and suggest using a flexible value of MINIMUM_OUSG_PRICE, it is clear that they need the same protection check also in rOUSG, to protect against oracle malfunction.

Thanks in advance!

[3docSec]

The oracle price is set by the team, so anything related to oracle malfunction, like any other misconfiguration of the admin-controlled contracts, and including setting a price lower than MINIMUM_OUSG_PRICE, falls in the centralization risks category and is therefore not a valid HM.