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

5 stars 6 forks source link

`OUSGInstantManager` will allow Excessive OUSG Token Minting During USDC Depeg Event #278

Open c4-bot-3 opened 6 months ago

c4-bot-3 commented 6 months ago

Lines of code

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

Vulnerability details

Proof of Concept

Any User can use mint function in ousgInstantManager contract to mint OUSG tokens by providing USDC token.

It calls internal function _mint where the main logic resides.


  function _mint(uint256 usdcAmountIn, address to) internal returns (uint256 ousgAmountOut) {

    // SNIP: Validation

    uint256 usdcfees = _getInstantMintFees(usdcAmountIn);
    uint256 usdcAmountAfterFee = usdcAmountIn - usdcfees;

    // Calculate the mint amount based on mint fees and usdc quantity
    uint256 ousgPrice = getOUSGPrice();
    ousgAmountOut = _getMintAmount(usdcAmountAfterFee, ousgPrice);

    require(ousgAmountOut > 0, "OUSGInstantManager::_mint: net mint amount can't be zero");

    // SNIP: Transfering USDC

    ousg.mint(to, ousgAmountOut);
  }

2 important Points to understand here:

1. OUSG Price Stability:

The contract depends on the OUSG price obtained from an oracle, which is heavily constrained (As per Readme) to ensure stability.

OUSG Price - The OUSG price tracks an off chain portfolio of cash equivalents and treasury bills, price changes are heavily constrained in the OUSG Oracle, which uses the change in the price of SHV to set the allowable OUSG price in between updates. 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. Also, scenarios where the OUSG price increases by many orders of magnitudes are not realistic and consequently not considered valid.

As per RWAOracleRateCheck Oracle, Constraints includes:

  1. OUSG price updates restricted to once every 23 hours.
  2. Price deviations limited to a maximum of 1%.

      function setPrice(int256 newPrice) external onlyRole(SETTER_ROLE) {
        if (newPrice <= 0) {
          revert InvalidPrice();
        }
 @->    if (block.timestamp - priceTimestamp < MIN_PRICE_UPDATE_WINDOW) {
          revert PriceUpdateWindowViolation();
        }
 @->    if (_getPriceChangeBps(rwaPrice, newPrice) > MAX_CHANGE_DIFF_BPS) {
          revert DeltaDifferenceConstraintViolation();
        }

        // Set new price
        int256 oldPrice = rwaPrice;
        rwaPrice = newPrice;
        priceTimestamp = block.timestamp;

        emit RWAPriceSet(oldPrice, newPrice, block.timestamp);
      }

These constraints ensure relative stability of the OUSG price.

2. Calculation Assumptions:

The calculation of the amount of OUSG tokens to mint assumes a fixed conversion rate of 1 USDC = 1 USD.

Key point:


  function _getMintAmount(
    uint256 usdcAmountIn,
    uint256 price
  ) internal view returns (uint256 ousgAmountOut) {
    uint256 amountE36 = _scaleUp(usdcAmountIn) * 1e18;
    ousgAmountOut = amountE36 / price;
  }

Here, There are No validation checks implemented regarding the current USDC price.

Scenario of Issue

Consider Alice's attempt to mint OUSG tokens by providing 100,000 USDC, assuming no minting fees and OUSG price of 105e18 USD. The calculation yields: 100_000e36 / 105e18 which is approximately 95_000e18 or 95_000 OUSG tokens for the 100_000 USDC provided.

However, in the event of a USDC depeg, where USDC's value deviates from 1 USD:

This scenario leads to Alice getting close to 95_000 OUSG tokens again for 100_000 USDC provided But this time 100_000 USDC can be worth as low as 87_000 USD if we take recent depeg event in March 2023, where USDC price went as low as 87 cents (Reference).

This way, contract will allow Users to mint excessive OUSG tokens during Depeg event.

Impact

Minting of Excessive Token in case of USDC Depeg.

Tools Used

VS Code

Recommended Mitigation Steps

Ideally, there needs to be an additional oracle to check current Price of USDC and take it's price into the consideration when calculation OUSG tokens to mint.

Assessed type

Context

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #297

c4-judge commented 6 months ago

3docSec changed the severity to 3 (High Risk)

c4-judge commented 6 months ago

3docSec marked the issue as satisfactory

c4-judge commented 6 months ago

3docSec marked the issue as selected for report

3docSec commented 6 months ago

Upgraded as High because there is risk of value extraction from the protocol under conditions that can be monitored by an attacker.

cameronclifton commented 5 months ago

Ondo has the ability to convert USDC to USD at a 1:1 rate and USD to BUIDL at a 1:1 rate, and this ability can be assumed. Should these abilities be removed this contract will be immediately paused. If someone is able to purchase USDC at a lower price to their benefit that should not negatively affect the protocol.

For the above reasons we do not believe this is a high severity issue.

c4-sponsor commented 5 months ago

cameronclifton marked the issue as disagree with severity

notbozho commented 5 months ago

Hey @3docSec

This is a really really rare event. For proof, let's look at the USDC price chart.

Over the past 1 year, the price of USDC has moved the most by $0.001 (the difference between the highest and lowest value) and the price has been consistently maintained at $1 over the past year.

usdc-one-year

For the all time chart, the USDC low is $0.97 and the high is $1.034 (only $0.064 movement all time!).

usdc-all-time

Source: coinmarketcap

Also, as the sponsor says above:

Should these abilities be removed this contract will be immediately paused. If someone is able to purchase USDC at a lower price to their benefit that should not negatively affect the protocol.

That being said, this issue should be considered as medium severity.

3docSec commented 5 months ago

Noted, I'll think about it.

cameronclifton commented 5 months ago

After further review, we will be mitigating this by adding a Chainlink USDC/USD oracle to the OUSGInsatntManager contract. If the price is lower than what we are comfortable with, all mints and redemptions will be blocked. While we think it is unlikely that we won't be able to convert USDC->USD 1:1 in our backend systems, we decided to do this out of extreme caution.

c4-sponsor commented 5 months ago

cameronclifton (sponsor) confirmed