Ondo Finance contest - Viktor_Cortess's results

[N-01] USE A MORE RECENT VERSION OF SOLIDITY

All contracts

[N-02] CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USED TO IMMUTABLE RATHER THAN CONSTANT

There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. Constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

contracts\cash\token\Cash.sol

22: bytes32 public constant TRANSFER_ROLE = keccak256("TRANSFER_ROLE");

contracts\cash\token\CashKYCSender.sol

26: bytes32 public constant KYC_CONFIGURER_ROLE = keccak256("KYC_CONFIGURER_ROLE");

contracts\cash\token\CashKYCSenderReceiver.sol

26: bytes32 public constant KYC_CONFIGURER_ROLE = keccak256("KYC_CONFIGURER_ROLE");

contracts\cash\factory\CashFactory.sol contracts\cash\factory\CashKYCSenderFactory.sol contracts\cash\factory\CashKYCSenderReceiverFactory.sol

44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

contracts\cash\kyc\KYCRegistry.sol

31: bytes32 public constant _APPROVAL_TYPEHASH = keccak256( "KYCApproval(uint256 kycRequirementGroup,address user,uint256 deadline)" ); 36: bytes32 public constant REGISTRY_ADMIN = keccak256("REGISTRY_ADMIN");

contracts\cash\CashManager.sol

122:bytes32 public constant MANAGER_ADMIN = keccak256("MANAGER_ADMIN"); 123: bytes32 public constant PAUSER_ADMIN = keccak256("PAUSER_ADMIN"); 124: bytes32 public constant SETTER_ADMIN = keccak256("SETTER_ADMIN");

[N-03] THE NONREENTRANT MODIFIER SHOULD OCCUR BEFORE ALL OTHER MODIFIERS

contracts\cash\CashManager.sol

195: function requestMint( uint256 collateralAmountIn ) external override updateEpoch nonReentrant whenNotPaused checkKYC(msg.sender)

241: function claimMint( address user, uint256 epochToClaim ) external override updateEpoch nonReentrant whenNotPaused checkKYC(user)

662: function requestRedemption( uint256 amountCashToRedeem ) external override updateEpoch nonReentrant whenNotPaused checkKYC(msg.sender) {

949: function multiexcall( ExCallData[] calldata exCallData ) external payable override nonReentrant onlyRole(MANAGER_ADMIN) whenPaused returns (bytes[] memory results) {

[L-01] Nobody has access to address(0), so msg.sender == address(0) doesn’t make sense

contracts\lending\tokens\cToken\CTokenModified.sol

1090 : if (msg.sender != pendingAdmin || msg.sender == address(0)) { revert AcceptAdminPendingAdminCheck(); }

[L-02] ADD ZERO ADDRESS VALIDATION IN CONSTRUCTOR

Parameter used in constructor is used to initialize the state variable, error in these can lead to redeployment of contract.

contracts\cash\factory\CashFactory.sol contracts\cash\factory\CashKYCSenderFactory.sol contracts\cash\factory\CashKYCSenderReceiverFactory.sol

53: constructor(address _guardian) { guardian = _guardian; }

contracts\lending\JumpRateModelV2.sol 66: owner = owner_;

[L-03] USE OF ASSERT() INSTEAD OF REQUIRE()

Contracts use assert() instead of require() in multiple places. This causes a Panic error on failure and prevents the use of error strings.

contracts\cash\factory\CashFactory.sol

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

contracts\cash\factory\CashKYCSenderFactory.sol

107:assert(cashKYCSenderProxyAdmin.owner() == guardian);

contracts\cash\factory\CashKYCSenderReceiverFactory.sol

106: assert(cashKYCSenderReceiverProxyAdmin.owner() == guardian);

[L-04] UNSPECIFIC COMPILER VERSION PRAGMA

Avoid floating pragmas for non-library contracts. While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain. It is recommended to pin to a concrete compiler version.

contracts\lending\tokens\cCash\CCashDelegate.sol => pragma solidity ^0.8.10; contracts\lending\tokens\cToken\CTokenDelegate.sol => pragma solidity ^0.8.10; contracts\lending\JumpRateModelV2.sol => pragma solidity ^0.5.16; contracts\lending\tokens\cCash\CCash.sol => pragma solidity ^0.8.10; contracts\lending\tokens\cToken\CErc20.sol => pragma solidity ^0.8.10; contracts\lending\tokens\cCash\CTokenInterfacesModifiedCash.sol => pragma solidity ^0.8.10; contracts\lending\tokens\cToken\CTokenInterfacesModified.sol => pragma solidity ^0.8.10; contracts\lending\tokens\cErc20ModifiedDelegator.sol pragma solidity ^0.5.12; contracts\lending\tokens\cCash\CTokenCash.sol pragma solidity ^0.8.10; contracts\lending\tokens\cToken\CTokenModified.sol pragma solidity ^0.8.10;

[L-05] NON-USAGE OF SPECIFIC IMPORTS

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly.

Context: all contracts

[L-06] In CashManager.sol function setFeeRecipient(), function setAssetRecipient and function setAssetSender() don’t check zero address values.

There are checks of zero values for variables assetRecipient, feeRecipient and assetSender in the constructor, but they are not checked in functions that set them later.

Accidental input of address(0) can lead to loss of funds sending assets or fees to a 0 address.

contracts\cash\CashManager.sol

452: function setFeeRecipient( address _feeRecipient ) external override onlyRole(MANAGER_ADMIN) { address oldFeeRecipient = feeRecipient; feeRecipient = _feeRecipient; emit FeeRecipientSet(oldFeeRecipient, _feeRecipient); }

465: function setAssetRecipient( address _assetRecipient ) external override onlyRole(MANAGER_ADMIN) { address oldAssetRecipient = assetRecipient; assetRecipient = _assetRecipient; emit AssetRecipientSet(oldAssetRecipient, _assetRecipient); }

803: function setAssetSender( address newAssetSender ) external onlyRole(MANAGER_ADMIN) { address oldAssetSender = assetSender; assetSender = newAssetSender; emit AssetSenderSet(oldAssetSender, newAssetSender); }

[L-07] DUPLICATED REQUIRE()/REVERT() CHECKS SHOULD BE REFACTORED TO A MODIFIER OR FUNCTION

There are 3 places in the code with the same require(msg.sender == admin, …) It could be refactored to an error/modifier/function with message: “Only Admin”.

contracts\lending\tokens\cToken\CTokenModified.sol

44: require(msg.sender == admin, "only admin may initialize the market");

1360: require(msg.sender == admin, "Only admin can set KYC requirement group");

1382: require(msg.sender == admin, "Only admin can set KYC registry");

[L-08] WE CAN USE FUNCTIONS INSTEAD OF MODIFIERS WITH THESE FUNCTIONS INSIDE

Also, modifiers make code more elegant but cost more than normal functions.

contracts\cash\CashManager.sol

modifier updateEpoch() { transitionEpoch(); _; }

modifier checkKYC(address account) { _checkKYC(account); _; }

modifier updateEpoch() { transitionEpoch(); _; }

[G-01] SPLITTING REQUIRE() STATEMENTS THAT USE && SAVES GAS

Instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement (saving 3 gas per &):

contracts\lending\OndoPriceOracleV2.sol

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

contracts\lending\tokens\cCash\CTokenCash.sol

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

contracts\lending\tokens\cToken\CTokenModified.sol

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

[G-02] USE NAMED RETURNS WHERE APPROPRIATE

contracts\lending\OndoPriceOracleV2.sol

94: external view override returns (uint256)

contracts\lending\tokens\cCash\CTokenCash.sol

771: internal returns (uint)

contracts\lending\tokens\cToken\CTokenModified.sol

771: internal returns (uint)

[G-03] REQUIRE() OR REVERT() STATEMENTS THAT CHECK INPUT ARGUMENTS SHOULD BE AT THE TOP OF THE FUNCTION

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting gas in a function that may ultimately revert in the unhappy case.

contracts\cash\token\CashKYCSenderReceiver.sol

56: function _beforeTokenTransfer( address from, address to, uint256 amount ) internal override { super._beforeTokenTransfer(from, to, amount); require( _getKYCStatus(_msgSender()), "CashKYCSenderReceiver: must be KYC'd to initiate transfer" );

if (from != address(0)) {
  // Only check KYC if not minting
  require(
    _getKYCStatus(from),
    "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens"
  );
}

if (to != address(0)) {
  // Only check KYC if not burning
  require(
    _getKYCStatus(to),
    "CashKYCSenderReceiver: `to` address must be KYC'd to receive tokens"
  );
}

}`

[G-04] State variables should be cached in stack variables rather than re-reading them from storage

contracts\lending\tokens\cToken\CTokenModified.sol

borrowSnapshot.principal in function borrowBalanceStoredInternal():

323: if (borrowSnapshot.principal == 0) {

330: ' uint principalTimesIndex = borrowSnapshot.principal * borrowIndex;`

[G-05] Parameters of function should be checked first to avoid waste of gas:

contracts\lending\tokens\cToken\CTokenModified.sol

In this function we should check msg.sender first, then

if (reduceAmount > totalReserves) { revert ReduceReservesCashValidation(); }

1256 : function _reduceReservesFresh(uint reduceAmount) internal returns (uint) { uint totalReservesNew; if (msg.sender != admin) { revert ReduceReservesAdminCheck(); }

if (accrualBlockNumber != getBlockNumber()) { revert ReduceReservesFreshCheck(); }

if (getCashPrior() < reduceAmount) {
  revert ReduceReservesCashNotAvailable();
}

if (reduceAmount > totalReserves) {
  revert ReduceReservesCashValidation();
}`

The same in function _setReserveFactorFresh()

After checking msg.sender we check newReserveFactorMantissa:

` if (newReserveFactorMantissa > reserveFactorMaxMantissa) {
  revert SetReserveFactorBoundsCheck();
}`

1154: ` function _setReserveFactorFresh( uint newReserveFactorMantissa ) internal returns (uint) { if (msg.sender != admin) { revert SetReserveFactorAdminCheck(); }

if (accrualBlockNumber != getBlockNumber()) {
  revert SetReserveFactorFreshCheck();
}

if (newReserveFactorMantissa > reserveFactorMaxMantissa) {
  revert SetReserveFactorBoundsCheck();
}`

[G-06] pendingAdmin = payable(address(0)); consumes more gas than pendingAdmin = address(0);

contracts\lending\tokens\cToken\CTokenModified.sol

When you use pendingAdmin = payable(address(0)); you are converting the address(0) to a payable address, which can be used to receive ether, but this is not necessary in this case because address(0) is already a valid address, so no conversion is needed.

On the other hand, when you use pendingAdmin = address(0); you are simply assigning the address(0) to the variable, and this is less expensive in terms of gas usage because it doesn't require any additional computation.

Therefore, using pendingAdmin = address(0); is more efficient in terms of gas usage, as it doesn't require the extra step of type casting the address to a payable address.

1101: // Clear the pending value pendingAdmin = payable(address(0));