Ondo Finance - MohammedRizwan's results

Summary

Low Risk Issues

[L‑01] The contracts has used outdated version of openzeppelin library

From package.json, It is confirmed that the contracts has used "@openzeppelin/contracts": "^4.8.3", which is an old version and has security issues. While using external libraries, Ensure it is of latest version.

Recommended Mitigation steps

Update the openzeppelin version to v4.9.3

[L‑02] stale or misleading comment in rUSDYFactory.sol

In rUSDYFactory.sol, There is misleading or stale comment. When asked about it sponsor made it stale Natspec.

          ii) Revoke the `MINTER_ROLE`, `PAUSER_ROLE` & `DEFAULT_ADMIN_ROLE` from address(this).

Recommended Mitigation steps

Remove the stale Natspec which are no longer needed in contract to prevent misleading.

[L‑03] multiexcall() being payable is seriously dangerous

In rUSDYFactory.sol, multiexcall() allows for arbitrary batched calls but it is made payable which will cause issues since address(exCallData[i].target).call is in a loop, it is not advisable to add payable to the existing call(). This is because msg.value may be used multiple times.

There is 1 instance of this issue:

File: contracts/usdy/rUSDYFactory.sol

  function multiexcall(
    ExCallData[] calldata exCallData
  ) external payable override onlyGuardian returns (bytes[] memory results) {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);
      require(success, "Call Failed");
      results[i] = ret;
    }
  }

Recommended Mitigation steps

Current implementation is not recommended, Howeve this discussion is helpful to mitigate the issue.

[L‑04] Calls inside loops that may address DoS

In rUSDYFactory.sol, multiexcall() function, Calls to external contracts inside a loop are dangerous because it could lead to DoS if one of the calls reverts or execution runs out of gas. Such issue also introduces chance of problems with the gas limits.

Per SWC-113: External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract. To minimize the damage caused by such failures, it is better to isolate each external call into its own transaction that can be initiated by the recipient of the call.

Reference link- https://swcregistry.io/docs/SWC-113

There is 1 instance of this issue:

File: contracts/usdy/rUSDYFactory.sol

  function multiexcall(
    ExCallData[] calldata exCallData
  ) external payable override onlyGuardian returns (bytes[] memory results) {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);
      require(success, "Call Failed");
      results[i] = ret;
    }
  }

Recommended Mitigation Steps

1. Avoid combining multiple calls in a single transaction, especially when calls are executed as part of a loop
2. Always assume that external calls can fail
3. Implement the contract logic to handle failed calls

Summary

Gas Optimizations

[G‑01] _rmul() can gas optimized by removing extra lines of code

_rmul() has been used in derivePrice(). With current implementation it has added extra bytes resulting in more deployment cost. This can be further gas optimized per recommendation.

There is 1 instance of this issue:

File: contracts/rwaOracles/RWADynamicOracle.sol

  function _rmul(uint256 x, uint256 y) internal pure returns (uint256 z) {
    z = _mul(x, y) / ONE;
  }

  function _mul(uint256 x, uint256 y) internal pure returns (uint256 z) {
    require(y == 0 || (z = x * y) / y == x);
  }

Recommended Mitigation steps

  function _rmul(uint256 x, uint256 y) internal pure returns (uint256 z) {
-    z = _mul(x, y) / ONE;
+    z = x * y;
+    require(y == 0 || z / y == x);
+       z = z / ONE;
  }

-  function _mul(uint256 x, uint256 y) internal pure returns (uint256 z) {
-    require(y == 0 || (z = x * y) / y == x);
-  }

[G‑02] Use assembly to emit events

Assembly can be used to emit events efficiently by utilizing scratch space and the free memory pointer. This will allow us to potentially avoid memory expansion costs. Note: In order to do this optimization safely, we will need to cache and restore the free memory pointer.

There are 2 instances of this issue:

File: contracts/rwaOracles/RWADynamicOracle.sol

  event RangeSet(
    uint256 indexed index,
    uint256 start,
    uint256 end,
    uint256 dailyInterestRate,
    uint256 prevRangeClosePrice
  );


  event RangeOverriden(
    uint256 indexed index,
    uint256 newStart,
    uint256 newEnd,
    uint256 newDailyInterestRate,
    uint256 newPrevRangeClosePrice
  );

[G‑03] Catch arguments as local variables in simulateRange()

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

There is 1 instance of this issue:

File: contracts/rwaOracles/RWADynamicOracle.sol

  function simulateRange(
    uint256 blockTimeStamp,
    uint256 dailyIR,
    uint256 endTime,
    uint256 startTime,
    uint256 rangeStartPrice
  ) external view returns (uint256 price) {

Recommended Mitigation steps

Catch function arguments as local variables.

[G‑04] Catch value in roundUpTo8()

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

  function roundUpTo8(uint256 value) internal pure returns (uint256) {
+    uint256 _value = value;
-    uint256 remainder = value % 1e10;
+    uint256 remainder = _value % 1e10;
    if (remainder >= 0.5e10) {
-      value += 1e10;
+      _value += 1e10;
    }
-    value -= remainder;
+    _value -= remainder;
-    return value;
+    _return value;
  }

[G‑05] save gas by using 10e27 instead 10 ** 27

The compiler needs to do extra computation while fetching the value 10 ** 27. However this can be optimized by using 10e27.

There is 1 instance of this issue:

Recommended Mitigation steps

-  uint256 private constant ONE = 10 ** 27;
+  uint256 private constant ONE = 10e27;

[G‑06] _rpow() and _rmul() can be imported instead of hardcoding it

_rpow() and _rmul() has been taken from makeDao contract, However these functions can be added in library instead of hardcoding it in contract. This will eliminate extra bytes and will make the contract simpler.

[G‑07] Short the string message in onlyGuardian to save 256 bit storage

Every character of string message count as 1 byte. Reduce it to save some gas.

  modifier onlyGuardian() {
-    require(msg.sender == guardian, "rUSDYFactory: You are not the Guardian");
+    require(msg.sender == guardian, "rUSDYFactory: Not Guardian");
    _;
  }