code-423n4 / 2022-07-fractional-findings

0 stars 0 forks source link

Gas Optimizations #611

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Overview

Risk Rating Number of issues
Gas Issues 9

Table of Contents:

1. State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

modules/protoforms/BaseVault.sol:19:    address public registry;
modules/Buyout.sol:29:    address public registry;
modules/Buyout.sol:31:    address public supply;
modules/Buyout.sol:33:    address public transfer;
modules/Migration.sol:37:    address payable public buyout;
modules/Migration.sol:39:    address public registry;
modules/Minter.sol:14:    address public supply;
VaultFactory.sol:15:    address public implementation;

2. Reduce the size of error messages (Long revert Strings)

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.

Revert strings > 32 bytes:

utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");

Consider shortening the revert strings to fit in 32 bytes.

3. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+

Affected code (saves around 2 gas + 20 for unchecked per instance):

utils/MerkleBase.sol:100:                _node = _node / 2;
utils/MerkleBase.sol:136:                result = new bytes32[](length / 2 + 1);
utils/MerkleBase.sol:142:                result = new bytes32[](length / 2);

4. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
modules/Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {
utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {

5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

Decrement:

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

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

Affected code:

utils/MerkleBase.sol:188:                ceil++;
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

6. Increments/decrements can be unchecked in for-loops

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.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
modules/Migration.sol:496:            for (uint256 i; i < modulesLength; ++i) {
modules/Migration.sol:504:            for (uint256 i; i < modulesLength; ++i) {
modules/Migration.sol:507:                for (uint256 j; j < leavesLength; ++j) {
utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

7. Without the Optimizer: it costs more gas to initialize variables with their default value than letting the default value be applied

If the optimizer is enabled, this finding isn't true anymore

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 (around 3 gas per instance).

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

Consider removing explicit initializations for default values.

8. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Consider replacing all revert strings with custom errors in the solution.

utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");
FERC1155.sol:263:        require(
FERC1155.sol:275:        require(
FERC1155.sol:297:        require(metadata[_id] != address(0), "NO METADATA");

9. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyController is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

FERC1155.sol:198:    function setContractURI(string calldata _uri) external onlyController {