Ondo Finance contest - RaymondFam's results

Missing minter role granting

In deployCash() of CashFactory.sol, cashProxied.grantRole() did not grant MINTER_ROLE to guardian address. Similarly, in deployCashKYCSender() of CashKYCSenderFactory.sol, cashKYCSenderProxied. did not grant MINTER_ROLE to guardian address either. (Note: The same instance also goes with deployCashKYCSenderReceiver() of CashKYCSenderReceiverFactory.sol). Although this can later be separately executed by the guardian who is now the default admin, consider having this specific role granting included in the atomic function call or having it commented in the NatSpec the reason for this omission.

File: CashFactory.sol#L75-L110

    cashProxied.grantRole(DEFAULT_ADMIN_ROLE, guardian);
    cashProxied.grantRole(PAUSER_ROLE, guardian);
+    cashProxied.grantRole(MINTER_ROLE, guardian);

File: CashKYCSenderFactory.sol#L98-L99

    cashKYCSenderProxied.grantRole(DEFAULT_ADMIN_ROLE, guardian);
    cashKYCSenderProxied.grantRole(PAUSER_ROLE, guardian);
+    cashKYCSenderProxied.grantRole(MINTER_ROLE, guardian);

File: CashKYCSenderReceiverFactory.sol#L98-L99

    cashKYCSenderReceiverProxied.grantRole(DEFAULT_ADMIN_ROLE, guardian);
    cashKYCSenderReceiverProxied.grantRole(PAUSER_ROLE, guardian);
+    cashKYCSenderReceiverProxied.grantRole(MINTER_ROLE, guardian);

Use of deprecated file

As denoted in OpenZeppelin's ERC20PresetMinterPauser.sol as well as Ondo Finance's ERC20PresetMinterPauserUpgradeable.sol:

This particular file is now deprecated in favor of https://wizard.openzeppelin.com/[Contracts Wizard].

Specifically, cashProxied.initialize(name, ticker) is invoked in deployCash() of CashFactory.sol. This leads to calling the parental initialize() in ERC20PresetMinterPauserUpgradeable.sol, which is discouraged.

File: CashFactory.sol#L87

    cashProxied.initialize(name, ticker);

File: ERC20PresetMinterPauserUpgradeable.sol#L36-L42

  function initialize(string memory name, string memory symbol)
    public
    virtual
    initializer
  {
    __ERC20PresetMinterPauser_init(name, symbol);
  }

Likewise, cashKYCSenderProxied.initialize(name, ticker, registry, kycRequirementGroup) is invoked in deployCashKYCSender() of CashKYCSenderFactory.sol, leading to the parental call on __ERC20PresetMinterPauser_init(name, symbol) in ERC20PresetMinterPauserUpgradeable.sol. (Note: An identical instance also happens on deployCashKYCSenderReceiver() of CashKYCSenderReceiverFactory.sol.)

File: CashKYCSenderFactory.sol#L91-L96

    cashKYCSenderProxied.initialize(
      name,
      ticker,
      registry,
      kycRequirementGroup
    );

File: CashKYCSender.sol#L46-L54

    __ERC20PresetMinterPauser_init(name, symbol);

File: ERC20PresetMinterPauserUpgradeable.sol#L53-L60

  function __ERC20PresetMinterPauser_init(

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are the contract instances lacking partial/full NatSpec that will be of added values to the users and developers if adequately provided:

File: Proxy.sol File: Cash.sol File: CashKYCSender.sol File: CashKYCSenderReceiver.sol

No Storage Gap for Upgradeable Contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain.

+ uint256[50] private __gap;

Here are the contract instances entailed:

File: Cash.sol File: CashKYCSender.sol File: CashKYCSenderReceiver.sol

Interfaces should be imported

Some contracts have interface(s) showing up in its/their entirety at the top/bottom of the contract facilitating an ease of references on the same file page. This has, in certain instances, made the already large contract size to house over a thousand code lines. Consider saving the interfaces entailed as Ixxx.sol respectively, and have them imported like all other files.

Here are some of the contract instances entailed:

File: cErc20ModifiedDelegator.sol#L9-L134 File: OndoPriceOracle.sol#L21-L29 File: IOndoPriceOracle.sol#L18-L25 File: CCash.sol#L6-L8 File: OndoPriceOracleV2.sol#L22-L36 File: CTokenInterfacesModifiedCash.sol File: cErc20ModifiedDelegator.sol#L9-L134 File: cErc20ModifiedDelegator.sol

Unspecific compiler version pragma

For some source-units the compiler version pragma is very unspecific, i.e. ^0.5.12, ^0.8.10 etc. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored as follows:

File: CashKYCSender.sol#L18-L19

- import "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol";
- import "contracts/cash/kyc/KYCRegistryClientInitializable.sol";

+ import {ERC20PresetMinterPauserUpgradeable} from "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol";
+ import {KYCRegistryClientInitializable} from "contracts/cash/kyc/KYCRegistryClientInitializable.sol";

Inconsistency in interface naming

Some interfaces in the code bases are named without the prefix I that could cause confusion to developers and readers referencing or interacting with the protocol. Consider conforming to Solidity's naming conventions by having the instances below refactored as follow:

File: cErc20ModifiedDelegator.sol#L9

- interface ComptrollerInterface {
+ interface IComptrollerInterface {

File: OndoPriceOracle.sol#L21

- interface CTokenOracle {
+ interface ICTokenOracle {

File: OndoPriceOracle.sol#L27

- interface CTokenLike {
+ interface ICTokenLike {

File: IOndoPriceOracle.sol#L18

- interface PriceOracle {
+ interface IPriceOracle {

File: CCash.sol#L6

- interface CompLike {
+ interface ICompLike {

All other instances entailed:

File: OndoPriceOracleV2.sol#L22-L30 File: cErc20ModifiedDelegator.sol#L9 File: cErc20ModifiedDelegator.sol#L144

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Missing checks for contract existence

Performing low-level calls without confirming contract’s existence (not yet deployed or have been destructed) could return success even though no function call was executed as documented in the link below:

https://docs.soliditylang.org/en/v0.8.7/control-structures.html#error-handling-assert-require-revert-and-exceptions

Consider having account existence checked prior to making call where possible.

Here are the three instances unanimously using the same/identical function logic:

File: CashFactory.sol#L128-L133 File: CashKYCSenderFactory.sol#L137-L142 File: CashKYCSenderReceiverFactory.sol#L137-L143

    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;

uint256 over uint

Across the codebase, there are numerous instances of uint, as opposed to uint256. In favor of explicitness, consider replacing all instances of uint with uint256.

Here are some of the contract instances entailed:

File: JumpRateModelV2.sol File: CCash.sol

Imprecise block time

Since resorting to POS, the Ethereum average block time is hardly 12 seconds according to the daily ycharts. 12 seconds per block only happens when no validator(s) is/are missing any slot(s) or turn(s) idling. For this reason, baseRatePerBlock, multiplierPerBlock, and jumpMultiplierPerBlock tend to be always slightly lesser than their actual values. Consider switching to precise time units in seconds if possible.

File: JumpRateModelV2.sol

29:  uint public constant blocksPerYear = 2628000; // block.time == 12

177:    baseRatePerBlock = baseRatePerYear.div(blocksPerYear);
178:    multiplierPerBlock = (multiplierPerYear.mul(1e18)).div(
179:      blocksPerYear.mul(kink_)
180:    );
181:    jumpMultiplierPerBlock = jumpMultiplierPerYear.div(blocksPerYear);

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = false instance below may be refactored as follows:

File: KYCRegistry.sol#L181

-      kycState[kycRequirementGroup][addresses[i]] = false;
+      delete  kycState[kycRequirementGroup][addresses[i]];

More robust sanity check

A sanity check was performed on underlying in initialize() of CCash.sol and CErc20.sol. Consider refactoring the code line entailed below just in case totalSupply() would not revert on failure:

File: CCash.sol#L53-L55
File: CErc20.sol#L53-L55

    // Set underlying and sanity check it
    underlying = underlying_;
-    EIP20Interface(underlying).totalSupply();
+    require(EIP20Interface(underlying).totalSupply() != 0);

Time Units

According to:

https://docs.soliditylang.org/en/v0.8.14/units-and-global-variables.html

suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit and units are considered naively in the following way:

1 == 1 seconds 1 minutes == 60 seconds 1 hours == 60 minutes 1 days == 24 hours 1 weeks == 7 days

To avoid human error while making the assignment more verbose, the following variable may be rewritten as:

File: OndoPriceOracleV2.sol#L77

-  uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours
+  uint256 public maxChainlinkOracleTimeDelay = 25 hours;

Valid price could include zero value

in getChainlinkOraclePrice() of OndoPriceOracleV2.sol, answer == 0, i.e. a meaningless zero scaled price could be returned. Consider having the affected code refactored as follows:

File: OndoPriceOracleV2.sol#L297

-    require(answer >= 0, "Price cannot be negative");
+    require(answer > 0, "Price cannot be negative or zero");

Better yet, set a low boundary to the lowest price acceptable to handle edge cases entailing dusts in very low wei.

Early checks

Checks via require statement in a function logic should be as early as possible to minimize gas waste in case of a revert.

For instance, the code block below can be refactored as follows:

File: Cash.sol#L29-L40

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 amount
  ) internal override {
-    super._beforeTokenTransfer(from, to, amount);

    require(
      hasRole(TRANSFER_ROLE, _msgSender()),
      "Cash: must have TRANSFER_ROLE to transfer"
    );

+    super._beforeTokenTransfer(from, to, amount);
  }

All other instances entailed:

File: CashKYCSenderReceiver.sol#L63-L66

File: CashKYCSender.sol#L63-L66

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: CashFactory.sol#L75-L110

  function deployCash(
    string calldata name,
    string calldata ticker
-  ) external onlyGuardian returns (address, address, address) {
+  ) external onlyGuardian returns (address cashProxiedAddr, address cashProxyAdminAddr, address cashImplementationAddr) {

    [ ... ]

-    return (
+    (cashProxiedAddr, cashProxyAdminAddr, cashImplementationAddr) =
      address(cashProxied),
      address(cashProxyAdmin),
      address(cashImplementation)
    );

Use break or continue in for loop

For loop entailing large array with reverting logic should incorporate break or continue to cater for element(s) failing to get through the iteration(s). This will tremendously save gas on instances where the loop specifically fails to execute at the end of the iterations.

Here are the three instances unanimously using the same/identical function logic:

File: CashFactory.sol#L123-L134 File: CashKYCSenderFactory.sol#L133-L144 File: CashKYCSenderReceiverFactory.sol#L133-L143

  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;
    }
  }

Private function with embedded modifier reduces contract size

Just as it has been implemented in the modifiers, checkKYC() and updateEpoch() of CashManager.sol, consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: CashFactory.sol#L151-L154

+    function _onlyGuardian() private view {
+        require(msg.sender == guardian, "CashFactory: You are not the Guardian");
+    }

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

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.16",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, the following <= inequality instance may be refactored as follows:

File: KYCRegistry.sol#L92

-    require(block.timestamp <= deadline, "KYCRegistry: signature expired");
+    require(block.timestamp < deadline + 1, "KYCRegistry: signature expired");

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ++i ;} to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

File: KYCRegistry.sol#L163-L165

-        for (uint256 i = 0; i < length; i++) {
+        for (uint256 i = 0; i < length;) {
             kycState[kycRequirementGroup][addresses[i]] = false;

+            unchecked {
+                ++i;
+            }
          }

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the function below may be refactored as follows:

File: KYCRegistry.sol#L144-L150

  function assignRoletoKYCGroup(
    uint256 kycRequirementGroup,
    bytes32 role
-  ) external onlyRole(REGISTRY_ADMIN) {
+  ) external payable onlyRole(REGISTRY_ADMIN) {
    kycGroupRoles[kycRequirementGroup] = role;
    emit RoleAssignedToKYCGroup(kycRequirementGroup, role);
  }

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here are some of the instances entailed:

File: OndoPriceOracleV2.sol#L292-L296

    require(
      (answeredInRound >= roundId) &&
        (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay),
      "Chainlink oracle price is stale"
    );

File: CTokenModified.sol#L45-L48

    require(
      accrualBlockNumber == 0 && borrowIndex == 0,
      "market may only be initialized once"
    );

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: CTokenCash.sol

221:    Exp memory exchangeRate = Exp({mantissa: exchangeRateCurrent()});

433:    Exp memory simpleInterestFactor = mul_(

506:    Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});

590:    Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});

1027:    Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});