Skip to content

Home

Jump to bottom
Riccardo Malatesta edited this page May 7, 2024 · 13 revisions

Index

Gas optimizations

Using bools for storage incurs overhead

Use uint256 for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past. This can save 17100 gas per instance.

Reference:

Array length not cached outside of loop

Caching the length eliminates the additional DUP<N> required to store the stack offset and converts each of them to a DUP<N>. Gas saved: 3 per instance.

Reference:

Variables initialized with default value

When a variable is not set / initialized, it's assumed to have the default value. This means that explicitly initialize a variable with its default value wastes gas.

Reference:

Missing implementation Shift Right/Left for division and multiplication

The SHR opcode only utilizes 3 gas, compared to the 5 gas used by the DIV opcode. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Reference:

Unsigned integer comparison with > 0

Comparisons done using != 0 are cheaper than > 0 when dealing with unsigned integer types.

Reference:

Long revert/require string

Strings in require() / revert() longer than 32 bytes cost extra gas.

Reference:

Postfix increment/decrement used

The prefix increment / decrease expression returns the updated value after it's incremented while the postfix increment / decrease expression returns the original value. Be careful when employing this optimization anytime the return value of the expression is utilized later; for instance, uint a = i++ and uint a = ++i produce different values for a.

References:

Variable not constant/immutable

If a variable doesn't need to change, use constant or immutable. This will avoid the fees due to a SLOAD operation.

Reference:

Make function external instead of public

Version 0.6.9 removed the restriction on public functions accepting calldata arguments. Public functions for Solidity versions 0.6.9 have to transfer the parameters to memory.

Reference:

Mark payable functions guaranteed to revert when called by normal users

If a normal user tries to pay the function and a function modifier, such as onlyOwner, is applied, the function will revert. By making the function payable, valid callers will avoid paying for gas as the compiler won't verify that a payment was made. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Reference:

Use assembly to check for address(0)

By checking for address(0) using assembly language, you can avoid the use of more gas-expensive operations such as calling a smart contract or reading from storage. This can save 6 gas per instance.

Reference:

Non-Critical issues

Use require instead of assert when possible

If assert() returns a false assertion, it compiles to the invalid opcode 0xfe, which eats up all the gas left and completely undoes the changes. require() compiles to 0xfd, which is the opcode for a REVERT, indicating that it will return the remaining gas if it returns a false assertion.

Reference:

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

Gas consumption can be greater if you use items that are less than 32 bytes in size. This is such that the EVM can only handle 32 bytes at once. In order to increase the element's size from 32 bytes to the necessary amount, the EVM must do extra operations if it is lower than that. When necessary, it is advised to utilize a bigger size and then downcast.

References:

Use selfbalance() instead of address(this).balance

The BALANCE opcode costs 100 GAS minimum, SELFBALANCE costs 5 GAS minimum.

References:

Usage of constant keccak variables results in extra hashing

The usage of constant for keccak variables results in extra hashing and more gas. You should use immutable. This saves about 20 gas.

References:

Split require() statements that use && to save gas

To save gas, it is advised to use two require instead of using one require with the operator &&. This can save 8 gas per instance.

Reference:

x += y costs more gas than x = x + y for state variables

Gas can be saved by substituting the addition operator with plus-equals, same for minus. This can save 10 gas per instance.

Reference:

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow

In solidity versions 0.8.0 and higher, unchecked saves 30-40 gas per loop.

Reference:

Superfluos event fields

In the event information, block.number and block.timestamp are already added by default.

Reference:

Use if(x) or if(!x) instead of if (x == bool)

Avoid comparing boolean expressions to boolean literals. This will reduce complexity and gas.

Reference:

When possible, use non-strict comparison >= and/or =< instead of > <

Non-strict inequalities are cheaper than strict ones due to some supplementary checks (ISZERO, 3 gas). It will save 15–20 gas.

Reference:

If possible, use private rather than public for constants

If necessary, the values can be obtained from the verified contract source code; alternatively, if there are many values, a single getter function that returns a tuple containing the values of all currently-public constants can be used. The compiler doesn't have to write non-payable getter functions for deployment calldata, store the value's bytes somewhere other than where it will be used, or add another entry to the method ID table. This saves 3406-3606 gas in deployment gas.

Reference:

Use a more recent version of Solidity to save gas

Use a version of Solidity at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value (from v 0.8.10), get custom errors, which are cheaper at deployment than revert()/require() strings (from v 0.8.4), get better struct packing and cheaper multiple storage reads (from v 0.8.3) and get simple compiler automatic inlining (from v 0.8.2).

Reference:

require()/revert() statements should have descriptive reason strings

To increase overall code clarity and aid in debugging whenever a need is not met, think about adding precise, informative error messages to all require and revert statements.

References:

Unnamed return parameters

To increase explicitness and readability, take into account introducing and utilizing named return parameters.

Reference:

Usage of abi.encodePacked instead of bytes.concat() for Solidity version >= 0.8.4

From the Solidity version 0.8.4 it was added the possibility to use bytes.concat with variable number of bytes and bytesNN arguments. With a more evocative name, it functions as a restricted abi.encodePacked.

References:

For modern and more readable code; update import usages

To be sure to only import what you need, use specific imports using curly brackets.

Reference:

Code base comments with TODOs

Consider keeping track of all TODO comments in the backlog of issues and connecting each inline TODO to the related item. Before deploying to a production environment, all TODOs must be completed.

Reference:

SPDX-License-Identifier missing

Missing license agreements (SPDX-License-Identifier) may result in lawsuits and improper forms of use of code.

Reference:

File is missing pragma

Without a pragma statement, the smart contract may encounter compatibility issues with future compiler versions, leading to unpredictable behavior.

Reference:

Consider commenting why the body of the function is empty

The functions shown have an empty body. Consider commenting why for a clearer reading of the code.

Reference:

Magic Numbers in contract

Magic numbers, undefined numeric literals embedded directly into the code, pose a risk to the readability, maintainability, and security of Solidity smart contracts. To mitigate this issue, establish clear constants or variables for numeric values, providing meaningful context and promoting code transparency.

Reference:

public function not used internally could be marked as external

public functions in a smart contract that aren't actually used within the contract itself could be marked as external as they serve no purpose internally.

Low issues

Compiler version Pragma is non-specific

For non-library contracts, floating pragmas may be a security risk for application implementations, since a known vulnerable compiler version may accidentally be selected or security tools might fallback to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

References:

Unsafe ERC20 operations

ERC20 operations might not be secure due to multiple implementations and vulnerabilities in the standard. It is advised to use OpenZeppelin's SafeERC20 or, at least, wrap each operation in a require statement.

References:

Deprecated OpenZeppelin library functions

The contracts use deprecated OpenZeppelin Library functions, it is recommend that you avoid using them.

References:

Avoid using abi.encodePacked() with dynamic types when passing the result to a hash function

Instead of using abi.encodePacked() use abi.encode(). It will pad items to 32 bytes, which will prevent hash collisions. It is possible to cast to bytes() or bytes32() in place of abi.encodePacked() when there is just one parameter, see how to compare strings in solidity?. bytes.concat() should be used if all parameters are strings or bytes.

Reference:

Use safeTransferOwnership instead of the transferOwnership method

transferOwnership function is used to change ownership. It is reccomended to use a 2 structure transferOwnership which is safer, such as safeTransferOwnership.

Reference:

Use _safeMint instead of _mint

In favor of _safeMint(), which guarantees that the receiver is either an EOA or implements IERC721Receiver, _mint() is deprecated.

References:

Draft OpenZeppelin dependencies

OpenZeppelin draft contracts may not have undergone sufficient security auditing or are subject to change as a result of upcoming development.

Reference:

Timestamp dependency: use of block.timestamp (or now)

The timestamp of a block is provided by the miner who mined the block. As a result, the timestamp is not guaranteed to be accurate or to be the same across different nodes in the network. In particular, an attacker can potentially mine a block with a timestamp that is favorable to them, known as \selective packing. For example, an attacker could mine a block with a timestamp that is slightly in the future, allowing them to bypass a time-based restriction in a smart contract that relies on block.timestamp. This could potentially allow the attacker to execute a malicious action that would otherwise be blocked by the restriction. It is reccomended to, instead, use an alternative timestamp source, such as an oracle, that is not susceptible to manipulation by a miner.

References:

Usage of calls inside of loop

A denial-of-service attack might result from calls made inside a loop.

Reference:

Outdated Compiler Version

Using an older compiler version might be risky, especially if the version in question has faults and problems that have been made public.

References:

Use Ownable2StepUpgradeable instead of OwnableUpgradeable contract

It is recommended to use Ownable2StepUpgradeable instead of OwnableUpgradeable contract.

Reference:

ecrecover() does not check for address(0)

In the contract it was found the use of ecrecover() without implementing proper checks for address(0). When a signature is incorrect, ecrecover may occasionally provide a random address rather than 0. It is also reccomended to implement the OpenZeppelin soludion ECDSA.sol.

Reference:

Use require instead of assert

It is reccomended to use require instead of assert since the latest, when false, uses up all the remaining gas and reverts all the changes made.

Reference:

Deprecated ChainLink library function

As per Chainlink's documentation, the contracts use deprecated ChainLink Library functions, it is recommend that you avoid using them.

Solidity >= 0.8.20 PUSH0 opcode incompatibility across EVM chains

Solidity compiler version 0.8.20 introduces a bytecode optimization that utilizes PUSH0 opcodes for gas efficiency. However, this may cause deployment issues on EVM implementations, such as certain L2 chains, that do not support PUSH0. It's crucial to consider the target deployment chain's compatibility and select the appropriate Solidity version or adjust the compiler settings to ensure seamless contract deployment.

Declared and not used errors in the contract

Some errors are declared in the contract but are never used. It is advisable to examine them to ascertain whether they need to be used or possibly comment or delete them.

Medium severity issues

Centralization risk detected: contract has a single point of control

Centralization risks are weaknesses that malevolent project creators as well as hostile outside attackers can take advantage of. They may be used in several forms of attacks, including rug pulls. When contracts have a single point of control, contract owners need to be trusted to prevent fraudulent upgrades and money draining since they have privileged access to carry out administrative chores. Some solutions to this issue include implementing timelocks and/or multi signature custody.

Reference:

NFT can be frozen in the contract, use _safeMint instead of _mint

The NFT can be frozen in the contract if msg.sender is an address for a contract that does not support ERC721. This means that users could lose their NFT. It is reccomended to use _safeMint instead of _mint.

References:

Use of the deprecated latestAnswer function in contracts

As per Chainlink's documentation, the latestAnswer function is no longer recommended for use. This function does not generate an error in case no answer is available; instead, it returns 0, which could lead to inaccurate prices being provided to various price feeds or potentially result in a Denial of Service. It is recommended to use latestRoundData().

SafeTransferLib.sol does not check if a token is a contract or not

As per Solmate's SafeTransferLib.sol, the contract does not verify the existence of the token contract, delegating this responability to the caller. This creates the possiblity for a honeypot attack. An example is the Qubit Finance hack in January 2022. Consider using OpenZeppelin's SafeERC20 instead.

Nested loops could lead to Denial of Service

Nested loops in Solidity can lead to an exponential increase in gas consumption. This can cause transactions to fail causing a Denial of Service which can compromise the reliability and scalability of the protocol.

High severity issues

Use of delegatecall inside of a loop

Using delegatecall in a payable function within a loop can pose a vulnerability where each call retains the msg.value of the initial transaction. This can lead to unexpected behaviors, especially in scenarios involving fund transfers.

References:

Arbitrary from in transferFrom / safeTransferFrom

Allowing any from address to be passed to transferFrom (or safeTransferFrom) may result in potential loss of funds, as it enables anyone to transfer tokens from the designated address upon approval.

Outdated version of openzeppelin-contracts

Implementing an outdated version of @openzeppelin/contracts, specifically prior to version 4.9.5, introduces multiple high severity issues into the protocol's smart contracts, posing significant security risks. Immediate updating is crucial to mitigate vulnerabilities and uphold the integrity and trustworthiness of the protocol's operations. Check openzeppelin-contracts public reported and fixed security issues.

Outdated version of openzeppelin-contracts-upgradeable

Implementing an outdated version of @openzeppelin/contracts-upgradeable, specifically prior to version 4.9.5, introduces multiple high severity issues into the protocol's smart contracts, posing significant security risks. Immediate updating is crucial to mitigate vulnerabilities and uphold the integrity and trustworthiness of the protocol's operations. Check openzeppelin-contracts public reported and fixed security issues.

Use of msg.value inside of a loop

Using msg.value inside a loop can be problematic as the same value will be reused and this can lead to breaking protocol logic depending on the scenario.

Clone this wiki locally