Canto contest - c3phas's results

FINDINGS

TYPO

File: BaseV1-periphery.sol line 463

        require(token.code.length > 0, "token code length faialure");

faialure instead of failure

Open TODO left in the code

File: Comptroller.sol line 1232

        // TODO: Don't distribute supplier COMP if the user is not in the supplier market.

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File:Proposal-Store.sol line 3

pragma solidity ^0.8.0;

File:WETH.sol line 1 File: Comptroller.sol line 2 File: CNote.sol line 1

FINDINGS

Cache the length of arrays in loops ~6 gas per iteration

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: BaseV1-periphery.sol line 132-136

    function getAmountsOut(uint amountIn, route[] memory routes) public view returns (uint[] memory amounts) {
        require(routes.length >= 1, 'BaseV1Router: INVALID_PATH');
        amounts = new uint[](routes.length+1);
        amounts[0] = amountIn;
        for (uint i = 0; i < routes.length; i++) {

The above should be modified to

    function getAmountsOut(uint amountIn, route[] memory routes) public view returns (uint[] memory amounts) {
        require(routes.length >= 1, 'BaseV1Router: INVALID_PATH');
				uint length = routes.length;
        amounts = new uint[](length+1);
        amounts[0] = amountIn;
        for (uint i = 0; i < length; i++) {

Other instances File:BaseV1-periphery.sol line 362

        for (uint i = 0; i < routes.length; i++) {

File: Comptroller.sol line 735

        for (uint i = 0; i < assets.length; i++) {

File: Comptroller.sol line 959

        for (uint i = 0; i < allMarkets.length; i ++) {

File: Comptroller.sol line 1106

For loops Increments can be unchecked to save gas.

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

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File:BaseV1-periphery.sol line 136-141

        for (uint i = 0; i < routes.length; i++) {
            address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable);
            if (IBaseV1Factory(factory).isPair(pair)) {
                amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from);
            }
        }

The above should be modified to

        for (uint i = 0; i < routes.length;) {
            address pair = pairFor(routes[i].from, routes[i].to, routes[i].stable);
            if (IBaseV1Factory(factory).isPair(pair)) {
                amounts[i+1] = IBaseV1Pair(pair).getAmountOut(amounts[i], routes[i].from);
            }
			   unchecked {
			   		i++;
			   }

Other instances File:BaseV1-periphery.sol line 362

        for (uint i = 0; i < routes.length; i++) {

File: BaseV1-core.sol line 207

        for (uint i = 0; i < _prices.length; i++) {

File: Comptroller.sol line 126

        for (uint i = 0; i < len; i++) {

File: Comptroller.sol line 206 File: Comptroller.sol line 735 File: Comptroller.sol line 959 File: Comptroller.sol line 1005

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

File:BaseV1-periphery.sol line 136

        for (uint i = 0; i < routes.length; i++) {

The above should be modified to

        for (uint i = 0; i < routes.length; ++i) {

Other instances File:BaseV1-periphery.sol line 362

        for (uint i = 0; i < routes.length; i++) {

File: BaseV1-core.sol line 207

        for (uint i = 0; i < _prices.length; i++) {

File: GovernorBravoDelegate.sol line 68

File: Comptroller.sol line 126 File: Comptroller.sol line 206 File: Comptroller.sol line 735 File: Comptroller.sol line 959 File: Comptroller.sol line 1005

use shorter revert strings(less than 32 bytes)

You can (and should) attach error reason strings along with require statements to make it easier to understand why a contract call reverted. These strings, however, take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

File:BaseV1-periphery.sol line 86

        require(tokenA != tokenB, 'BaseV1Router: IDENTICAL_ADDRESSES');

File:BaseV1-periphery.sol line 105

        require(reserveA > 0 && reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY');

File:BaseV1-periphery.sol line 223

                require(amountBOptimal >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT');

File:BaseV1-periphery.sol line 228

                require(amountAOptimal >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT');

File:BaseV1-periphery.sol line 295-296

        require(amountA >= amountAMin, 'BaseV1Router: INSUFFICIENT_A_AMOUNT');
        require(amountB >= amountBMin, 'BaseV1Router: INSUFFICIENT_B_AMOUNT');

File:BaseV1-periphery.sol line 387 File:BaseV1-periphery.sol line 402 File:BaseV1-periphery.sol line 417 File:BaseV1-periphery.sol line 430 File:BaseV1-periphery.sol line 452 File: WETH.sol line 29 File: WETH.sol line 96-97 File:GovernorBravoDelegate.sol line 25-27 File:GovernorBravoDelegate.sol line 46-47 File:GovernorBravoDelegate.sol line 78 File:GovernorBravoDelegate.sol line 87 File:GovernorBravoDelegate.sol line 146 File: GovernorBravoDelegate.sol line 164 File: Comptroller.sol line 491 File: Comptroller.sol line 998 File: CNote.sol line 16 File:CNote.sol line 43 & 45 File: CNote.sol line 54 File: CNote.sol line 77 File: CNote.sol line 114 File: CNote.sol line 130 File: CNote.sol line 146 File: CNote.sol line 198 File: CNote.sol line 214 File: CNote.sol line 264 File: CNote.sol line 310 File: CNote.sol line 330

Comparisons: != is more efficient than > in require

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs:

I suggest changing > 0 with != 0 here:

File:BaseV1-periphery.sol line 104

        require(amountA > 0, 'BaseV1Router: INSUFFICIENT_AMOUNT');

Splitting require() statements that use && saves gas - 8 gas per &&

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File:BaseV1-periphery.sol line 105

        require(reserveA > 0 && reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY');

The above should be modified to

        require(reserveA > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY' );
		     require(reserveB > 0, 'BaseV1Router: INSUFFICIENT_LIQUIDITY');

Other instances to modify

File: BaseV1-periphery.sol line 459

        require(success && (data.length == 0 || abi.decode(data, (bool))));

File: BaseV1-periphery.sol line 466

        require(success && (data.length == 0 || abi.decode(data, (bool))), "failing here");

File: BaseV1-core.sol line 272

        require(amount0 > 0 && amount1 > 0, 'ILB'); // BaseV1: INSUFFICIENT_LIQUIDITY_BURNED

File: BaseV1-core.sol line 288

        require(amount0Out < _reserve0 && amount1Out < _reserve1, 'IL'); // BaseV1: INSUFFICIENT_LIQUIDITY

File: BaseV1-core.sol line 294

        require(to != _token0 && to != _token1, 'IT'); // BaseV1: INVALID_TO

File: BaseV1-core.sol line 431

        require(recoveredAddress != address(0) && recoveredAddress == owner, 'BaseV1: INVALID_SIGNATURE');

File: BaseV1-core.sol line 468

        require(success && (data.length == 0 || abi.decode(data, (bool))));

File: GovernorBravoDelegate.sol line 42

File: GovernorBravoDelegate.sol line 115

File: GovernorBravoDelegate.sol line 132-133

File: GovernorBravoDelegate.sol line 164

File:Comptroller.sol line 1003

Proof The following tests were carried out in remix with both optimization turned on and off

    require ( a > 1 && a < 5, "Initialized");
    return  a + 2;
}

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

    require (a > 1 ,"Initialized");
    require (a < 5 , "Initialized");
    return a + 2;
}

Execution cost 21609 with optimization and split require 21968 without optimization and using split require'

No need to initialize variables with their default values

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas. It costs more gas to initialize variables to zero than to let the default of zero be applied

File:BaseV1-periphery.sol line 158

        uint _totalSupply = 0;

File: BaseV1-core.sol line 223-224

        uint nextIndex = 0;
        uint index = 0;

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible , some gas can be saved by using an unchecked block

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

File: Comptroller.sol line 1116

                uint accountReceivable = amountToSubtract - currentAccrual; // Underflow safe since amountToSubtract > currentAccrual

The above line cannot underflow due to the check on line 1114