byterocket / c4-common-issues

A collection of common security issues and possible gas optimizations in solidity smart contracts

c4-common-issues

This repository aims to provide a list of issues found regularly in Code4Rena contests.

Introduction

Code4Rena runs community-driven contests for smart contract audits in which wardens search for issues and judges allocate awards to wardens based on performance.

A problem currently in the community is how to handle commonly found issues as they increase the judge's workload.

This repo aims to identify common issues and provide background information.

The goal is, that wardens include the issue's identifier in a contest submission.

This would help judges mark the issues as duplicates and hopefully decrease their workload.

Gas Optimization Issues

Identifier	Title
G001	Don't Initialize Variables with Default Value
G002	Cache Array Length Outside of Loop
G003	Use != 0 instead of > 0 for Unsigned Integer Comparison
G004	Remove Unused Variables
G005	Make Variable constant/immutable
G006	Use immutable for OpenZeppelin AccessControl's Roles Declarations
G007	Long Revert Strings
G008	Use Shift Right/Left instead of Division/Multiplication if possible
G009	Make Function external instead of public
G010	Make Function payable
G011	Unnecessary checked arithmetic in for loop
G012	Use Prefix Increment instead of Postfix Increment if possible

Non-Critical Issues

Identifier	Title
NC001	Functions Mutating Storage Should Emit Events

Low Risk Issues

Identifier	Title
L001	Unsafe ERC20 Operation(s)
L002	FeeOnTransfer Tokens not Supported
L003	Unspecific Compiler Version Pragma
L004	Use Two-Step Transfer Pattern for Access Controls
L005	Do not use Deprecated Library Functions
L006	Check that Contract Exists before using solmate's SafeTransferLib

Contribution

Any kind of contribution is highly welcome!

License

This work is licensed under a Creative Commons Attribution 4.0 International License.

c4-common-issues's Issues

`iszero(x)` instruction is more gas efficient than `x == 0`

The iszero(x) instruction is more gas efficient than x == 0

Severity

Which severity would you assign to this Issue?

• Gas Optimization
• Non-Critical
• Low Risk
• Med Risk
• High Risk

Description

With the use of the EVM dialect (=Yul), zero checking can be made more efficient via inline assembly.

Example

🤦 Bad:

/// @dev Consumes ~21812 gas on average.
function foo(uint256 x) external pure returns (bool result) {
    return x == 0;
}

🚀 Good:

/// @dev Consumes ~21787 gas on average.
function bar(uint256 x) external pure returns (bool result) {
    assembly {
        result := iszero(x)
    }
}

Background Information

• https://twitter.com/PaulRBerg/status/1521094594833915904

Incorrect GasOptimization: G009 - Make function `external` instead of `public`

As per this SO answer, the "external is cheaper than public" mantra seems to be wrong.

However, this is only the case for versions >=0.6.9. Adjust the issue description to reflect this.

Remove or adjust G006

Remove or adjust: G006 - immutable for OpenZeppelin AccessControl's Roles Declarations

Since Solidity version 0.6.12 the keccak256 of string literals are treated specially and the hash is evaluated at compile time. It's not even necessary to have optimizations enabled.

Severity

Which severity would you assign to this Issue?

• Gas Optimization
• Non-Critical
• Low Risk
• Med Risk
• High Risk

`++i` is more gas efficient than `i++` or `i += i` (if x = 1)

++i is more gas efficient than i++ or i += x (if x = 1)

Severity

Which severity would you assign to this Issue?

• Gas Optimization
• Non-Critical
• Low Risk
• Med Risk
• High Risk

Description

writing a value to a slot that doesn’t have one will be more expensive than writing to one that does.
when incrementing an integer, ++i (return old value, then add 1) is cheaper than i++ (add 1, then return new value)

Example

🤦 Bad:

/// @dev Consumes ~21586 gas on average.
function war() external pure returns (uint256 result) {
         uint i = 1;
        result += i;
    }

or

/// @dev Consumes ~21557 gas on average.
function bar() external pure returns (uint256 result) {
     result ++;
    }

🚀 Good:

 /// @dev Consumes ~21530 gas on average. 
function foo() external pure returns (uint256 result) {
        ++ result;
    }

Background Information

https://m1guelpf.blog/d0gBiaUn48Odg8G2rhs3xLIjaL8MfrWReFkjg8TmDoM

Hidden unbounded loop in function call due to storage to memory copy

Hidden unbounded loop in function call due to storage to memory copy

Severity

Which severity would you assign to this Issue?

• Gas Optimization
• Non-Critical
• Low Risk
• Med Risk
• High Risk

Description

See https://twitter.com/danielvf/status/1519381832592199681.

Example

If applicable, provide a small example. If not, delete this section.

🤦 Bad:

unit[] public array;

function _requireLength(unit[] memory _array) internal pure {
    // The memory keyword leads to SLOADing the entire array into memory, i.e. creates hidden, unbounded loop.
    require(_array.length > 0, "Empty array");
}

🚀 Good:

unit[] public array;

function _requireLength(unit[] storage _array) internal pure {
    // No hidden loop.
    require(_array.length > 0, "Empty array");
}

Background Information

See https://twitter.com/danielvf/status/1519381832592199681.

Code4Rena Report/Issue Link

TODO: Not found yet

Missing check whether contract exists before using `solmate`'s `SafeTransferLib`

Missing check whether contract exists before using solmate's SafeTransferLib

Severity

Which severity would you assign to this Issue?

• Gas Optimization
• Non-Critical
• Low Risk
• Med Risk
• High Risk

Description

solmate's SafeTransferLib states in the contract docs that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

A call to an address with no code will be a no-op since low-level calls to non-contracts always return success.

Consider verifying whether a contract, i.e. token, exists before using any SafeTransferLib functions:

require(token.code.length != 0, "Contract does not exist");

Example

🤦 Bad:

// Note that this function is for demonstration purposes only and should not be used as is.
function fetchTokens(address token, address from, uint amount) {
    ERC20(token).safeTransferFrom(from, amount);
}

🚀 Good:

// Note that this function is for demonstration purposes only and should not be used as is.
function fetchToken(address token, address from, uint amount) {
    require(token.code.length != 0, "Contract does not exist");
    ERC20(token).safeTransferFrom(from, amount);
}

Background Information

• solmate's SafeTransferLib

Code4Rena Report/Issue Link

• First point in c4's backed QA report