# NM-0081-Ondo Finance

## List of status for each finding
- **Fixed:** Issue has been fixed in the code;

- **Acknowledged:** Client acknowledges that he/she is aware of the situation, but no action will be taken now (since they depend on future decisions, require more planning, etc.)

- **Mitigated:** The issue is still in the code, but some measures have been taken to mitigate the issue (by changing the user interface, updating user-facing documentation, or any other activities outside of this codebase);

- **Not Fixed:** The issue is not fixed and the client has not engaged in any action to solve it;

- **False-positive:** The client does not agree with the issue.

**Commit Hash:** `6face44a82b7300be928cba30cd970636a77d93a`

### [Medium] Rounding issue when calculating absolute changes in basis points (BPS)

**File(s)**: [`RWAOracleRateCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleRateCheck.sol), [`RWAOracleExternalComparisonCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol)

**Description**: In the RWA oracle, there is a `MAX_CHANGE_DIFF_BPS` parameter that limits the change in the new price. This is done by calculating the difference between the new and old prices and dividing it by the old price. However, rounding down when calculating `changeBps` could allow price change to be higher than `MAX_CHANGE_DIFF_BPS`.

**Example of an exploited scenario**:

1. The admin sets `MAX_CHANGE_DIFF_BPS = 10`.
2. If the old price is `previousPrice = 10011` and the new price is `newPrice = 10000`, the calculation will be:

change = newPrice - previousPrice = 10000 - 10011 = -11
changeBps = (change * BPS_DENOMINATOR) / previousPrice = (-11 * 10000) / 10011 = -10.99

3. Since Solidity performs calculations using integers, `changeBps` will round down to `-10`, making it still valid to set even though the change is more than `10 BPS`.

**Recommendation(s)**: Consider rounding up when calculating the absolute changes in basis points. Here is a [reference implementation](https://github.com/transmissions11/solmate/blob/main/src/utils/FixedPointMathLib.sol#L53) for rounding up.

**Status**: Acknowledged

**Update from the client**: This is a great call out, but exclusively rounding up or down would have a loosening or tightening effect based on the situation. The slightly more correct option here would be to conditionally round up or down depending on which action would result in the tightest possible constraint. Due to how loose this constraint already is, we do not expect to ever be operating at these bounds. This decision should have no tangible outcome for the goals of the contract. We have decided not to implement “conditional” rounding does not worth the complexity.

***Example where rounding down loosens constraint.***
If RWA bps change is 84.9, and the CL bps change is 10, the 84.9 gets truncated to 84. Because 84-10 = 74, the price setting would succeed.

***Example where rounding down tightens constraint***
If RWA bps change is -85 and the CL bps change is -10.9, The -10.9 gets truncated to -10. Because -85 - -10 = -75, the price setting would fail. (If we rounded up it would have “loosened” constraint and passed). Please let us know if you believe we are missing anything here.

**Update from Nethermind**: We agree with your perspective that rounding should be situation-dependent. However, we believe it remains an issue, not a false positive. The provided examples occur because `RWAOracleExternalComparisonCheck` employs two rounds of rounding (one for RWA BPS and one for Chainlink BPS), while in the case of `RWAOracleRateCheck`, the change is rounded only once, and rounding up always tightens the constraints.

**Update from Client**: PR to round up for `RWAOracleRateCheck`: [a998c11559457b4d789cfda75903ee489f827cb9](https://github.com/ondoprotocol/flux/commit/a998c11559457b4d789cfda75903ee489f827cb9).

---

### [Medium] No guarantee that `rwaPrice` and `chainlinkPrice` are close

**File(s)**: [`RWAOracleExternalComparisonCheck.sol`]()

**Description**: In contract `RWAOracleExternalComparisonCheck`, every time a new price is set it will check with the recent Chainlink price to ensure it is not too far away from each other.

However, there is no guarantee that `rwaPrice` and `chainlinkPrice` are close from the beginning or during operation. Consider the example below:
1. Initialization, `rwaPrice = 100000` and `chainlinkPrice = 100000`. They are exactly the same, so there is no problem.
2. During operation, Chainlink oracle went wrong `chainlinkPrice = 2000000`. As we can see, the difference between the new price and the old price of Chainlink is too large, it is ignored but still recorded.
4. Now, the setter calls the function `setPrice(...)` with `newPrice = 100100` (1% change). And now Chainlink oracle return price is `chainlinkPrice = 2002000` (1% change too). Their `changeDifferenceBps = 1% - 1% = 0`, thus still allowing the price to be updated.

**Recommendation(s)**: Consider reviewing the mechanism of RWAOracleExternalComparisonCheck again.

**Status**: False-positive

**Update from the client**: The RWA and Chainlink prices are not the same. Currently, OUSG (the RWA token whose price we plan to set) is $100.8 while the SHV ETF price (from the chainlink feed is ~$110). The key insight behind the design is as that because the RWA price is backed by a fund comprised of the ETF with the CL price feed, they should move in similar directions over **short** time horizons."

As for the specific example provided, when Chainlink reports an unrealistic price (more than 274 bps in change up or down), the code effectively ignores the Chainlink price change and solely relies on the “absolute check” (plus or minus 200 bps for setting a price.
On the subsequent setting of the price, the operator of the contract will need to evaluate whether using this contract and CL is still feasible. The resolution may be that this was a one-time bug from CL and we can continue as normal. It may also be that CL is no longer reliable anymore and a vote should be required to utilize some new pricing mechanism. It is hard to predict what the problem and resolution should be.

What is important here is that the invariant of the contract is not broken and the operators of the contract are not able to set the price in some dramatic fashion and rug pull or falsely liquidate investors in Flux Finance.

**Update from Nethermind**: We agree that RWA price and Chainlink price are different and don't need to be similar in value. Therefore this issue is false positive.

---

### [Medium] `setPrice(...)` function does not guarantee the latest price

**File(s)**: [`RWAOracleExternalComparisonCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol)

**Description**: In the `RWAOracleExternalComparisonCheck` contract, the `setPrice(...)` function can update the price only if there is a difference between the latest `roundId` from Chainlink data and the current `roundId`. This check ensures that the answer is updated. Below we present this check:

///////////////////////////////////////////////////////////
// @audit This check does not guarantee the latest price //
///////////////////////////////////////////////////////////
if (round.roundId == lastSetRound.roundId)
  revert ChainlinkRoundNotUpdated();

There is a scenario where the current `roundId` differs from the previous one, but the answer remains unchanged. This can occur when the `Aggregator` implementation is modified, causing the `phaseId` to change while the answer stays the same. The `roundId` is derived from the `phaseId` and `aggregatorRoundId`, so it changes when either of these values is modified.
Refer for more information here: https://docs.chain.link/data-feeds/historical-data.

**Recommendation(s)**: Verify that `answeredInRound` is equal to `roundId`. This ensures that the answer has been updated and is not being carried over.

**Status**: False-positive

**Update from the client**:
Per recent Chainlink Oracle updates, it is impossible for roundID to be different from answeredInRound. See [OffchainAggregator.sol](https://etherscan.deth.net/address/0x9E1320991057c1246cba9F02c79f272a4Da892b3) L883. To double-check this invariant, we have standardized all CL queries by having them sanitize responses by requiring `roundId==AnsweredInRound`.

**Update from Nethermind**: The provided address `0x9E1320991057c1246cba9F02c79f272a4Da892b3` corresponds to the `AccessControlledOffchainAggregator` contract for the SHV/USD pair. However, it is recommended to utilize the `EACAggregatorProxy` contract located at `0xc04611C43842220fd941515F86d1DDdB15F04e46`. This contract is listed on the official Chainlink page for the SHV/USD pair (https://docs.chain.link/data-feeds/price-feeds/addresses). The proxy contract directs to the current aggregator contract, allowing for upgrades without service interruption to consuming contracts. Consuming data from the proxy contract is advisable to ensure that the response originates from the current working aggregator, which is not guaranteed when consuming data directly from the aggregator. If the Ondo team decides to consume data from the proxy contract, it is recommended to verify that `answeredInRound` equals `roundId`. If the team chooses to consume data directly from the aggregator, this check is not necessary. However, additional logic must be implemented in the `RWAOracleExternalComparisonCheck` contract to allow for a change of the price feed address for the SHV/USD pair, as the aggregator contract used by Chainlink may change.

**Update from Client**: We will be interacting directly with the Proxy and understand that we will have to upgrade and do a vote if CL changes their implementation such that `roundId != answeredInRound`.

**Update from Nethermind**: During an internal discussion and a closer look at the CL codebase, we understood that such a condition could happen in the older Chainlink model, but it is not the case in the current implementation used by the RWA Oracle. After learning that answeredInRound is a deprecated variable, we decided to mark the issue as false-positive since we don’t believe it poses any security concern for Oracle.

---

### [Low] `_setFTokenToChainlinkOracle(...)` does not check oracle decimals

**File(s)**: [`contracts/lending/fluxOracles/FluxOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fluxOracles/FluxOracle.sol)

**Description**: The `_setFTokenToChainlinkOracle(...)` function assigns a specified Chainlink oracle to an `fToken`. However, there is no validation process in place to ensure that the number of decimals returned by the Chainlink oracle falls within an acceptable range. The function is presented below:

function _setFTokenToChainlinkOracle(
  address fToken,
  address chainlinkOracle,
  uint256 maxChainlinkOracleTimeDelay
) private {
  if (fTokenToOracleType[fToken] != OracleType.CHAINLINK) {
    revert InvalidOracleType(
      OracleType.CHAINLINK,
      fTokenToOracleType[fToken]
    );
  }
  address underlying = ICTokenLike(fToken).underlying();
  fTokenToChainlinkOracle[fToken].scaleFactor = (10 **
    (36 -
      uint256(IERC20Like(underlying).decimals()) -
      //////////////////////////////////////////////////////////////
      // @audit The decimals from Oracle are not checked 
      //////////////////////////////////////////////////////////////
      uint256(AggregatorV3Interface(chainlinkOracle).decimals())));
  fTokenToChainlinkOracle[fToken].oracle = AggregatorV3Interface(
    chainlinkOracle
  );
  fTokenToChainlinkOracle[fToken]
    .maxChainlinkOracleTimeDelay = maxChainlinkOracleTimeDelay;
}

**Recommendation(s)**: Verify the number of decimals returned by the Chainlink oracle.

**Status**: Acknowledged

**Update from the client**:
The contract works as intended in cases where `underlyingDecimals + chainlinkDecimals <= 36`. If the sum is > 36, the function will underflow. If the sum is == 36, the `scaleFactor` will be 1. If the sum is < 36, the scaleFactor will be some value >= 1e2. A check is not needed.

**Update from Nethermind**: We believe that a check for decimals should be performed to ensure that the Chainlink response is not corrupted. This check would be consistent with the other checks for Chainlink response decimals in the `fTokenOracle` and `RWAOracleExternalComparisonCheck` contracts.

---

### [Low] Changing the Oracle type does not remove previous Oracle data

**File(s)**: [`contracts/lending/fluxOracles/FluxOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fluxOracles/FluxOracle.sol)

**Description**: The `FluxOracle` contract allows for the oracle type to be changed for each asset. However, when the oracle type is changed, the data previously stored in the `fTokenToHardcodedPrice`, `fTokenToRWAOracle`, and `fTokenToChainlinkOracle` mappings for the given asset is not removed. This can result in unintended behavior. It is good practice to remove unused data to prevent such issues and to save on gas costs. Below we present `setFTokenToOracleType(...)` function responsible for changing the oracle type:

function setFTokenToOracleType(
  address fToken,
  OracleType oracleType
) external override onlyRole(DEFAULT_ADMIN_ROLE) {
  fTokenToOracleType[fToken] = oracleType;
  /////////////////////////////////////////////////////////////////////////////////
  // @audit Data from mappings fTokenToHardcodedPrice, fTokenToRWAOracle, and
  //              fTokenToChainlinkOracle is not removed.                    
  /////////////////////////////////////////////////////////////////////////////////
  emit FTokenToOracleTypeSet(fToken, oracleType);
}

**Recommendation(s)**: Consider removing any unused data when changing the oracle type for a given asset.

**Status**: Fixed

**Update from the client**:
Cleared stale data in this PR:
[c39b3532de80600b201d801f28a699ae93bd07de](https://github.com/ondoprotocol/flux/commit/c39b3532de80600b201d801f28a699ae93bd07de).

---

### [Low] Lack of a two-step process for transferring ownership

**File(s)**: [`contracts/lending/fTokenOracle/fTokenOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fTokenOracle/fTokenOracle.sol)

**Description**: The `fTokenOracle` contract inherits OpenZeppelin’s `Ownable` contract, which enables ownership to be transferred in a single step. However, this one-step transfer process can be prone to errors and may cause significant damage to the protocol if a mistake occurs. The function is presented below:

//////////////////////////////////////////////////////////////////////////////////////////
// @audit Providing an incorrect address will result in the immediate loss of ownership //
//////////////////////////////////////////////////////////////////////////////////////////
function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _transferOwnership(newOwner);
}

**Recommendation(s)**: We recommend implementing a two-step ownership transfer process, such as the propose-accept scheme. You can refer to the [Ownable2Step.sol](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol) contract from OpenZeppelin for an example.

**Status**: Fixed

**Update from the client**:
Added Ownable2Step in this PR: [0b86a1b554a8c345faef8b22045c718e52e2d533](https://github.com/ondoprotocol/flux/commit/0b86a1b554a8c345faef8b22045c718e52e2d533).

---

### [Low] Use AccessControlDefaultAdminRules

**File(s)**: [`contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol), [`contracts/lending/rwaOracles/RWAOracleRateCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleRateCheck.sol), [`contracts/lending/fluxOracles/FluxOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fluxOracles/FluxOracle.sol)

**Description**: The `FluxOracle`, `RWAOracleRateCheck`, and `RWAOracleExternalComparisonCheck` contracts inherit from [`AccessControl`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol). However, the `AccessControl` contract allows for potentially insecure behavior with the `DEFAULT_ADMIN_ROLE`, as multiple addresses can be granted this role and it is its own admin, meaning it has permission to grant and revoke the default admin role. A more secure solution is the [`AccessControlDefaultAdminRules`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControlDefaultAdminRules.sol) contract, which implements additional risk mitigations on top of `AccessControl`:
- only one account holds the `DEFAULT_ADMIN_ROLE` since deployment until it's potentially renounced
- enforces a 2-step process to transfer the `DEFAULT_ADMIN_ROLE` to another account.
- enforces a configurable delay between the two steps, with the ability to cancel before the transfer is accepted.
- the delay can be changed by scheduling
- it is not possible to use another role to manage the `DEFAULT_ADMIN_ROLE`.

**Recommendation(s)**: Consider using `AccessControlDefaultAdminRules` to increase the security of role management.

**Status**: Acknowledged

**Update from the client**: Understand the benefits of `AccessControlDefaultAdminRules`; however, we don't want to believe we need the added features/complexity at this point in time.

---

### [Info] No check for `fToken` validity

**File(s)**: [`contracts/lending/fluxOracles/FluxOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fluxOracles/FluxOracle.sol)

**Description**: In the `FluxOracle` contract, the oracle type can be assigned to an `fToken` through the `fTokenToOracleType` mapping. However, there is no check in place to ensure that the `fToken` is a valid address. This could be implemented in a similar manner to the `fTokenOracle` contract:

if (!IFToken(_fToken).isCToken()) {
  revert InvalidFToken();
}

**Recommendation(s)**: Consider checking the validity of the `fToken` when assigning the oracle type to the asset.

**Status**: Fixed

**Update from the client**:
Check added in this PR: [f19e5a38b78153ea436632f480b73194cdcda930](https://github.com/ondoprotocol/flux/commit/f19e5a38b78153ea436632f480b73194cdcda930).

---

### [Info] Inconsistent behavior on `price == 0`

**File(s)**: [`contracts/lending/fluxOracles/FluxOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fluxOracles/FluxOracle.sol)

**Description**: The `FluxOracle` contract supports three types of oracles: hardcoded, RWA, and Chainlink. However, these oracle types do not exhibit consistent behavior when the price is equal to `0`. Here is a summary of the behavior for each oracle type:
- Hardcoded: The price can be set to and retrieved as `0`.
- RWA: The oracle cannot be set if returned price is `0`. The price can be retrieved as `0`.
- Chainlink: The price can be retrieved as `0` (but reverts if less than `0`).

This inconsistent behavior with zero values may lead to incorrect assumptions when using the oracles. While checking for a zero value when setting the RWA oracle is good practice, it creates an assumption that this oracle will always work correctly and does not check the returned price on a get-call. Additionally, the price set for the hardcoded oracle is not checked and can be `0`. The Chainlink price is only considered invalid if it is less than `0`, allowing for `0` to be a valid price.

**Recommendation(s)**: Consider establishing a unified definition of a zero price for all types of oracles and implementing the set and get functions to behave consistently when the price is zero.

**Status**: Fixed

**Update from the client**: FluxOracle contract now prevents setting price=0 for Hardcoded types and retrieving zero prices for all Oracle types. Fixed in: [48d5c4052795b082e876926f18f884927bb653b9](https://github.com/ondoprotocol/flux/commit/48d5c4052795b082e876926f18f884927bb653b9).

---

### [Info] Events in RWA oracles differ

**File(s)**: [`RWAOracleRateCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleRateCheck.sol), [`RWAOracleExternalComparisonCheck.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol)

**Description**: The `RWAOracleRateCheck` and `RWAOracleExternalComparisonCheck` contracts can both be assigned as oracles to a `fToken` with the same specified in the `FluxOracle`. As such, they are interchangeable. However, it should be noted that the events emitted when the price is set to contain a varying number of fields. This discrepancy may pose a problem if these logs are utilized by off-chain infrastructure. The events in both contracts are presented below:

////////////////////////
// RWAOracleRateCheck //
////////////////////////
event RWAPriceSet(
  int256 oldPrice, 
  int256 newPrice, 
  uint256 timestamp
);

//////////////////////////////////////
// RWAOracleExternalComparisonCheck //
//////////////////////////////////////
event RWAPriceSet(
  int256 oldChainlinkPrice,
  uint80 indexed oldRoundId,
  int256 newChainlinkPrice,
  uint80 indexed newRoundId,
  int256 oldRWAPrice,
  int256 newRWAPrice
);

**Recommendation(s)**: Maintain consistency in the number of fields included in the RWAPriceSet event. Alternatively, change the event names to correspond to the contracts that emit them.

**Status**: Fixed

**Update from the client**:
We have changed the name of the event in `RWAOracleExternalComparisonCheck` in this commit. [22716a82e7382ab2479d6fe74f79d59e43879a65](https://github.com/ondoprotocol/flux/commit/22716a82e7382ab2479d6fe74f79d59e43879a65).

---

### [Info] Incorrect check for decimals

**File(s)**: [`RWAOracleExternalComparisonCheck`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/rwaOracles/RWAOracleExternalComparisonCheck.sol)

**Description**: The constructor includes a check to ensure that the Chainlink oracle returns at least 4 decimals. The relevant code is shown below:

constructor(
    int256 initialPrice,
    address _chainlinkOracle,
    string memory _description,
    address admin,
    address _setter
  ) {
    chainlinkOracle = AggregatorV3Interface(_chainlinkOracle);
    if (chainlinkOracle.decimals() < 4) revert CorruptedChainlinkResponse();
    ....
}

However, it should be noted that the current minimum number of decimals returned by Chainlink is 8 for non-ETH pairs. In the event that the returned decimals are less than 8 but greater than 3, which could indicate a malfunction of Chainlink, this would not be detected by the contract.

**Recommendation(s)**: Consider checking if the decimals returned by chainlink is 8.

**Status**: Fixed

**Update from the client**:
We have changed the check to `decimals==8` in this commit. [f983d5487e568016ad1ae2e4db72c2ec50d2b74d](https://github.com/ondoprotocol/flux/commit/f983d5487e568016ad1ae2e4db72c2ec50d2b74d).

---

### [Best Practice] `renounceOwnership(...)` function should be disabled

**File(s)**: [`contracts/lending/fTokenOracle/fTokenOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fTokenOracle/fTokenOracle.sol)

**Description**: The `fTokenOracle` contract, which is inherited from the `Ownable` contract, includes the `renounceOwnership(...)` function. This function allows the owner to be removed from the contract. Below is a presentation of the function:

//////////////////////////////////////////////////////////////////////////
// @audit Renouncing ownership will leave the contract without an owner //
//////////////////////////////////////////////////////////////////////////
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

**Recommendation(s)**: If the `fTokenOracle` is intended to operate with an owner, consider disabling the `renounceOwnership(...)` function.

**Status**: Acknowledged

**Update from the client**:

---

### [Best Practice] Use of magic numbers

**File(s)**: [`fTokenOracle.sol`](https://github.com/ondoprotocol/flux/blob/6face44a82b7300be928cba30cd970636a77d93a/contracts/lending/fTokenOracle/fTokenOracle.sol)

**Description**: The `fTokenOracle` contract calculates the `scaleFactor` using the following equation:

/////////////////////////////////////////////////////////
// @audit Constant 8 is not explained 
/////////////////////////////////////////////////////////
scaleFactor = 10 ** (priceFeedDecimals - 8 + fTokenUnderlyingDecimals);

The constant `8` is used in the calculation, but its purpose is not explained.

**Recommendation(s)**: It is recommended to store constants as variables and provide a description that explains their purpose. This would improve the readability and maintainability of the code.

**Status**: Fixed

**Update from the client**: Removed the magic number in this PR: [0b86a1b554a8c345faef8b22045c718e52e2d533](https://github.com/ondoprotocol/flux/commit/0b86a1b554a8c345faef8b22045c718e52e2d533).

---