Swivel v3 contest - Meera's results

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Unsafe casting may overflow
  • 1.2. Missing address(0) checks
• 2. Non-Critical Issues
  • 2.1. Typos
  • 2.2. Duplicated code: Hardcoded strings having multiple occurrences should be declared as constant
  • 2.3. Open TODOS
  • 2.4. The pragmas used are not the same everywhere

1. Low Risk Issues

1.1. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

Creator/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
Creator/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
Creator/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
Creator/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
Creator/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {
Marketplace/Compounding.sol:51:    if (p == uint8(Protocols.Compound)) {
Marketplace/Compounding.sol:53:    } else if (p == uint8(Protocols.Rari)) {
Marketplace/Compounding.sol:55:    } else if (p == uint8(Protocols.Yearn)) {
Marketplace/Compounding.sol:57:    } else if (p == uint8(Protocols.Aave)) {
Marketplace/Compounding.sol:59:    } else if (p == uint8(Protocols.Euler)) {
Marketplace/Compounding.sol:69:    if (p == uint8(Protocols.Compound)) {
Marketplace/Compounding.sol:71:    } else if (p == uint8(Protocols.Rari)) { 
Marketplace/Compounding.sol:73:    } else if (p == uint8(Protocols.Yearn)) {
Marketplace/Compounding.sol:75:    } else if (p == uint8(Protocols.Aave)) {
Marketplace/Compounding.sol:78:    } else if (p == uint8(Protocols.Euler)) {
Swivel/Swivel.sol:708:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:710:    } else if (p == uint8(Protocols.Yearn)) {
Swivel/Swivel.sol:713:    } else if (p == uint8(Protocols.Aave)) {
Swivel/Swivel.sol:719:    } else if (p == uint8(Protocols.Euler)) {
Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:743:    } else if (p == uint8(Protocols.Yearn)) {
Swivel/Swivel.sol:746:    } else if (p == uint8(Protocols.Aave)) {
Swivel/Swivel.sol:750:    } else if (p == uint8(Protocols.Euler)) {
Tokens/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
Tokens/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
Tokens/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
Tokens/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
Tokens/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {
VaultTracker/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
VaultTracker/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
VaultTracker/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
VaultTracker/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
VaultTracker/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {

1.2. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

Creator/VaultTracker.sol:21:  address public immutable admin;
Creator/VaultTracker.sol:22:  address public immutable cTokenAddr;
Creator/VaultTracker.sol:23:  address public immutable swivel;
Creator/ZcToken.sol:15:    address public override immutable underlying;
Creator/ZcToken.sol:21:    address public immutable cToken;
Marketplace/MarketPlace.sol:26:  address public immutable creator;
Swivel/Swivel.sol:29:  address public immutable marketPlace;
Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
Tokens/ZcToken.sol:15:    address public override immutable underlying;
Tokens/ZcToken.sol:21:    address public immutable cToken;
VaultTracker/VaultTracker.sol:21:  address public immutable admin;
VaultTracker/VaultTracker.sol:22:  address public immutable cTokenAddr;
VaultTracker/VaultTracker.sol:23:  address public immutable swivel;

2. Non-Critical Issues

2.1. Typos

• adpated

Creator/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.
Tokens/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.
Marketplace/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.

• compatability

Creator/ZcToken.sol:9:// Utilizing an external custody contract to allow for backwards compatability with some projects.
Creator/ZcToken.sol:22:    /// @dev address and interface for an external custody contract (necessary for some project's backwards compatability)
Tokens/ZcToken.sol:9:// Utilizing an external custody contract to allow for backwards compatability with some projects.
Tokens/ZcToken.sol:22:    /// @dev address and interface for an external custody contract (necessary for some project's backwards compatability)

• redeemption

Tokens/ZcToken.sol:67:    /// @notice Post maturity simulates the effects of redeemption at the current block. Returns 0 pre-maturity.
Creator/ZcToken.sol:67:    /// @notice Post maturity simulates the effects of redeemption at the current block. Returns 0 pre-maturity.

• recieving

Creator/ZcToken.sol:145:    /// @param t Address recieving the minted amount
Tokens/ZcToken.sol:145:    /// @param t Address recieving the minted amount

• ddress

Marketplace/Compounding.sol:34:  // @dev Deployed ddress of the associated Aave Pool

• reddem

Swivel/Swivel.sol:677:    // NOTE: for swivel reddem there is no transfer out as there is in redeemVaultInterest

• Varifies

Swivel/Swivel.sol:682:  /// @notice Varifies the validity of an order and it's signature.

• withraw

Swivel/Swivel.sol:747:      // Aave v2 docs state that withraw returns uint256

2.2. Duplicated code: Hardcoded strings having multiple occurrences should be declared as constant

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");

2.3. Open TODOS

Consider resolving the TODOs before deploying.

Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
Swivel/Swivel.sol:120:    // TODO cheaper to assign amount here or keep the ADD?
Swivel/Swivel.sol:157:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:192:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:221:    // TODO assign amount or keep ADD?
Swivel/Swivel.sol:286:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:317:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:347:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:382:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:707:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
Swivel/Swivel.sol:708:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:716:      // TODO explain the Aave deposit args
Swivel/Swivel.sol:721:      // TODO explain the 0 (primary account)
Swivel/Swivel.sol:740:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:748:      // TODO explain the withdraw args
Swivel/Swivel.sol:752:      // TODO explain the 0

2.4. The pragmas used are not the same everywhere

Creator/Compounding.sol:3:pragma solidity 0.8.13;
Creator/Creator.sol:3:pragma solidity 0.8.13;
Creator/Erc20.sol:4:pragma solidity ^0.8.0;
Creator/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Creator/IERC5095.sol:2:pragma solidity ^0.8.0;
Creator/Interfaces.sol:3:pragma solidity 0.8.13;
Creator/IRedeemer.sol:2:pragma solidity ^0.8.0;
Creator/LibCompound.sol:2:pragma solidity >=0.8.4;
Creator/LibFuse.sol:1:pragma solidity 0.8.13;
Creator/Protocols.sol:3:pragma solidity 0.8.13;
Creator/VaultTracker.sol:3:pragma solidity 0.8.13;
Creator/ZcToken.sol:2:pragma solidity ^0.8.4;
Marketplace/Compounding.sol:3:pragma solidity 0.8.13;
Marketplace/Erc20.sol:4:pragma solidity ^0.8.0;
Marketplace/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Marketplace/Interfaces.sol:3:pragma solidity 0.8.13;
Marketplace/LibCompound.sol:2:pragma solidity >=0.8.4;
Marketplace/LibFuse.sol:1:pragma solidity 0.8.13;
Marketplace/MarketPlace.sol:3:pragma solidity 0.8.13;
Marketplace/Protocols.sol:3:pragma solidity 0.8.13;
Swivel/Hash.sol:3:pragma solidity 0.8.13;
Swivel/Interfaces.sol:3:pragma solidity 0.8.13;
Swivel/Protocols.sol:3:pragma solidity 0.8.13;
Swivel/Safe.sol:3:pragma solidity 0.8.13;
Swivel/Sig.sol:3:pragma solidity 0.8.13;
Swivel/Swivel.sol:3:pragma solidity 0.8.13;
Tokens/Compounding.sol:3:pragma solidity 0.8.13;
Tokens/Erc20.sol:4:pragma solidity ^0.8.0;
Tokens/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Tokens/IERC5095.sol:2:pragma solidity ^0.8.0;
Tokens/Interfaces.sol:3:pragma solidity 0.8.13;
Tokens/IRedeemer.sol:2:pragma solidity ^0.8.0;
Tokens/LibCompound.sol:2:pragma solidity >=0.8.4;
Tokens/LibFuse.sol:1:pragma solidity 0.8.13;
Tokens/Protocols.sol:3:pragma solidity 0.8.13;
Tokens/ZcToken.sol:2:pragma solidity ^0.8.4;
VaultTracker/Compounding.sol:3:pragma solidity 0.8.13;
VaultTracker/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
VaultTracker/Interfaces.sol:3:pragma solidity 0.8.13;
VaultTracker/LibCompound.sol:2:pragma solidity >=0.8.4;
VaultTracker/LibFuse.sol:1:pragma solidity 0.8.13;
VaultTracker/Protocols.sol:3:pragma solidity 0.8.13;
VaultTracker/VaultTracker.sol:3:pragma solidity 0.8.13;

Overview

Table of Contents:

• 1. State variables only set in the constructor should be declared immutable
• 2. Cheap Contract Deployment Through Clones
• 3. Use of this instead of marking as public an external function
• 4. Use calldata instead of memory
• 5. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 6. Use Custom Errors instead of Revert Strings to save Gas

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:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

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:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

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");