search

code-423n4 / 2024-03-ondo-finance-findings

5 stars 6 forks source link

MINIMUM_OUSG_PRICE constant used as safety circuit breaker could lead to unwanted DoS of OUSGInstantManager #78

Closed c4-bot-3 closed 5 months ago

c4-bot-3 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L479-L485 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L63

Vulnerability details

Impact

The MINIMUM_OUSG_PRICE constant is defined in ousgInstantManager.sol#L63 with value 105e18. This constant is used in getOUSGPrice() function:

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    }

So, the constant is used to protect against Oracle malfunction. However, the value of OUSG could genuinely decrease under 105e18 thanks to market fluctuations. In this case, several functionalities of OUSGInstantManager will be unavailable:

Proof of Concept

In ousgInstantManager.sol#L63 is defined the MINIMUM_OUSG_PRICE constant:

63    uint256 public constant MINIMUM_OUSG_PRICE = 105e18;

This constant is used just inside ousgInstantManager.getOUSGPrice() function:

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    }

This function returns the price of OUSG using 18 decimals calling the oracle.getPriceData() function.

As described inside the Discord channel, this is the oracle contract: RWAOracleExternalComparisonCheck. So, this is the oracle.getPriceData() function:

L115-L128

/**
* @notice Retrieve the last set RWA price data
*
* @dev `price` is in 18 decimals, `timestamp` is unix seconds since epoch
*/
function getPriceData()
external
view
override
returns (uint256 price, uint256 timestamp)
{
price = uint256(rwaPrice);
timestamp = priceTimestamp;
}

So, the returned price is the value of the rwaPrice variable. It is set initially inside the constructor (L107):

L76-L113

/**
* @notice constructor
*
* @param _initialPrice     The initial RWA price
* @param _chainlinkOracle Chainlink oracle to compare differences with
* @param _description     Human readable description
* @param _admin           admin which holds the DEFAULT_ADMIN_ROLE
* @param _setter          setter address which holds the role to set rwa price
*/
constructor(
int256 _initialPrice,
address _chainlinkOracle,
string memory _description,
address _admin,
address _setter
) {
if (_admin == address(0) || _setter == address(0)) {
    revert InvalidAddress();
}
chainlinkOracle = AggregatorV3Interface(_chainlinkOracle);
// Revert if Chainlink oracle is not reporting 8 decimals
if (chainlinkOracle.decimals() != 8) {
    revert CorruptedChainlinkResponse();
}

ChainlinkRoundData memory round = _getLatestChainlinkRoundData();
if (block.timestamp > round.updatedAt + MAX_CL_WINDOW) {
    revert ChainlinkOraclePriceStale();
}

description = _description;
rwaPrice = _initialPrice;
priceTimestamp = block.timestamp;
lastSetRound = round;

_grantRole(DEFAULT_ADMIN_ROLE, _admin);
_grantRole(SETTER_ROLE, _setter);
}

and then it can be modified just using the setPrice() function (L214) 

L147-217

function setPrice(int256 newPrice) external override onlyRole(SETTER_ROLE) {
    // RWA price must be greater than zero
    if (newPrice <= 0) {
      revert InvalidRWAPrice();
    }

    ChainlinkRoundData memory round = _getLatestChainlinkRoundData();
    // Chainlink price update must be recent
    if (block.timestamp > round.updatedAt + MAX_CL_WINDOW) {
      revert ChainlinkOraclePriceStale();
    }

    // Chainlink price update must not be comparing the same rounds against
    // eachother
    if (round.roundId == lastSetRound.roundId) {
      revert ChainlinkRoundNotUpdated();
    }

    // Ensure at least `MIN_PRICE_UPDATE_WINDOW` seconds have passed since
    // last RWA price update
    if (block.timestamp < priceTimestamp + MIN_PRICE_UPDATE_WINDOW) {
      revert PriceUpdateWindowViolation();
    }

    int256 rwaPriceChangeBps = _getPriceChangeBps(rwaPrice, newPrice);
    // Never allow a price change that violates the max absolute change
    // threshold.
    if (_abs_unsigned(rwaPriceChangeBps) > MAX_ABSOLUTE_DIFF_BPS) {
      revert AbsoluteDifferenceConstraintViolated();
    }

    int256 chainlinkPriceChangeBps = _getPriceChangeBps(
      lastSetRound.answer,
      round.answer
    );

    if (
      _abs_unsigned(chainlinkPriceChangeBps) <=
      MAX_ABSOLUTE_DIFF_BPS + MAX_CHANGE_DIFF_BPS
    ) {
      // Chainlink price change is sane, so we compare rwa price changes
      // against the Chainlink price changes.
      uint256 changeDifferenceBps = _abs_unsigned(
        rwaPriceChangeBps - chainlinkPriceChangeBps
      );

      if (changeDifferenceBps > MAX_CHANGE_DIFF_BPS) {
        revert DeltaDifferenceConstraintViolation();
      }
    } else {
      emit ChainlinkPriceIgnored(
        lastSetRound.answer,
        lastSetRound.roundId,
        round.answer,
        round.roundId
      );
    }

    emit RWAExternalComparisonCheckPriceSet(
      lastSetRound.answer,
      lastSetRound.roundId,
      round.answer,
      round.roundId,
      rwaPrice,
      newPrice
    );

    rwaPrice = newPrice;
    priceTimestamp = block.timestamp;
    lastSetRound = round;
  }

As we can see, inside the oracle there is no check about the OUSG's price. There are many conditions, described into setPrice() comments (L130-146):

/**
* @notice Sets the price of RWA if all the following criteria are met:
*  - It is able to pull a consistent and recent Chainlink price that is
*    different than the last used Chainlink round
*  - The price wasn't updated too recently (`MIN_PRICE_UPDATE_WINDOW`
*    seconds)
*  - The change in RWA price is < MAX_ABSOLUTE_DIFF_BPS
*  - The change in RWA price has not deviated `MAX_CHANGE_DIFF_BPS` more
*    than the change in the Chainlink price.
* If the change in Chainlink price is larger than `MAX_ABSOLUTE_DIFF_BPS +
* MAX_CHANGE_DIFF_BPS` it is deemed malfunctioning and ignored.
*
* @param newPrice The new price of some RWA token (In `decimals` decimals)
*
* @dev The decimal representation is not enforced yet must be respected by
*      the caller of this function and deployer of this contract
*/

but none of these conditions state that the price of OUSG will not decrease under 105e18. This means that the oracle could work well, and returns the right OUSG price, but ousgInstantManager.getOUSGPrice() still reverts.

We don't think this is the wanted behavior. The intended goal of MINIMUM_OUSG_PRICE constant should be to protect against oracle malfunction. To date, the price of OUSG is about 105.56$, according to coingecko. It followed a constant increase in its value since it was created. However, it has suffered losses of value in the past. For example:

In the case above, the ousgInstantManager contract would suffer a block of its functionalities, and this condition would have lasted for at least 24 hours because the oracle updates the price once a day. In this case, the user can't use the following functionalities, as we said above:

OUSGInstantManager._mintRebasingOUSG() and OUSGInstantManager._mint()

OUSGInstantManager._mintRebasingOUSG() is used to mint rOUSG for a given amount of USDC. It calls OUSGInstantManager._mint() at line ousgInstantManager.sol#L263. OUSGInstantManager._mint() calls ousgInstantManager.getOUSGPrice() at line ousgInstantManager.sol#L307. According to the above situation, it reverts. The user's transaction used to mint rOUSG at 104.87$ fails.

OUSGInstantManager.redeemRebasingOUSG() and OUSGInstantManager._redeem()

OUSGInstantManager.redeemRebasingOUSG() is used to redeem USDC for a given amount of rOUSG (even if the comment of the function is wrong). It calls OUSGInstantManager._redeem() at line ousgInstantManager.sol#L379. OUSGInstantManager._redeem() calls ousgInstantManager.getOUSGPrice() at line ousgInstantManager.sol#L399. According to the above situation, it reverts. The user's transaction used to redeem rOUSG at 104.87$ fails. In this case, if there is a drop in OUSG value, users can't redeem their rOUSG. This could cause a further drop in the value, and so on.

Further considerations

We want to underline that Ondo's team accepts the possibility that the price of OUSG decrease. As described in the contest's C4 webpage, in the Automated Findings / Publicly Known Issues section:

Price Decreases - We are aware that an extreme change in the price of SHV could
prevent Ondo Finance from accurately reporting the OUSG price in its oracle.
We are also aware that the code does not prevent a “negative rebase” in the
event that the OUSG price goes down.

However, the problem described above isn't due to an extreme change in the price and doesn't describe a problem due to a “negative rebase”.

Moreover, in rOUSG.sol#L378-L380 there is a local version of getOUSGPrice(), but without any constant or validation check on the price value. We reported another issue about it and the need to protect against oracle malfunction using flexible behavior.

Tools Used

Visual inspection

Recommended Mitigation Steps

The goal to protect against oracle malfunction is genuine. However, there should be the possibility to change the MINIMUM_OUSG_PRICE value:

49  contract OUSGInstantManager is
50    ReentrancyGuard,
51    InstantMintTimeBasedRateLimiter,
52    AccessControlEnumerable,
53    IOUSGInstantManager,
54    IMulticall
55  {
56    // Role to configure the contract
57    bytes32 public constant CONFIGURER_ROLE = keccak256("CONFIGURER_ROLE");
58  
59    // Role to pause minting and redemptions
60    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
61  
62    // Safety circuit breaker in case of Oracle malfunction
63    uint256 public constant MINIMUM_OUSG_PRICE = 105e18;
+     uint256 public minimum_ousg_price = 105e18;
64
[...]
144    constructor(
145      address defaultAdmin,
146      address _usdc,
147      address _usdcReciever,
148      address _feeReceiver,
149      address _ousgOracle,
150      address _ousg,
151      address _rousg,
152      address _buidl,
153      address _buidlRedeemer,
154      RateLimiterConfig memory rateLimiterConfig
155    )
156      InstantMintTimeBasedRateLimiter(
157        rateLimiterConfig.mintLimitDuration,
158        rateLimiterConfig.redeemLimitDuration,
159        rateLimiterConfig.mintLimit,
160        rateLimiterConfig.redeemLimit
161      )
162    {
[...]
214    }
215  
+      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;
+      }
+
216    /*//////////////////////////////////////////////////////////////
217                              Mint/Redeem
218    //////////////////////////////////////////////////////////////*/
[...]
479    function getOUSGPrice() public view returns (uint256 price) {
480      (price, ) = oracle.getPriceData();
481      require(
-482       price > MINIMUM_OUSG_PRICE,
+          price > minimum_ousg_price,
483        "OUSGInstantManager::getOUSGPrice: Price unexpectedly low"
484      );
485    }

This proposal will guarantee to prevent DoS in case of a drop in OUSG's value. Furthermore, it permits to guarantee that ADMIN_ROLE cannot set a too stringent condition.

Assessed type

Invalid Validation

c4-pre-sort commented 5 months ago

0xRobocop marked the issue as duplicate of #245

c4-judge commented 5 months ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 5 months ago

3docSec marked the issue as grade-b

niser93 commented 5 months ago

Hi @3docSec. Thank you for your judging! I saw your comment here:

The Oracle price is set by the team and does not follow normal market fluctuations - this significantly mitigates risk. However, the recommended mitigation of implementing a setter makes sense, so this group makes for a good QA finding.

The Oracle price is indeed set by the team. However, in the section Background on OUSG of contest page documentation, we can read:

OUSG is backed by an off chain portfolio comprised of cash equivalents. The price of OUSG tracks 
this portfolio's performance minus management fees. OUSG is supported in collateral-only mode 
on the Flux Finance protocol.

So, the price of OUSG depends on the portfolio's performance. According to this, the Ondo team shouldn't set a price that differs from the portfolio's performance, even if this performance leads to a price decrease. In 245 sponsor wrote:

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.

However, in this way, they could move against the market, and the market always wins. If the portfolio's performance goes bad, they have to choose if keeping to set a higher OUSG price unbounded from that performance, or DoS OusgInstantManager functionalities. We think this would have a reputation impact in any case. For these reasons, the function of the protocol or its availability could be impacted and this finding should have medium severity.

Thanks in advance

3docSec commented 5 months ago

Hi @niser93 I see what you mean.

If the portfolio's performance goes bad, they have to choose if keeping to set a higher OUSG price unbounded from that performance, or DoS OusgInstantManager functionalities.

This is a fairly accurate description, but entirely known, and more than that it's how the protocol is designed to be, that is rather centralized. It can therefore be best categorized as a systemic or centralization risk rather than a proper HM finding.

niser93 commented 5 months ago

Thank you for your answer! I'm answering above this issue, but similar considerations are valid for #166 and #251.

1) According to documentation, oracle's price is bound to the portfolio's performance. This documentation should be the only source of truth. Even if the oracle works well, the problem still could happen: it is not a centralization risk 2) According to what the sponsor said, "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.". However, it goes against documentation. If the price goes down, this behavior seems like an indirect way to pause contract functionalities. 3) Yes, it seems a systemic issue. There is a wrong assumption: they assumed the portfolio's performance couldn't go bad. But why a systemic issue should not be a valid HM? Many issues in the past were found on the wrong market assumptions of developers. I think this is not different.

Furthermore, the severity categorization rules are based on the consequences of a vulnerability. As we said above the function of the protocol or its availability could be impacted, and this happens when the oracle is working according to the documentation.

3docSec commented 5 months ago

Roger that. This doesn't change that the oracle is set by the Ondo team, as stated in the README. This means that at each and every point in time, the protocol will behave like the team wants it to. That's not a vulnerability, it's a centralization issue. The decision on this escalation is final.