Ondo Finance contest - 0x1f8b's results

• Low
  • 1. Integer overflow by unsafe casting
  • 2. Mixing and Outdated compiler
• Non critical
  • 3. Wong naming
  • 4. Lack of checks supportsInterface
  • 5. Avoid hardcoded values
  • 6. Use abstract for base contracts

Low

1. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

return uint256(answer) * chainlinkInfo.scaleFactor;

Affected source code:

• OndoPriceOracleV2.sol:300

2. Mixing and Outdated compiler

The pragma version used are:

pragma solidity ^0.5.12;
pragma solidity ^0.5.16;
pragma solidity 0.8.16;

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

---

Non critical

3. Wong naming

Public methods should not use '_' before the name. We have an example in CTokenModified.sol:1063, a different naming should be established, removing the '_'.

Affected source code:

• CTokenModified.sol:1063
• CTokenModified.sol:1088
• CTokenModified.sol:1115
• CTokenModified.sol:1141
• CTokenModified.sol:1303
• CTokenCash.sol:1060
• CTokenCash.sol:1085
• CTokenCash.sol:1112
• CTokenCash.sol:1138
• CTokenCash.sol:1239
• CErc20.sol:168
• CErc20.sol:268
• CCash.sol:168
• CCash.sol:268
• cErc20ModifiedDelegator.sol:720
• cErc20ModifiedDelegator.sol:1099
• cErc20ModifiedDelegator.sol:1113
• cErc20ModifiedDelegator.sol:1127
• cErc20ModifiedDelegator.sol:1144
• cErc20ModifiedDelegator.sol:1156
• cErc20ModifiedDelegator.sol:1168
• cErc20ModifiedDelegator.sol:1181

4. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• KYCRegistry.sol:57

5. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

• cErc20ModifiedDelegator.sol:351

6. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• CTokenInterfacesModifiedCash.sol:13
• CTokenInterfacesModifiedCash.sol:143
• CTokenInterfacesModifiedCash.sol:383
• CTokenInterfacesModified.sol:11
• CTokenInterfacesModified.sol:141
• CTokenInterfacesModified.sol:381
• cErc20ModifiedDelegator.sol:327
• cErc20ModifiedDelegator.sol:364
• cErc20ModifiedDelegator.sol:569
• cErc20ModifiedDelegator.sol:576
• cErc20ModifiedDelegator.sol:605
• cErc20ModifiedDelegator.sol:612
• cErc20ModifiedDelegator.sol:631

• Gas
  • 1. Optimize sanction List
  • 2. Remove return
  • 3. Use a recent solidity version
  • 4. Use require instead of assert
  • 5. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 8 = 104
  • 6. delete optimization
    • Total gas saved: 5 * 4 = 20
  • 7. Remove natspec complaints
  • 8. Optimize CErc20.initialize and CCash.initialize
  • 9. Remove SafeMath from JumpRateModelV2

Gas

1. Optimize sanction List

If the ISanctionsList interface had an isSanctionedAny method, you could check multiple addresses at once without jumping into the contract multiple times.

    require(!sanctionsList.isSanctioned(spender), "Spender is sanctioned");
    require(!sanctionsList.isSanctioned(src), "Source is sanctioned");
    require(!sanctionsList.isSanctioned(dst), "Destination is sanctioned");

Affected source code:

• CTokenModified.sol:97-99

2. Remove return

It always returns NO_ERROR, it is better not to return or check the value.

Affected source code:

• CTokenModified.sol:143
• CTokenModified.sol:401
• CTokenModified.sol:471
• CTokenCash.sol:401
• CTokenCash.sol:471
• CTokenCash.sol:1077
• CTokenCash.sol:1104
• CTokenCash.sol:1130
• CTokenCash.sol:1291
• CTokenCash.sol:1345
• CErc20.sol:68
• CErc20.sol:79
• CErc20.sol:92
• CErc20.sol:102
• CErc20.sol:112
• CErc20.sol:126
• CErc20.sol:143
• CCash.sol:68
• CCash.sol:79
• CCash.sol:92
• CCash.sol:102
• CCash.sol:112
• CCash.sol:126
• CCash.sol:143

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/

3. Use a recent solidity version

Using an updated version of solidity you can achieve significant gas savings, like the one mentioned above:

• With at least 0.8.10 to skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist.

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

Affected source code:

• ERC721Upgradeable.sol:479

4. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• CashFactory.sol:97
• CashKYCSenderFactory.sol:106
• CashKYCSenderReceiverFactory.sol:106

5. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint256 private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
Uint256 private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• CashManager.sol:221
• CashManager.sol:582
• CashManager.sol:630
• CashManager.sol:649
• CashManager.sol:678-680
• CashManager.sol:681
• CashManager.sol:865
• CashManager.sol:867

Total gas saved: 13 * 8 = 104

6. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• CashManager.sol:259
• CashManager.sol:754
• CashManager.sol:790
• KYCRegistry.sol:181

Total gas saved: 5 * 4 = 20

7. Remove natspec complaints

This is not the best way to avoid some warnings and it will increase the contract sizes. Remove natspec complaints in order to save gas.

Affected source code:

• CCashDelegate.sol:23-28
• CCashDelegate.sol:41-43
• CTokenDelegate.sol:23-28
• CTokenDelegate.sol:41-43

8. Optimize CErc20.initialize and CCash.initialize

It's better to use the local variable instead of the storage one.

function initialize(
    address underlying_,
    ComptrollerInterface comptroller_,
    InterestRateModel interestRateModel_,
    uint initialExchangeRateMantissa_,
    string memory name_,
    string memory symbol_,
    uint8 decimals_,
    address kycRegistry_,
    uint kycRequirementGroup_
  ) public {
    ...

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

Affected source code:

• CErc20.sol:54-55
• CCash.sol:54-55

9. Remove SafeMath from JumpRateModelV2

The code use SafeMath, but it will be compiled in 0.8.X version of solidity, so the overflows checks will be added, and also will be checked by code, wasting resources and gas.

Affected source code:

• JumpRateModelV2.sol:4