Ondo Finance - dd0x7e8's results

Burn Anyone's rOUSG

https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L624C1-L640C4

In the ROUSG contract, the burn function allows an address with the BURNER_ROLE role to directly burn rOUSG tokens of a specified user. However, the corresponding OUSG tokens that should be exchanged as compensation for the burnt rOUSG are not transferred to the user; instead, they are given to the caller. This results in a loss for the user.

Here are a few possible reasons why the contract might be designed with this feature:

1. Emergency Situations: If the rOUSG system encounters a critical security issue or vulnerability, there may be a need to urgently stop the circulation of tokens and reclaim them. In such cases, an authorized administrator or guardian might require the authority to destroy tokens to protect user assets or maintain system stability.

2. Compliance or Regulatory Requirements: If a user's funds are of unclear origin or suspected of illegal activities, there might be a need for the capability to freeze or confiscate these funds. This design allows contract administrators to take action when compliance or legal requirements necessitate it.

3. Error Correction: If users incorrectly receive rOUSG tokens due to some error, there may need to be a mechanism to correct this, which might involve destroying tokens that should not have been distributed.

However, this design does indeed bring risks and potential for abuse, as administrators or roles with the burning privilege could misuse this function to deprive users of their assets, leading to a loss of trust and compromising the overall security of the system.

In summary, while the burn method may have its specific uses, it must be implemented cautiously, ensuring that sufficient safeguards are in place to prevent abuse and protect the interests of users.

Inconsistent Price Check in getOUSGPrice of ROUSG Contract

There is indeed an inconsistency in the getOUSGPrice function between the two contracts, OUSGInstantManager and ROUSG.

OUSGInstantManager Contract:

The getOUSGPrice function in the OUSGInstantManager contract includes a safety check to ensure that the price returned by the oracle is not unexpectedly low:

function getOUSGPrice() public view returns (uint256 price) {
  (price, ) = oracle.getPriceData();
  require(
    price > MINIMUM_OUSG_PRICE,
    "OUSGInstantManager::getOUSGPrice: Price unexpectedly low"
  );
}

This check ensures that the price is above a defined MINIMUM_OUSG_PRICE, which acts as a circuit breaker in case of oracle malfunction or other unexpected behaviors that could result in a price that is too low, potentially leading to adverse effects on the contract's economics.

ROUSG Contract:

In contrast, the getOUSGPrice function in the ROUSG contract does not perform any safety checks on the price returned by the oracle:

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

This version of the function simply returns the price provided by the oracle without any additional validation. As a result, if the oracle were to return a price that is too low (or even zero), the contract would accept it as is, which could lead to incorrect calculations of token balances, shares, and potentially other critical financial operations within the contract.

The inconsistency in the getOUSGPrice function between the two contracts could lead to different behaviors and security postures. In OUSGInstantManager, the system is protected against extremely low oracle prices, while in ROUSG, it is not.

In scenarios where an oracle malfunction or attack results in an artificially low price, the ROUSG contract could incorrectly calculate token balances and shares, leading to significant financial losses or allowing for exploits such as the unwrapping of tokens at incorrect rates.

To resolve this inconsistency, it would be prudent to align the getOUSGPrice function across both contracts by implementing the same safety checks. This would ensure that both contracts operate under the same assumptions and protections regarding the price data they rely on.

Redundant whenNotPaused Modifier in ROUSG Contract

In the rOUSG contract, the whenNotPaused modifier is used to prevent certain functions from being executed when the contract is paused.

In the provided snippet, the whenNotPaused modifier is applied to the _mintShares and _burnShares internal functions:

function _mintShares(
  address _recipient,
  uint256 _sharesAmount
) internal whenNotPaused returns (uint256) {
  // ... implementation
}

  function _burnShares(
    address _account,
    uint256 _sharesAmount
) internal whenNotPaused returns (uint256) {
    // ... implementation
}

This internal function appears to be called by the external functions, which also have the whenNotPaused modifier applied:

The redundancy here is that the check for whether the contract is paused is being performed twice: once in the wrap function and once in the _mintShares internal function that wrap calls. This is unnecessary and could be considered inefficient because the state of the contract (paused or not) would not change within the same transaction, and checking it twice does not provide additional security or functional benefits.

It's generally a best practice to apply such modifiers at the highest level of publicly accessible functions (i.e., external or public functions that are part of the contract's interface) rather than on internal functions. This approach minimizes the number of checks and gas costs associated with function calls while maintaining the intended security constraints.

Therefore, to improve gas efficiency and avoid redundancy, the whenNotPaused modifier should only be applied to the external functions and not to the internal functions. The external functions serve as the entry point for the transaction, and once the contract state is checked and validated at this point, all subsequent internal calls within the same transaction can be considered safe to proceed without additional pause checks.