Canto v2 contest - c3phas's results

Non critical

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: TreasuryDelegate.sol line 1

pragma solidity ^0.8.10;

Almost all contracts are using a floating pragma.

Missing checks for address(0x0) when assigning values to address state variables( Immutable addresses should be 0-checked)

File: GovernorAlpha.sol line 133

        guardian = guardian_;

Consider adding zero-address checks in the above.

require(newAddr != address(0));

Get rid of safeMath

The code base is using Solidity 0.8.0 which has safeMath integrated in the compiler. In addition, the codebase also utilizes OpenZepplin SafeMath library for arithmetic operations.Removing safeMath from the code base results in gas usage optimization and also clearer code.

File: NoteInterest.sol line 15

    using SafeMath for uint;

SafeMath is being imported and later used on some functions eg at line 139

        uint deltaBlocks = blockNumber.sub(lastUpdateBlock);

see consensys report for more information

Misleading comment

File: CNote.sol line 108

        return balanceAfter;   // underflow already checked above, just subtract

The comment says we need to subtract but in this case we just return balanceAfter

Unused named return

Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those. File: BaseV1-core.sol line 206

    function quote(address tokenIn, uint amountIn, uint granularity) external view returns (uint amountOut) {
        console.log("tokenIn: ", tokenIn);
        uint [] memory _prices = sample(tokenIn, amountIn, granularity, 1);
        uint priceAverageCumulative;
        for (uint i = 0; i < _prices.length; i++) {
            priceAverageCumulative += _prices[i];
        }
        return priceAverageCumulative / granularity;
    }

FINDINGS

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: CToken.sol line 82

        uint startingAllowance = 0;

Other instances to modify File: BaseV1-core.sol line 48

    uint public totalSupply = 0;

File: BaseV1-core.sol line 226-227

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 (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

File: GovernorBravoDelegate.sol line 180-183

    function sub256(uint256 a, uint256 b) internal pure returns (uint) {
        require(b <= a, "subtraction underflow");
        return a - b;
    }

The operation a-b cannot underflow due to the check on line 181 which ensures that the value of a is greater than b before performing the operation

The above can be modified like

    function sub256(uint256 a, uint256 b) internal pure returns (uint) {
        require(b <= a, "subtraction underflow");
				 unchecked{
        	return a - b;
			}
    }

File: Comp.sol line 293

        return a - b;

File: CToken.sol line 1064

        totalReservesNew = totalReserves - reduceAmount;

Using unchecked blocks to save gas - Increments in for loop can be unchecked (saves around 30-40 gas per loop iteration)

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: GovernorAlpha.sol line 181

        for (uint i = 0; i < proposal.targets.length; i++) {
            _queueOrRevert(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], eta);
        }

The above should be modified to:

        for (uint i = 0; i < proposal.targets.length; ) {
            _queueOrRevert(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], eta);
							unchecked{
							  ++i;
							}
        }

Other Instances to modify FIle: GovernorAlpha.sol line 197 FIle: GovernorAlpha.sol line 211

File: GovernorBravoDelegate.sol line 66 File: GovernorBravoDelegate.sol line 88

see resource

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-core.sol line 210

        for (uint i = 0; i < _prices.length; i++) {
            priceAverageCumulative += _prices[i];
        }

The above should be modified to

	uint length = _prices.length;
        for (uint i = 0; i < length; i++) {
            priceAverageCumulative += _prices[i];
        }

++i costs less gas compared to i++ or i += 1 (saves ~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.

Instances include: File: GovernorAlpha.sol line 181

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

FIle: GovernorAlpha.sol line 152 FIle: GovernorAlpha.sol line 197 FIle: GovernorAlpha.sol line 211

File: GovernorBravoDelegate.sol line 66 File: GovernorBravoDelegate.sol line 88

File: BaseV1-core.sol line 210

Use Shift Right/Left instead of Division/Multiplication

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

relevant source

File:ERC20DirectBalanceManipulation.sol line 18

    uint256 half = amount / 2;

The above should be modified to

    uint256 half = amount >> 1;

Comparisons: != is more efficient than > in require statement (6 gas less)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Note: This only applies in a require statement

I suggest changing > 0 with != 0 here:

File: BaseV1-core.sol line 289

        require(amount0Out > 0 || amount1Out > 0, 'IOA'); // BaseV1: INSUFFICIENT_OUTPUT_AMOUNT

File: BaseV1-periphery.sol line 122 File: BaseV1-periphery.sol line 123

Splitting require() statements that use && saves gas - (saves around 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: GovernorAlpha.sol line 138

        require(targets.length == values.length && targets.length == signatures.length && targets.length == calldatas.length, "GovernorAlpha::propose: proposal function information arity mismatch");

File: GovernorAlpha.sol line 228

File: GovernorBravoDelegate.sol line 42

File: CToken.sol line 34

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

function multiple (uint a) public pure returns (uint){
	require ( a > 1 && a < 5, "Initialized");
	return  a + 2;
}

Execution cost 21617 with optimization and using &&

After splitting the require statement

function multiple(uint a) public pure returns (uint){
	require (a > 1 ,"Initialized");
	require (a < 5 , "Initialized");
	return a + 2;
}

Execution cost 21609 with optimization and split require

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

consequences:

• Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

• Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )
  

File: TreasuryDelegate.sol line 12

    bytes32 constant cantoDenom = keccak256(bytes("CANTO"));

File: TreasuryDelegate.sol line 13

    bytes32 constant noteDenom = keccak256(bytes("NOTE")); //cache hashed values to reduce unnecessary gas costs

File: Comp.sol line 39

    bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");

File: Comp.sol line 42

    bytes32 public constant DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");

File: GovernorAlpha.sol line 110 File: GovernorAlpha.sol line 113

File: GovernorBravoDelegate.sol line 15 File: GovernorBravoDelegate.sol line 18

File: ERC20MinterBurnerDecimals.sol line 27 File: ERC20MinterBurnerDecimals.sol line 28 File: ERC20MinterBurnerDecimals.sol line 29