Ondo Finance - 0xmystery's results

Gas estimate on SourceBridge.burnAndCallAxelar

The UI may have the gas estimate taken care of. Nonetheless, users interacting with this function have no clue what amount of estimated msg.value to send along with the call. Apparently, burnAndCallAxelar() only checks if msg.value is not zero.

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L72-L74

    if (msg.value == 0) {
      revert GasFeeTooLow();
    }

While this prevents the latter from calling the function without sending any ether, it does not ensure the amount of ether sent is adequate to cover the gas fees.

Consider implementing the Axelar gas estimate to the function logic where possible.

rUSDY.approve poses a threat to unsavvy users

The contract already has increaseAllowance() and decreaseAllowance() implemented as an alternative to approve() that can be used as a mitigation to spender frontrunning the revised allowance. Where possible, remove approve() since user can always use increaseAllowance() for the same intended purpose.

Missing crucial check in RWADynamicOracle.simulateRange

Consider adding the following check:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L119-L128

    } else {
      Range memory lastRange = ranges[ranges.length - 1];
      uint256 prevClosePrice = derivePrice(lastRange, lastRange.end - 1);
+      // Check that the endTime is greater than the last range's end time
+      if (lastRange.end >= endTime) revert InvalidRange();
      rangeList[length] = Range(
        lastRange.end,
        endTime,
        dailyIR,
        prevClosePrice
      );
    }
    for (uint256 i = 0; i < length + 1; ++i) {
      Range memory range = rangeList[(length) - i];
      if (range.start <= blockTimeStamp) {
        if (range.end <= blockTimeStamp) {
          return derivePrice(range, range.end - 1);
        } else {
          return derivePrice(range, blockTimeStamp);
        }
      }
    }

This will prevent underflow on line 266 below when internally calling derivePrice() in the for loop above:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L262-L274

  function derivePrice(
    Range memory currentRange,
    uint256 currentTime
  ) internal pure returns (uint256 price) {
266:    uint256 elapsedDays = (currentTime - currentRange.start) / DAY;
    return
      roundUpTo8(
        _rmul(
          _rpow(currentRange.dailyInterestRate, elapsedDays + 1, ONE),
          currentRange.prevRangeClosePrice
        )
      );
  }

Comment and code mismatch

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L211-L212

-      // Ensure that the newStart time is less than the end time of the previous range
+      // Ensure that the newStart time is not less than the end time of the previous range
      if (newStart < ranges[indexToModify - 1].end) revert InvalidRange();

Chain reorganization attack

As denoted in the Moralis academy article:

"... If a node receives a new chain that’s longer than its current active chain of blocks, it will do a chain reorg to adopt the new chain, regardless of how long it is."

Depending on the outcome, if it ended up placing the transaction earlier than anticipated, quite a number of the function calls could backfire. For instance, a user calling rUSDY.transfer() could end up transferring more shares than anticipated due to a lower USDY price entailed.

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L388-L392

  function getSharesByRUSDY(
    uint256 _rUSDYAmount
  ) public view returns (uint256) {
    return (_rUSDYAmount * 1e18 * BPS_DENOMINATOR) / oracle.getPrice();
  }

(Note: On Ethereum this is unlikely but this is meant for contracts going to be deployed on any compatible EVM chain many of which like Polygon, Optimism, Arbitrum are frequently reorganized.)

Non-linear vs linear representation

The graphical representation of the price of evolution of USDY over time in README.md should be non-linear step wise instead of a continuous straight line plot to better portray the price appreciation via the daily interest set for the month.

Smaller periodic USDY appreciation

The interest rate derived for each subsequent day based on the interest rate for the month presents kinks that is too abrupt around the end of the day and the beginning of the next day.

Imagine two users transferring their rUSDY tokens one second apart such that the user A did it at 23:59:59 and user B did it at 00:00:00. For a matter of a second difference, the user A ended up unfavorably transferring a greater amount of shares due to a smaller denominator entailed on the return code line below:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L388-L392

  function getSharesByRUSDY(
    uint256 _rUSDYAmount
  ) public view returns (uint256) {
return (_rUSDYAmount * 1e18 * BPS_DENOMINATOR) / oracle.getPrice();
  }

This could create imbalanced feelings where the user wished he/she did it seconds/minutes/... later.

Consider reducing the daily interest appreciation model to a smaller periodic model to help circumvent the aforesaid situations.

Incorrect arguments associated getRUSDYByShares() in the function logic of _burnShares()

The preRebaseTokenAmount and postRebaseTokenAmount values will always be identical, which doesn't provide any meaningful information to the user about the impact of the burn operation on the value of their shares.

Consider having the affected codes refactored as follows:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L575-L601

  function _burnShares(
    address _account,
    uint256 _sharesAmount
  ) internal whenNotPaused returns (uint256) {
    require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS");

    _beforeTokenTransfer(_account, address(0), _sharesAmount);

    uint256 accountShares = shares[_account];
    require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE");

-    uint256 preRebaseTokenAmount = getRUSDYByShares(_sharesAmount);
+    uint256 preRebaseTokenAmount = getRUSDYByShares(accountShares);

    totalShares -= _sharesAmount;

+    uint256 postAccountShares = accountShares - _sharesAmount;
-    shares[_account] = accountShares - _sharesAmount;
+    shares[_account] = postAccountShares;

-    uint256 postRebaseTokenAmount = getRUSDYByShares(_sharesAmount);
+    uint256 postRebaseTokenAmount = getRUSDYByShares(postAccountShares);

    emit SharesBurnt(
      _account,
      preRebaseTokenAmount,
      postRebaseTokenAmount,
      _sharesAmount
    );

    return totalShares;

Typo/grammatical mistakes

Here's a grammatical mistake not found by the bot:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L252

-   *      and will thresholds corresponding to the params of this function. Passing
+   *      and will set thresholds corresponding to the params of this function. Passing

CEI recommendation

Here's an instance of check-effects-interaction practice not captured by the bot.

Where possible, transfer the needed USDY amount from the caller prior to minting the equivalent amount of shares:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L434-L440

  function wrap(uint256 _USDYAmount) external whenNotPaused {
    require(_USDYAmount > 0, "rUSDY: can't wrap zero USDY tokens");
-    _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR);
    usdy.transferFrom(msg.sender, address(this), _USDYAmount);
+    _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR);
    emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount));
    emit TransferShares(address(0), msg.sender, _USDYAmount);
  }

In this comprehensive analysis of the specified smart contracts, I shed light on an array of vulnerabilities, potential misconfigurations, and areas of improvement, ranging from DOS threats and rate limiter exploitation to architectural and systemic challenges. By meticulously reviewing each function and mechanism, juxtaposed with the intended logic and business models provided by the developer documentation, I present a holistic view of the protocol's current state, its risks, and provide actionable recommendations to bolster its resilience and functionality.

Comments for the judge to contextualize my findings

1. DOS on DestinationBridge._execute arising from an intended design in DestinationBridge.setThresholds (Medium) The DestinationBridge.setThresholds function in the DestinationBridge contract allows for the removal of all previously set thresholds for a specified chain. If empty arrays are passed to the function, the associated chainToThresholds[srcChain] becomes an empty Threshold[], which subsequently bypasses the subsequent for loop. This configuration poses a problem when the _attachThreshold() function is later invoked by DestinationBridge._execute: the function's logic will cause a revert due to a threshold match failure. This interruption can halt or impede the regular progression of transactions, especially when there are no defined thresholds to validate against. The issue was identified manually. To mitigate, a recommendation has been proposed to adjust the conditional statement in _attachThreshold() to only revert when no threshold match is identified and thresholds were indeed present to be checked against.

2. Misconfiguration in DestinationBridge.setThresholds could lead to Exploitation of the Minting Rate Limiter (Medium) The setThresholds function in the DestinationBridge contract has a flaw where it ensures transaction amounts are in ascending order but neglects to do the same for the number of approvers (numOfApprovers). This oversight could allow large transactions to require fewer approvals, enabling potential misuse. To fix this, the function should be updated to ensure both transaction amounts and their corresponding number of approvers are sorted in ascending order.

3. The Zero-Sum Nature of the Wrap/Unwrap Mechanism in rUSDY Token System and its Associated Risks (Medium) The rUSDY token system's wrap/unwrap mechanism poses significant risks without clear benefits. While it facilitates token transfer in USD and approves spenders' allowances, the zero-sum nature of wrapping and unwrapping doesn't provide any net gain. Users can lose access to their assets if they are later sanctioned or blocked, a concern exacerbated by the system's ability to destroy these assets. The system's value is also questionable, given simpler alternatives such as oracle.getPrice(). Recommendations include re-evaluating the mechanism's value, strengthening protections against unjust user blocking, improving transparency on sanctions, providing asset recovery options for penalized users, and educating users about associated risks.

Approach taken in evaluating the codebase

I basically reread README.md multiple times and even checked on the relevant sections Axelar Docs. Doing this has tremendously helped a lot in thoroughly understanding the intended accounting and business logic of the protocol regardless of the bug hunting results. Paying attention to what the sponsor has explained and highlighted in the Discord Channel is also of paramount importance to focus on the steered direction.

Architecture recommendations

For the SourceBridge.burnAndCallAxelar function, non-UI users can be unclear about the amount of msg.value to send. Though the function checks if the msg.value is not zero, it doesn't ensure the sent ether is adequate for gas fees. A suggestion is made to implement the Axelar gas estimate at contract level. In the rUSDY contract, the approve function poses risks to uninformed users due to potential spender frontrunning. It's recommended to remove the approve function since alternatives like increaseAllowance exist. There's also a crucial check missing in the RWADynamicOracle.simulateRange function that prevents underflow issues. A mismatch between comments and code logic is found in the RWADynamicOracle.sol contract. Furthermore, the contracts may be vulnerable to chain reorganization attacks, especially on EVM-compatible chains that experience frequent reorgs. Issues with the representation of USDY price evolution over time and abrupt interest rate changes on the rUSDY contract could lead to imbalances in user transactions. Also, incorrect arguments in the _burnShares function need refactoring. Lastly, the report identifies a typo in the DestinationBridge.sol contract and recommends a check-effects-interaction practice adjustment in the rUSDY.sol contract.

Codebase quality analysis

This is one of the well structured and secured codebases out there where hardly any critical vulnerabilities could ever be detected and identified. A more complete set of NatSpec on top of more elaborate/extensive documentations would greatly add to the its quality. Perhaps, the protocol should also look into providing some forms of flow charts. This will to a greater extent improve the readability and comprehension of the logic intentions to both the readers and the developers.

Centralization risks

The sponsor has specified quite a number of known issues hovering much on centralization. As such, there is not much I can commend although I would want to highlight the admin roles decide a lot on users' trust needing further affirmations that the latter will not be rug pulled.

Mechanism review

Diving into the mechanics of the protocol reveals the underpinnings of its operations and the inherent vulnerabilities tied to its design. The rUSDY token system's wrap/unwrap mechanism stands out due to its zero-sum nature. On paper, this system provides a seamless way for users to convert their assets. In practice, however, the mechanism poses substantial risks. These risks are not offset by commensurate benefits, thereby questioning the utility of the mechanism in its current form. Not only does it open up the potential for users to lose access to their assets under certain conditions, but the lack of net gain from wrapping and unwrapping complicates the user experience without apparent benefits. This scrutiny suggests that the mechanism requires a comprehensive review, possibly leading to a redesign that keeps user safety and usability at the forefront.

Systemic risks

In conjunction to the centralized issues, I suggest that the protocol would at least look into a more decentralized model starting from a multisig model that could eventually transition to a DAO governed body. Doing this will greatly foster users' sense of ownership and widely expand the horizon of the community base in a mutual sense.

Time spent:

25 hours