Ondo Finance contest - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

1 Other contract addresses can only be set once

Other contract addresses are immutable so they can only be set once in the constructor. There is no other setter method for this.

This makes it so there is no room for error and can lead to contract reverts and redeployment.

A solution for this is to make a setter method which only the owner can call. So if the contract address is wrong it can always be reset.

CashManager.sol

L163:   collateral = IERC20(_collateral);
L164:   cash = Cash(_cash);

2 Everyone can call initialize() function

When the contract is not initialized, the initialize() function can be called by anybody. This can be an issue because roles will be set in the __ERC20PresetMinterPauser_init function.

• CashKYCSender
• CashKYCSenderReceiver
• CCash.sol
• CErc20.sol

3 type(uint).max is not equal to -1

Documentation says fail if the repayAmount is -1. However type(uint).max is not equal to -1. Better use just -1 here.

    /* Fail if repayAmount = -1 */
    if (repayAmount == type(uint).max) {
      revert LiquidateCloseAmountIsUintMax();
    }

Non critical

1 Don't use keccack256 in constants

Use the literal bytes32 value instead of the keccak256 or other computations

CashManager.sol

L122:  bytes32 public constant MANAGER_ADMIN = keccak256("MANAGER_ADMIN");
L123:  bytes32 public constant PAUSER_ADMIN = keccak256("PAUSER_ADMIN");
L124:  bytes32 public constant SETTER_ADMIN = keccak256("SETTER_ADMIN");

Also found in the following contracts:

• CashFactory.sol
• CashKYCSenderFactory.sol
• CashKYCSenderReceiverFactory.sol
• KYCRegistry.sol
• Cash.sol
• CashKYCSender.sol
• CashKYCSenderReceiver.sol

2 Require() or revert() statement that check input arguments should be at the top of the function

This way no gas is wasted when the function is destined to fail.

• CashManager.sol#L249-L251
• CashManager.sol#L716-L718
• Cash.sol#L36-L39
• CashKYCSender.sol#L63-L66
• CashKYCSenderReceiver.sol#L63-L66

3 Similar contracts

CashKYCSender and CashKYCSenderReceiver are almost Identical.

It's better to work with an abstract contract here where both of the contracts inherit from. This will save a lot of duplicate code.

The same applies for

CashKYCSenderFactory and CashKYCSenderReceiverFactory

For the two erc20 contracts CTokenModified.sol and CTokenCash.sol there can also be one main contract where both of them inherit from, there are only 4 functions that are different so it will remove a lot of duplicate code.

Same applies for CCash.sol and CErc20.sol

4 Use modifier instead of duplicate require

• CTokenCash.sol#L1064
• CTokenCash.sol#L1116
• CTokenCash.sol#L1155
• CTokenCash.sol#L1258
• CTokenCash.sol#L1321 if (msg.sender != admin) is called 5 times. It's better to use a modifier instead. It improves readability. One reason why you might have done this is because it reverts a different error message each time. But for this error you can also just a have one error message: revert SENDERISNOTADMIN for example .

Same thing can be done for the statement if (accrualBlockNumber != getBlockNumber()). This is called 9 times

5 Multiple address mappings can be combined into a single mapping of an address to a struct

OndoPriceOracleV2.sol#L55-L73

  /// @notice fToken to Oracle Type association
  mapping(address => OracleType) public fTokenToOracleType;

  /// @notice Contract storage for fToken's underlying asset prices
  mapping(address => uint256) public fTokenToUnderlyingPrice;

  /// @notice fToken to cToken associations for piggy backing off
  ///         of Compound's Oracle
  mapping(address => address) public fTokenToCToken;

  struct ChainlinkOracleInfo {
    AggregatorV3Interface oracle;
    uint256 scaleFactor;
  }

  /// @notice fToken to Chainlink oracle association
  mapping(address => ChainlinkOracleInfo) public fTokenToChainlinkOracle;

  /// @notice Price cap for the underlying asset of an fToken. Optional.
  mapping(address => uint256) public fTokenToUnderlyingPriceCap;

Can be changed to

  struct tokenInfo{
    OracleType oracleType;
    uint256 underlyingPrice;
    address cToken;
    ChainlinkOracleInfo oracleInfo;
    uint256 underlyingPriceCap;
  }
  /// @notice fToken to Oracle Type association
  mapping(address => tokenInfo) public fTokenToInfo;


  struct ChainlinkOracleInfo {
    AggregatorV3Interface oracle;
    uint256 scaleFactor;
  }

Miscellaneous

Checking for under or overflow is useless

The evm will revert whenever over or underflow occurs because the contract is using a higher version of solidity than 0.8.0. So I don't see the point of doing this seperately. CTokenCash.sol#L121-L130

My solution with also a bit of gas saved:

    /* Do the calculations, checking for {under,over}flow */
    uint allowanceNew = startingAllowance - tokens;
-   uint srcTokensNew = accountTokens[src] - tokens;
-   uint dstTokensNew = accountTokens[dst] + tokens;
+   accountTokens[src] = accountTokens[src] - tokens;
+   accountTokens[dst] = accountTokens[dst] + tokens;
    /////////////////////////
    // EFFECTS & INTERACTIONS
    // (No safe failures beyond this point)

-   accountTokens[src] = srcTokensNew;
-   accountTokens[dst] = dstTokensNew;

Summary

Details

1 Make for loops unchecked

The risk of for loops getting overflowed is extremely low. Because it always increments by 1. Even if the arrays are extremely long, it will take a massive amount of time and gas to let the for loop overflow.

KYCRegistry.sol#L163-L165

addKycAdresses function avg gas saved: 414

The more addresses you want to add, the more gas you will save with this gas optimization

L163
+   uint256 i;
-	for (uint256 i = 0; i < length; i++) {
+   for (; i < lenght; ){ 
		kycState[kycRequirementGroup][addresses[i]] = true;
+	unchecked {
+		i++;
+		}		
	}

It can also be done on every other for loop in the project. The larger the array the more gas you will save.

2 Save storage variable to memory when it's used more than once in a function

When a storage variable is read for the second time from storage it costs 100 gas. When it's read from memory it only costs 3 gas. CashManager.sol#L241-L269

claimMint function avg gas saved: 130

  function claimMint(
    address user,
    uint256 epochToClaim
  ) external override updateEpoch nonReentrant whenNotPaused checkKYC(user) {
+   uint256 epochToExchangeRateMemory = epochToExchangeRate[epochToClaim]; 
    uint256 collateralDeposited = mintRequestsPerEpoch[epochToClaim][user];
    if (collateralDeposited == 0) {
      revert NoCashToClaim();
    }
-   if (epochToExchangeRate[epochToClaim] == 0) {
+   if (epochToExchangeRateMemory == 0) {
      revert ExchangeRateNotSet();
    }

    // Get the amount of CASH due at a given rate per epoch
    uint256 cashOwed = _getMintAmountForEpoch(
      collateralDeposited,
      epochToClaim
    );

    mintRequestsPerEpoch[epochToClaim][user] = 0;
    cash.mint(user, cashOwed);

    emit MintCompleted(
      user,
      cashOwed,
      collateralDeposited,
-     epochToExchangeRate[epochToClaim],
+     epochToExchangeRateMemory
      epochToClaim
    );
  }

CashManager.sol#L576-L587

transitionEpoch function avg gas saved: 89

  function transitionEpoch() public {
+   uint256 epochDurationMemory = epochDuration;    
    uint256 epochDifference = (block.timestamp - currentEpochStartTimestamp) /
-      epochDuration;
+      epochDurationMemory 
    if (epochDifference > 0) {
      currentRedeemAmount = 0;
      currentMintAmount = 0;
      currentEpoch += epochDifference;
      currentEpochStartTimestamp =
        block.timestamp -
-       (block.timestamp % epochDuration);
+       (block.timestamp % epochDurationMemory);
    }
  }

OndoPriceOracle.sol#L61-L72

getUnderlyingPrice() avg gas saved: 77

  function getUnderlyingPrice(
    address fToken
  ) external view override returns (uint256) {
+   uint256 price = fTokenToUnderlyingPrice[fToken];
-   if (fTokenToUnderlyingPrice[fToken] != 0) {
-     return fTokenToUnderlyingPrice[fToken];
+   if (price != 0) {
+     return price;
    } else {
      // Price is not manually set, attempt to retrieve price from Compound's
      // oracle
      address cTokenAddress = fTokenToCToken[fToken];
      return cTokenOracle.getUnderlyingPrice(cTokenAddress);
    }
  }

Other places where this can be done:

• CashManager.sol#L281-L319 lastSetMintExchangeRate: avg gas saved: 544

3 Don't write old value to memory

Instead of writing the old value to memory and putting it in the event at the end. You can put the event first so you don't have to write the old value to memory.

Around 20 gas saved per function: CashManager.sol#L596-L600

  function setMintLimit(uint256 _mintLimit) external onlyRole(MANAGER_ADMIN) {
+   emit MintLimitSet(mintlimit, _mintLimit); 
-   uint256 oldMintLimit = mintLimit;
    mintLimit = _mintLimit;
-   emit MintLimitSet(oldMintLimit, _mintLimit);
  }

Can also be done at

• CashManager.sol#L314-L317
• CashManager.sol#L395-L400
• CashManager.sol#L416-L418
• CashManager.sol#L439-L444
• CashManager.sol#L455-L457
• CashManager.sol#L468-L470
• CashManager.sol#L549-L551
• CashManager.sol#L612-L614
• CashManager.sol#L806-L808
• CashManager.sol#L820-L822
• KYCRegistryClient.sol#L43-L45
• KYCRegistryClient.sol#L55-L57
• CTokenCash.sol#L1064-L1066
• CTokenCash.sol#L1169-L1172
• CTokenCash.sol#L1368-L1370
• OndoPriceOracle.sol#L81-L83
• OndoPriceOracle.sol#L96-98
• OndoPriceOracle.sol#L107-L109
• OndoPriceOracleV2.sol#L168-L170
• OndoPriceOracleV2.sol#L183-L185
• OndoPriceOracleV2.sol#L198-L200
• OndoPriceOracleV2.sol#L237-L241
• OndoPriceOracleV2.sol#L312-L317

4 Function getBlockNumber is useles

The function only returns the blocknumber and does nothing else.

CTokenModified.sol#L246-L247

  function getBlockNumber() internal view virtual returns (uint) {
    return block.number;
  }

It's cheaper and even shorter to just call block.number directly. Calling block.number directly only costs 2 gas.

Around 30 gas can be saved per function where you apply this.

L62:
-     accrualBlockNumber = getBlockNumber();
+     accrualBlockNumber = block.number; 

Documentation says the function is mainly used for inheriting test contracts to stub the result. If that's the case the function can still exist but don't use the function anywhere else in the contract.

This also applies for CTokenCash.sol

5 Don't emit 2 similar events in 1 function

Having a transfer event and a mint event is useless instead use address(0) in the transfer event to make clear it's a mint.

• CTokenCash.sol#L539
• CTokenModified.sol#L539 Mint function gas saved: 1289

-    emit Mint(minter, actualMintAmount, mintTokens);
-    emit Transfer(address(this), minter, mintTokens);
+    emit Transfer(address(0), minter, mintTokens);

Try to minimize emitting events in other functions as well. It costs a lot of gas to emit an event.

Miscellaneous

Switch order of enums

IOndoPriceOracleV2.sol#L71-L76

According to the tests, in the function setFTokenToOracleType is the least set to UNINITIALIZED. I don't know if this will occur in real life also. If it does it's better to place the value that gets set the most at the first place in the enum. Writing the first value of an enum to a variable requires way less gas. In the tests this was an average saving of 4000 gas in the setFTokenToOracleType() function with the following. setup.

  enum OracleType {
+   MANUAL,
    UNINITIALIZED,
-   MANUAL,
    COMPOUND,
    CHAINLINK
  }

Make owner immutable

JumpRateModelV2.sol#L24

Owner is only set in the constructor so it's best to make it immutable to save some gas on deployment.