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

0 stars 1 forks source link

Gas Optimizations #96

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Overview

Risk Rating Number of issues
Gas Issues 6

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)

Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?

Furthermore, this variable was already flagged as "to be made immutable".

2. Cheap Contract Deployment Through Clones

Creator/Creator.sol:40:    address zct = address(new ZcToken(p, u, m, c, marketPlace, n, s, d));
Creator/Creator.sol:41:    address tracker = address(new VaultTracker(p, m, c, sw));

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

Here with a cloneDeterministic method to mimic the current create2

Keep in mind however, that the gas saving only concerns the deployment cost. Indeed, by using this pattern, the cost to use the deployed contract increases a bit.

Depending on the sponsor's choice (saving a bit of gas for the end-users or a lot of gas for the deployer), this pattern might be considered.

3. Use of this instead of marking as public an external function

Using this. is like making an expensive external call.

Consider marking previewWithdraw() as public (if possible with the current IERC5095 implementation):

Creator/ZcToken.sol:
  99:         uint256 previewAmount = this.previewWithdraw(underlyingAmount);

Tokens/ZcToken.sol:
  99:         uint256 previewAmount = this.previewWithdraw(underlyingAmount);

4. Use calldata instead of memory

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly bypasses this loop.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gas-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Affected code (around 60 gas per occurence):

Swivel/Swivel.sol:495:  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {

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:

Swivel/Swivel.sol:100:      unchecked {i++;}
Swivel/Swivel.sol:269:      unchecked {i++;}
Swivel/Swivel.sol:418:        i++;
Swivel/Swivel.sol:511:        x++;
Swivel/Swivel.sol:564:        i++;

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

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

Creator/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
Creator/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
Tokens/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
Tokens/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
VaultTracker/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
VaultTracker/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");