Ondo Finance contest - tsvetanovv's results

[QA-01] Use latest Solidity version

Solidity pragma versioning should be upgraded to latest available version.

---

[QA-02] Use stable pragma statement

Using a floating pragma ^0.8.10 statement is discouraged as code can compile to different bytecodes with different compiler versions. Use a stable pragma statement to get a deterministic bytecode.

CCashDelegate.sol:
pragma solidity ^0.8.10;

CTokenDelegate.sol:
pragma solidity ^0.8.10;

JumpRateModelV2.sol:
pragma solidity ^0.5.16;

CCash.sol:
pragma solidity ^0.8.10;

CErc20.sol:
pragma solidity ^0.8.10;

CTokenInterfacesModifiedCash.sol:
pragma solidity ^0.8.10;

CTokenInterfacesModified.sol:
pragma solidity ^0.8.10;

cErc20ModifiedDelegator.sol:
pragma solidity ^0.5.12;

CTokenCash.sol:
pragma solidity ^0.8.10;

CTokenModified.sol::
pragma solidity ^0.8.10;

---

[QA-03] Different pragma directives are used

Use one Solidity version on each contract.

---

[QA-04] USE NAMED IMPORTS INSTEAD OF PLAIN `IMPORT ‘FILE.SOL’

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

Cash.sol:
18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol";

CCashDelegate.sol:
4: import "./CCash.sol";

CTokenDelegate.sol:
4: import "./CErc20.sol";

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

OndoPriceOracle.sol:
17: import "./IOndoPriceOracle.sol";
18: import "contracts/lending/compound/Ownable.sol";

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

CashFactory.sol:
19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol";
20: import "contracts/cash/Proxy.sol";
21: import "contracts/cash/token/Cash.sol";
22: import "contracts/cash/interfaces/IMulticall.sol";

CashKYCSenderFactory.sol:
19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol";
20: import "contracts/cash/Proxy.sol";
21: import "contracts/cash/token/CashKYCSender.sol";
22: import "contracts/cash/interfaces/IMulticall.sol";

CashKYCSenderReceiverFactory.sol:
19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol";
20: import "contracts/cash/Proxy.sol";
21: import "contracts/cash/token/CashKYCSenderReceiver.sol";
22: import "contracts/cash/interfaces/IMulticall.sol";

JumpRateModelV2.sol:
3: import "./compound/InterestRateModel.sol";
4: import "./compound/SafeMath.sol";

KYCRegistry.sol:
18: import "contracts/cash/interfaces/IKYCRegistry.sol";
19: import "contracts/cash/external/openzeppelin/contracts/access/AccessControlEnumerable.sol";
20: import "contracts/cash/external/chainalysis/ISanctionsList.sol";
21: import "contracts/cash/external/openzeppelin/contracts/utils/cryptography/EIP712.sol";
22: import "contracts/cash/external/openzeppelin/contracts/utils/cryptography/ECDSA.sol";

CCash.sol:
4: import "./CTokenCash.sol":

CErc20.sol:
4: import "./CTokenModified.sol";

OndoPriceOracleV2.sol:
17: import "./IOndoPriceOracleV2.sol";
18: import "contracts/cash/external/openzeppelin/contracts/access/Ownable.sol";
19: import "contracts/lending/chainlink/AggregatorV3Interface.sol";

CTokenInterfacesModifiedCash.sol:
4: import "contracts/lending/tokens/cErc20Delegate/ComptrollerInterface.sol";
5: import "contracts/lending/tokens/cErc20Delegate/InterestRateModel.sol";
6: import "contracts/lending/tokens/cErc20Delegate/EIP20NonStandardInterface.sol";
7: import "contracts/lending/tokens/cErc20Delegate/ErrorReporter.sol";

CTokenInterfacesModified.sol:
4: import "contracts/lending/tokens/cErc20Delegate/ComptrollerInterface.sol";
5: import "contracts/lending/tokens/cErc20Delegate/InterestRateModel.sol";
6: import "contracts/lending/tokens/cErc20Delegate/EIP20NonStandardInterface.sol";
7: import "contracts/lending/tokens/cErc20Delegate/ErrorReporter.sol";

CashManager.sol:
17: import "contracts/cash/interfaces/ICashManager.sol";
18: import "contracts/cash/interfaces/IMulticall.sol";
19: import "contracts/cash/token/Cash.sol";
20: import "contracts/cash/kyc/KYCRegistryClientConstructable.sol";
21: import "contracts/cash/external/openzeppelin/contracts/security/Pausable.sol";
22: import "contracts/cash/external/openzeppelin/contracts/token/IERC20.sol";
23: import "contracts/cash/external/openzeppelin/contracts/token/IERC20Metadata.sol";
24: import "contracts/cash/external/openzeppelin/contracts/token/SafeERC20.sol";
25: import "contracts/cash/external/openzeppelin/contracts/access/AccessControlEnumerable.sol";
26: import "contracts/cash/external/openzeppelin/contracts/security/ReentrancyGuard.sol";

KYCRegistryClientConstructable.sol:
17: import "contracts/cash/kyc/KYCRegistryClient.sol";
21: import "contracts/cash/kyc/KYCRegistryClient.sol";

KYCRegistryClient.sol:
17: import "contracts/cash/interfaces/IKYCRegistry.sol";
18: import "contracts/cash/interfaces/IKYCRegistryClient.sol";

KYCRegistryClientInitializable.sol:
17: import "contracts/cash/kyc/KYCRegistryClient.sol";
18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/proxy/Initializable.sol";

CTokenCash.sol:
4: import "../cErc20Delegate/ComptrollerInterface.sol";
5: import "../cErc20Delegate/ErrorReporter.sol";
6: import "../cErc20Delegate/EIP20Interface.sol";
7: import "../cErc20Delegate/InterestRateModel.sol";
8: import "../cErc20Delegate/ExponentialNoError.sol";
9: import "./CTokenInterfacesModifiedCash.sol";

CTokenModified.sol:
4: import "../cErc20Delegate/ComptrollerInterface.sol";
5: import "../cErc20Delegate/ErrorReporter.sol";
6: import "../cErc20Delegate/EIP20Interface.sol";
7: import "../cErc20Delegate/InterestRateModel.sol";
8: import "../cErc20Delegate/ExponentialNoError.sol";
9: import "./CTokenInterfacesModified.sol";

IKYCRegistryClient.sol:
18: import "contracts/cash/interfaces/IKYCRegistry.sol";

---

[QA-05] STOP USING V != 27 && V != 28 OR V == 27 || V == 28

KYCRegistry.sol:
87: require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");

You can see this for reference.

---

[QA-06] 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.

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

CashKYCSender.sol:
26: bytes32 public constant KYC_CONFIGURER_ROLE =
27:    keccak256("KYC_CONFIGURER_ROLE");

CashKYCSenderReceiver.sol:
26:  bytes32 public constant KYC_CONFIGURER_ROLE =
27:    keccak256("KYC_CONFIGURER_ROLE");

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

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

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

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");

KYCRegistry.sol:
31: bytes32 public constant _APPROVAL_TYPEHASH =
36: bytes32 public constant REGISTRY_ADMIN = keccak256("REGISTRY_ADMIN");

---

[QA-07] Too many digits

Literals with many digits are difficult to read and review. Is better to separate the digits with _.

JumpRateModelV2.sol:
29: uint public constant blocksPerYear = 2628000;

---

[QA-08] Missing zero address validation

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

CCash.sol:
193: function doTransferIn(
236: function doTransferOut(
268: function _delegateCompLikeTo(address compLikeDelegatee) external {

CErc20.sol:
193: function doTransferIn(
    	address from,
236: function doTransferOut(
      	address payable to,
268: function _delegateCompLikeTo(address compLikeDelegatee) external {

OndoPriceOracleV2.sol:
130: function setPriceCap(
    	address fToken,
145: function setFTokenToOracleType(
    	address fToken,
182: function setOracle(address newOracle) external override onlyOwner {
194: function setFTokenToCToken(
    	address fToken,
233: function setFTokenToChainlinkOracle(
    	address fToken,
    	address newChainlinkOracle

CashManager.sol:
241: function claimMint(
    	address user,
336: function setPendingMintBalance(
    	address user,
452: function setFeeRecipient(
    	address _feeRecipient
465: function setAssetRecipient(
    	address _assetRecipient
907: function setKYCRegistry(
    	address _kycRegistry

Recommendation

Check that the address is not zero.

[GAS-01] STATE VARIABLES ONLY SET IN THE CONSTRUCTOR SHOULD BE DECLARED IMMUTABLE.

It allows setting contract-level variables at construction time which gets stored in code rather than storage.

JumpRateModelV2.sol
24: address public owner;

cErc20ModifiedDelegator.sol
218: ddress payable public admin;

---

[GAS-02] ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED()

Use abi.encodePacked() where possible to save gas.

KYCRegistry.sol:
93: bytes32 structHash = keccak256(
      abi.encode(_APPROVAL_TYPEHASH, kycRequirementGroup, user, deadline)

---

[GAS-03] Require() or revert() statements that check input arguments should be at the top of the function

CErc20.sol:
function sweepToken(EIP20NonStandardInterface token) external override {
    require(
      msg.sender == admin,
      "cErc20::sweepToken: only admin can sweep tokens"
    );
    require(
      address(token) != underlying,
      "cErc20::sweepToken: can not sweep underlying token"
    );
    uint256 balance = token.balanceOf(address(this));
    token.transfer(admin, balance);
  }

---

[GAS-04] USE NAMED RETURNS FOR LOCAL VARIABLES WHERE IT IS POSSIBLE

OndoPriceOracleV2.sol:
92: function getUnderlyingPrice(
    address fToken
  ) external view override returns (uint256) {
    uint256 price;

CTokenCash.sol:
771: ) internal returns (uint) {

---

[GAS-05] UNCHECKING ARITHMETICS OPERATIONS THAT CAN’T UNDERFLOW/OVERFLOW

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

CashFactory.sol:
127: for (uint256 i = 0; i < exCallData.length; ++i) {

CashKYCSenderFactory.sol:
137: for (uint256 i = 0; i < exCallData.length; ++i) {

CashKYCSenderReceiverFactory.sol:
137: for (uint256 i = 0; i < exCallData.length; ++i) {

KYCRegistry.sol:
163: for (uint256 i = 0; i < length; i++) {
180: for (uint256 i = 0; i < length; i++) {

CashManager.sol:
750: for (uint256 i = 0; i < size; ++i) {
786: for (uint256 i = 0; i < size; ++i) {
933: for (uint256 i = 0; i < size; ++i) {
961: for (uint256 i = 0; i < exCallData.length; ++i) {

---

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

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

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

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

---

[GAS-07] Should use arguments instead of state variable

CashManager.sol:
821: 	minimumRedeemAmount = newRedeemMinimum;
822:    emit MinimumRedeemAmountSet(oldRedeemMin, minimumRedeemAmount);

cErc20ModifiedDelegator.sol:
737: 	implementation = implementation_;
746: 	emit NewImplementation(oldImplementation, implementation);