Ondo Finance contest - Udsen's results

Institutional-Grade Finance. On-Chain. For Everyone.

General Information

Platform: Code4rena

Start Date: 11/01/2023

Pot Size: $60,500 USDC

Total HM: 6

Participants: 69

Period: 6 days

Judge: Trust

Total Solo HM: 2

Id: 204

League: ETH

Ondo Finance

Findings Distribution

Researcher Performance

Rank: 24/69

Findings: 1

Award: $304.58

QA:

grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Ondo Finance

Findings Information

🌟 Selected for report: CodingNameKiki

Also found by: 0x1f8b, 0x52, 0x5rings, 0xAgro, 0xSmartContract, 0xcm, 0xkato, 2997ms, Aymen0909, BClabs, BPZ, BRONZEDISC, Bauer, Bnke0x0, Deekshith99, IllIllI, Josiah, Kaysoft, RaymondFam, Rolezn, SaeedAlipoor01988, Tajobin, Udsen, Viktor_Cortess, adriro, arialblack14, betweenETHlines, btk, chaduke, chrisdior4, cryptphi, csanuragjain, cygaar, defsec, descharre, erictee, gzeon, hansfriese, horsefacts, joestakey, koxuan, lukris02, luxartvinsec, nicobevi, oyc_109, pavankv, peanuts, rbserver, scokaf, shark, tnevler, tsvetanovv, zaskoh

Awards

304.5822 USDC - $304.58

Labels

bug

grade-a

QA (Quality Assurance)

Q-02

External Links

Link to GitHub issue

1. USE NAMED IMPORTS INSTEAD OF PLAIN `IMPORT ‘FILE.SOL’

Can import the required specific contracts, functions or variables by using the named imports explicitly. Plain imports will import the entire context of the imported contract which could lead into variable name conflicts etc ...

Currently the TransparentUpgradeableProxy is imported as follows:

import "contracts/cash/external/openzeppelin/contracts/proxy/TransparentUpgradeableProxy.sol";

But it can be imported explicitly by the name as follows:

import {TransparentUpgradeableProxy} from "contracts/cash/external/openzeppelin/contracts/proxy/TransparentUpgradeableProxy.sol";

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/Proxy.sol#L18

There are 17 more instances of this issue:

2. NATSPEC IS MISSING

NatSpec is missing for the following functions , constructor and modifier

contract TokenProxy is TransparentUpgradeableProxy {
	constructor(
		address _logic,
		address _admin,
		bytes memory _data
	) TransparentUpgradeableProxy(_logic, _admin, _data) {}
}

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/Proxy.sol#L21

There are 1 more instances of this issue:

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/token/Cash.sol#L29

3. NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

pragma solidity ^0.8.10;

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCashDelegate.sol#L2

There are 6 more instances of this issue:

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CTokenDelegate.sol#L2 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/JumpRateModelV2.sol#L1 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L2 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CErc20.sol#L2 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CTokenInterfacesModifiedCash.sol#L2 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol#L2

4. MISSING `address(0)` CHECKS FOR THE FOLLOWING ADDRESS INPUTS TO THE CONSTRUCTORS AND FUNCTIONS.

_setFTokenToCToken(fToken, cToken);

By mistake fToken and cToken could be set to address(0). It is a good practice to check for address(0).

require(fToken != address(0), "Address cannot be zero");
require(cToken != address(0), "Address cannot be zero");

Hence it is recommended to check for the address(0) for the fToken adn cToken variable before _setFTokenToCToken(fToken, cToken) is called.

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/OndoPriceOracle.sol#L92-L99

There are 9 more instances of this issue:

5.`assert()` FUNCTION IS USED WITHOUT DESCRIPTION MESSAGE FOR THE REVERT.

assert(cashProxyAdmin.owner() == guardian);

Add a description message to the assert function for the revert. It is very important to add a revert message, then msg.sender has enough information to learn the reason of failure.

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashFactory.sol#L97

There are 2 more instances of this issue:

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L106 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L106

6. USE NAMED RETURN VALUES FOR BETTER READABILITY AND UNDERSTANDING OF THE CODE

) external payable override onlyGuardian returns (bytes[] memory results) { 
	results = new bytes[](exCallData.length);

above code can be changed as follows for better readability and understanding

) external payable override onlyGuardian returns (bytes[] memory) { 
	bytes[] memory results = new bytes[](exCallData.length);
	...
	...
	return results;

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashFactory.sol#L123-L134

There are 2 more instances of this issue:

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L133-L144 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L133-L144

7. PROPER COMMENTING WILL INCREASE THE READABILITY AND UNDERSTANDING OF THE CODE.

// `_logic' is set to `address(cash)`.

Above comment should be changed as _logic' is set to address(CashKYCSender)` https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L34

// Following the above mentioned deployment, the address of the CashFactory contract will:

Above comment should be changes as Following the above mentioned deployment, the address of the CashKYCSenderFactory contract will: https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L36

// `_logic' is set to `address(cash)`.

Above comment should be changed as _logic' is set to address(CashKYCSenderReceiver)` https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L34

// Following the above mentioned deployment, the address of the CashFactory contract will:

Above comment should be changes as Following the above mentioned deployment, the address of the CashKYCSenderReceiver contract will: https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L36

// @param repayAmount The amount to repay, or -1 for the full outstanding amount

Above comment says -1 can be given as an input. But repayAmount is declared as uint and can not accept negative values. Should change the comment accordingly. https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L107 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L118 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CErc20.sol#L107 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CErc20.sol#L118

// @param addAmount The amount fo underlying token to add as reserves

The fo of above comment should be changed to of https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cCash/CCash.sol#L165 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/tokens/cToken/CErc20.sol#L165

// @param chainlinkOracle cToken contract address

In the above comment the parameter represents chainlink oracle address and not the cToken address. https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/OndoPriceOracleV2.sol#L249

// Mapping from address to amount of CASH address burned

Above comment should be changed as follows: // Mapping from address to amount of CASH amount burned https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L74

// 1) The total supply at the end of the epoch is stored

The condition required in the above comment is not implemented inside the function. https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L570-L587

8. CONSTANTS SHOULD BE DECLARED IN UPPERCASE LETTERS FOR BETTER READABILITY OF THE CODE.

uint public constant blocksPerYear = 2628000;

Above constant should be declared as:

uint public constant BLOCKS_PER_YEAR = 2628000;

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/JumpRateModelV2.sol#L29

There are 10 more instances of this issue

9. USE DATA TYPES WITH EXPLICIT DATA SIZE, WHEN DECLARING VARIABLES FOR BETTER READABILITY OF THE CODE.

uint public multiplierPerBlock;

Above variable can be declared as uint256 as follows:

uint256 public multiplierPerBlock;

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/JumpRateModelV2.sol#L34

There are multiple occurences of this issue in the code base.

10. USE `SafeMathLib.sol` FOR SECURED MATH OPERATIONS INVOLVING `uint`.

  uint256 collateralAmountDue = (amountToDist * cashAmountReturned) / 
    quantityBurned;

The arithmetic functions of the SafeMathLib.sol can be used instead. https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L755-L756

11. CHECK CONTRACT EXISTENCE WHEN USING LOW LEVEL `call` FUNCTION TO SEND ETHER.

  (bool success, bytes memory ret) = address(exCallData[i].target).call{
    value: exCallData[i].value
  }(exCallData[i].data);

Low level call function will return true even if the destination contract does not exist. Hence use EXTCODESIZE low level command to check the contract existence at the destination address before transfering ETH

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L962-L964

#0 - c4-judge
2023-01-23T14:56:27Z

trust1995 marked the issue as grade-a

Ondo Finance contest - Udsen's results

Institutional-Grade Finance. On-Chain. For Everyone.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-02] QA Report - $304.58

Findings Information

Awards

Labels

External Links

1. USE NAMED IMPORTS INSTEAD OF PLAIN `IMPORT ‘FILE.SOL’

2. NATSPEC IS MISSING

3. NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

4. MISSING address(0) CHECKS FOR THE FOLLOWING ADDRESS INPUTS TO THE CONSTRUCTORS AND FUNCTIONS.

5.assert() FUNCTION IS USED WITHOUT DESCRIPTION MESSAGE FOR THE REVERT.

6. USE NAMED RETURN VALUES FOR BETTER READABILITY AND UNDERSTANDING OF THE CODE

7. PROPER COMMENTING WILL INCREASE THE READABILITY AND UNDERSTANDING OF THE CODE.

8. CONSTANTS SHOULD BE DECLARED IN UPPERCASE LETTERS FOR BETTER READABILITY OF THE CODE.

9. USE DATA TYPES WITH EXPLICIT DATA SIZE, WHEN DECLARING VARIABLES FOR BETTER READABILITY OF THE CODE.

10. USE SafeMathLib.sol FOR SECURED MATH OPERATIONS INVOLVING uint.

11. CHECK CONTRACT EXISTENCE WHEN USING LOW LEVEL call FUNCTION TO SEND ETHER.

#0 - c4-judge2023-01-23T14:56:27Z

4. MISSING `address(0)` CHECKS FOR THE FOLLOWING ADDRESS INPUTS TO THE CONSTRUCTORS AND FUNCTIONS.

5.`assert()` FUNCTION IS USED WITHOUT DESCRIPTION MESSAGE FOR THE REVERT.

10. USE `SafeMathLib.sol` FOR SECURED MATH OPERATIONS INVOLVING `uint`.

11. CHECK CONTRACT EXISTENCE WHEN USING LOW LEVEL `call` FUNCTION TO SEND ETHER.

#0 - c4-judge
2023-01-23T14:56:27Z