Canto v2 contest - grGred's results

Execution layer for original work.

General Information

Platform: Code4rena

Start Date: 28/06/2022

Pot Size: $25,000 USDC

Total HM: 14

Participants: 50

Period: 4 days

Judge: GalloDaSballo

Total Solo HM: 7

Id: 141

League: ETH

Canto

Findings Distribution

Researcher Performance

Rank: 29/50

Findings: 2

Award: $65.03

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Canto

Findings Information

🌟 Selected for report: zzzitron

Also found by: 0v3rf10w, 0x1f8b, 0x29A, AlleyCat, Bnke0x0, Chom, Funen, JC, Lambda, Limbooo, Meera, Picodes, Sm4rty, TerrierLover, TomJ, __141345__, asutorufos, aysha, c3phas, cccz, defsec, fatherOfBlocks, grGred, hake, ignacio, ladboy233, mrpathfindr, oyc_109, rfa, sach1r0, samruna, slywaters, ynnad

Awards

43.2289 USDC - $43.23

Labels

bug

QA (Quality Assurance)

#0 - GalloDaSballo
2022-08-13T22:33:34Z

Space + Tabulation

Missed comment:

Findings Information

🌟 Selected for report: 0x1f8b

Also found by: 0x29A, 0xArshia, 0xKitsune, Bnke0x0, Chom, Fitraldys, Funen, JC, Lambda, Meera, Noah3o6, Picodes, RedOneN, Rohan16, Sm4rty, TerrierLover, TomJ, Tomio, Waze, ajtra, c3phas, cRat1st0s, defsec, durianSausage, fatherOfBlocks, grGred, hake, ladboy233, m_Rassska, mrpathfindr, oyc_109, rfa, ynnad

Awards

21.8032 USDC - $21.80

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Consider having short revert strings

Consider the following require statement:

// condition is boolean
// str is a string
require(condition, str)

The string str is split into 32-byte sized chunks and then stored in memory using mstore, then the memory offsets are provided to revert(offset, length). For chunks shorter than 32 bytes, and for low --optimize-runs value (usually even the default value of 200), instead of push32 val, where val is the 32 byte hexadecimal representation of the string with 0 padding on the least significant bits, the solidity compiler replaces it by shl(value, short-value)). Where short-value does not have any 0 padding. This saves the total bytes in the deploy code and therefore saves deploy time cost, at the expense of extra 6 gas during runtime. This means that shorter revert strings saves deploy time costs of the contract. Note that this kind of saving is not relevant for high values of --optimize-runs as push32 value will not be replaced by a shl(..., ...) equivalent by the Solidity compiler.

Going back, each 32 byte chunk of the string requires an extra mstore. That is, additional cost for mstore, memory expansion costs, as well as stack operations. Note that, this runtime cost is only relevant when the revert condition is met.

Overall, shorter revert strings can save deploy time as well as runtime costs.

Line references:

NoteInterest.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/NoteInterest.sol#L167 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/NoteInterest.sol#L180 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/NoteInterest.sol#L193

CNote.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/CNote.sol#L17 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/CNote.sol#L105

TreasuryDelegator.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Treasury/TreasuryDelegator.sol#L31 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Treasury/TreasuryDelegator.sol#L32 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Treasury/TreasuryDelegator.sol#L121

AccountantDelegate.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegate.sol#L87 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegate.sol#L101

AccountantDelegator.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegator.sol#L42 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegator.sol#L43 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegator.sol#L123

Note that if your contracts already allow using at least Solidity 0.8.4, then consider using Custom errors. This is more gas efficient, while allowing the developer to describe the errors in detail using NatSpec. A disadvantage to this approach is that, some tooling may not have proper support for this.

Note.sol https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Note.sol#L22 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Note.sol#L23

No need to add an extra variable since `twapMantissa` is used only once.

I recommend changing:

145:            uint twapMantissa = oracle.getUnderlyingPrice(cNote); // returns price as mantissa 
146:            //uint ir = (1 - twapMantissa).mul(adjusterCoefficient).add(baseRatePerYear);
147:            int diff = BASE - int(twapMantissa); //possible annoyance if 1e18 - twapMantissa > 2**255, differ

To:

146:            //uint ir = (1 - twapMantissa).mul(adjusterCoefficient).add(baseRatePerYear);
147:            int diff = BASE - int(oracle.getUnderlyingPrice(cNote)); //possible annoyance if 1e18 - twapMantissa > 2**255, differ

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/NoteInterest.sol#L145-L147

No need to do type conversion since an `accountant` is already an address

I recommend changing:

     require(address(accountant) == address(0));

To:

    require(accountant == address(0));

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Note.sol#L23

Use `calldata` instead of `memory` for function parameters

In some cases, having function arguments in calldata instead of memory is more optimal. When arguments are read-only on external functions, the data location should be calldata.

Example:

contract C {
    function add(uint[] memory arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length;) {
            sum += arr[i];
            unchecked { ++i; }
        }
    }
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
    function add(uint[] calldata arr) external returns (uint sum) {
        uint length = arr.length;
        for (uint i = 0; i < arr.length;) {
            sum += arr[i];
            unchecked { ++i; }
        }
    }
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause “unimplemented feature error”. This can be avoided by using a newer (0.8.*) Solidity compiler.

For loops improvement:

Caching the length in for loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset ) in the stack. Caching the array length in the stack saves around 3 gas per iteration. I suggest storing the array’s length in a variable before the for-loop.

Example of an array arr and the following loop:

for (uint i = 0; i < length; i++) {
    // do something that doesn't change the value of i
}

In the above case, the solidity compiler will always read the length of the array during each iteration.

If it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929) for each iteration except for the first),
If it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
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):

uint length = arr.length;
for (uint i = 0; i < length; i++) {
    // do something that doesn't change arr.length
}

In the above example, the 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.

This optimization is especially important if it is a storage array or if it is a lengthy for loop.

The increment in for loop post condition can be made unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

Example for loop:

for (uint i = 0; i < length; i++) {
    // do something that doesn't change the value of i
}

In this example, the for loop post condition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks. You should manually do this by:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

Or just:

for (uint i = 0; i < length;) {
    // do something that doesn't change the value of i
    unchecked { i++; }
}

Note that it’s important that the call to unchecked_inc is inlined. This is only possible for solidity versions starting from 0.8.2.

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant! (This is only relevant if you are using the default solidity checked arithmetic.)

++i costs less gas compared to i++ or i += 1

++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.

Example: 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

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas. As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with: for (uint256 i; i < numIterations; ++i) {

To sum up, the best gas optimized loop will be:

uint length = arr.length;
for (uint i; i < length;) {
    unchecked { ++i; }
}

https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L208 // in link below you should make this block unchecked and increment via ++index; https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L235

https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-periphery.sol#L154 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-periphery.sol#L380

https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L66 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L88

Add unchecked module

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this and save some gas a, but at the cost of some code readability, as this uncheck cannot be made inline.

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Comptroller.sol#L1121

Proof: You even added a comment there: // Underflow safe since amountToSubtract > currentAccrual.

No need to initialize extra storage variable

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant.

function borrow(
   Position storage position = positions[_nftIndex];

But in your case you have only one storage variable usage, this won't make code cheaper.

Line reference: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Comptroller.sol#L1162 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Comptroller.sol#L1241 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Comptroller.sol#L1280

Use double `require` instead of operator `&&`

contract Requires {
    uint256 public gas;

    function check1(uint x) public {
        gas = gasleft();    
        require(x == 0 && x < 1 ); // gas cost 22156
        gas -= gasleft();
    }

    function check2(uint x) public {
        gas = gasleft(); 
        require(x == 0); // gas cost 22148
        require(x < 1);
        gas -= gasleft();
    }
}

Gas savings: Usage of double require will save you around 10 gas with the optimizer enabled.

Change `constant` to `immutable` for keccak variables

Use of constant keccak variables results in extra hashing (and so gas). This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas. You should use immutables until the referenced issues are implemented, then you only pay the gas costs for the computation at deploy time.

Example:

contract Immutables is AccessControl {
    uint256 public gas;

    bytes32 public immutable MANAGER_ROLE_IMMUT;
    bytes32 public constant MANAGER_ROLE_CONST = keccak256('MANAGER_ROLE');

    constructor(){
        MANAGER_ROLE_IMMUT = keccak256('MANAGER_ROLE');
        _setupRole(MANAGER_ROLE_CONST, msg.sender);
        _setupRole(MANAGER_ROLE_IMMUT, msg.sender);
    }

    function immutableCheck() external {
        gas = gasleft();
        require(hasRole(MANAGER_ROLE_IMMUT, msg.sender), 'Caller is not in manager role'); // 24408 gas
        gas -= gasleft();
    }

    function constantCheck() external {
        gas = gasleft();
        require(hasRole(MANAGER_ROLE_CONST, msg.sender), 'Caller is not in manager role'); // 24419 gas
        gas -= gasleft();
    }
}

As you can see on the compiler version 0.8.15 and with optimizator on 200 runs immutables are cheaper, and saves you about 20 gas. For other variables, constants are equal to immutables.

See: (ethereum/solidity#9232 (comment), Inefficient Hash Constants)

Line reference: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Treasury/TreasuryDelegate.sol#L12 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Treasury/TreasuryDelegate.sol#L13

https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L15 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Governance/GovernorBravoDelegate.sol#L18

Missed require statement in modifier

This modifier is useless since the require is commented:

    modifier ensure(uint deadline) {
        //require(deadline >= block.timestamp, "BaseV1Router: EXPIRED");
        _;
    }

I recommend removing this modifier from all functions and removing deadline parameter from following functions.

No need to explicitly initialize variables with default values

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-periphery.sol#L123 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L48 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L72

Remove new bytes(0)

There is no need in additionally initialazing bytes when sending native via call().

(bool success,) = to.call{value:value}(new bytes(0));

Use .call.value(...)("") instead.

See: https://swcregistry.io/docs/SWC-134

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-periphery.sol#L469

Remove hardhat/console.sol from imports

Adding hardhat lib to your contract will make deploy more expensive.

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Accountant/AccountantDelegate.sol#L7 https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L4

Remove `console.log`

Remove console.log from contract before deploying it.

Line references: https://github.com/Plex-Engineer/lending-market-v2/blob/main/contracts/Stableswap/BaseV1-core.sol#L207

#0 - GalloDaSballo
2022-08-14T20:48:44Z

Change constant to immutable for keccak variables

Nope, disputed for ages -> https://twitter.com/GalloDaSballo/status/1543729080926871557

Less than 200 gas saved

Canto v2 contest - grGred's results

Execution layer for original work.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-15] QA Report - $43.23

Findings Information

Awards

Labels

External Links

Remove extra space

Add space

Remove extra tabulation

Missed comment:

#0 - GalloDaSballo2022-08-13T22:33:34Z

Space + Tabulation

Missed comment:

[G-17] Gas Report - $21.80

Findings Information

Awards

Labels

External Links

Consider having short revert strings

No need to add an extra variable since twapMantissa is used only once.

No need to do type conversion since an accountant is already an address

Use calldata instead of memory for function parameters

For loops improvement:

Add unchecked module

No need to initialize extra storage variable

Use double require instead of operator &&

Change constant to immutable for keccak variables

Missed require statement in modifier

No need to explicitly initialize variables with default values

Remove new bytes(0)

Remove hardhat/console.sol from imports

Remove console.log

#0 - GalloDaSballo2022-08-14T20:48:44Z

Change constant to immutable for keccak variables

#0 - GalloDaSballo
2022-08-13T22:33:34Z

No need to add an extra variable since `twapMantissa` is used only once.

No need to do type conversion since an `accountant` is already an address

Use `calldata` instead of `memory` for function parameters

Use double `require` instead of operator `&&`

Change `constant` to `immutable` for keccak variables

Remove `console.log`

#0 - GalloDaSballo
2022-08-14T20:48:44Z