Nibbl contest - joestakey's results

QA Report

Table of Contents

• summary
• hash collision with abi.encodePacked
• Immutable addresses lack zero-address check
• Receive function
• Setters should check the input value
• Unchecked transfer
• Comment Missing function parameter
• Commented Code
• Constants instead of magic numbers
• Event should be emitted in setters
• Function missing comments
• Function order
• Inconsistent spacing
• Non-library files should use fixed compiler versions
• Non-library files should use the same compiler version
• Public functions can be external
• Require statements should have descriptive strings
• Scientific notation
• Timelock for critical parameter change
• Uint256 alias

summary

Few vulnerabilities were found, the main concerns are with potential lock of ETH in some contracts

hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NibblVaultFactory.sol

here

bytes memory code = abi.encodePacked(type(ProxyVault).creationCode, uint256(uint160(address(this))));
bytes32 _hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)))

and here, with a string

ProxyBasket{salt: keccak256(abi.encodePacked(_curator, _mix))}(basketImplementation))

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ProxyBasket.sol

implementation = payable(_implementation)

ProxyVault.sol

factory = payable(_factory)

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the immutable variables aforementioned.

Receive function

PROBLEM

ProxyBasket and ProxyDelegate have receive() functions, but do not have any withdrawal function. A call to these contracts will trigger the fallback function. But if a call sends ETH to these contracts with no msg.data, fallback will not be triggered, only receive(), resulting in the ETH getting locked.

SEVERITY

Low

PROOF OF CONCEPT

ProxyBasket.sol

receive() external payable { }

ProxyVault.sol

receive() external payable { }

TOOLS USED

Manual Analysis

MITIGATION

Add require(0 == msg.value) in receive() or remove the function altogether.

Setters should check the input value

PROBLEM

Setters and initializers should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NibblVault.sol

_assetAddress lacks a check in initialize()
_curator lacks a check in initialize()
_newCurator lacks a check in updateCurator()

NibblVaultFactory.sol

_vaultImplementation lacks a check in the constructor
_feeTo lacks a check in the constructor
_basketImplementation lacks a check in the constructor
_newBasketImplementation lacks a check in proposeNewBasketImplementation
_newFeeAddress lacks a check in proposeNewAdminFeeAddress
_newVaultImplementation lacks a check in proposeNewVaultImplementation

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks - address - to the setters aforementioned.

Unchecked transfer

PROBLEM

Several tokens do not revert in case of ERC20.transfer() failure and return false. It is good practice to use safeTransfer() from OpenZeppelin, or simply check the return value of .transfer()

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NibblVault.sol

IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)))
IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)))

TOOLS USED

Manual Analysis

MITIGATION

use safeTransfer() from OpenZeppelin or check the return value of .transfer().

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

@param _totalSupply
@param _totalSupply
@param _totalSupply
@param _totalSupply
@return _saleReturn
@return _buyoutBid
@param _to
@return _amtOut
@return _feeAccruedCurator

NibblVaultFactory.sol

@return _proxyVault
@return _vault

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

// _purchaseReturn = _buySecondaryCurve(_to, _lowerCurveDiff)
// buyoutValuationDeposit = _currentValuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance)

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

1e18
1e18
1e18
1e18
2**32
2**32
2**32
2**32

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

function updateCurator

NibblVaultFactory.sol

function updateBasketImplementation
function updateNewAdminFeeAddress
function updateNewAdminFee
function updateVaultImplementation

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function missing comments

PROBLEM

Some functions are missing Natspec comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

function permit
function safeTransferETH
function onERC721Received
function onERC1155Received
function onERC1155BatchReceived

NibblVaultFactory.sol

function getVaults
function createBasket
function getBasketAddress
function withdrawAdminFee

Twav.sol

function getTwavObservations()

EIP712Base.sol

function INIT_EIP712()
function getChainID()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Function order

PROBLEM

Functions should be ordered following the Soldiity conventions: receive() function should be placed after the constructor and before every other function.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Several contracts have receive() at the end:

• NibblVault.sol

• NibblVaultFactory.sol

• Basket.sol

TOOLS USED

Manual Analysis

MITIGATION

Place the receive()functions after the constructor, before all the other functions.

Inconsistent spacing

PROBLEM

Spacing is present in the vast majority of comments (// x), but some comments do not have space.

SEVERITY

Non-Critical

PROOF OF CONCEPT

NibblVault.sol

///@notice current status of vault
///@notice reenterancy guard
///@notice withdraw multiple ERC721s

TOOLS USED

Manual Analysis

MITIGATION

Use spacings consistently in all comments.

Non-library files should use fixed compiler versions

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

AccessControlMechanism.sol has a floating pragma.

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

Non-library files should use the same compiler version

PROBLEM

contracts within the scope should be compiled using the same compiler version.

SEVERITY

Non-Critical

PROOF OF CONCEPT

All the files in scope have the compiler version set to 0.8.10, except for AccessControlMechanism.sol which has its pragma set to ^0.8.0.

TOOLS USED

Manual Analysis

MITIGATION

Use the same compiler version throughout the contracts

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVaultFactory.sol

function getVaultAddress
function getVaults
function createBasket
function getBasketAddress

Twav.sol

function getTwavObservations

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Require statements should have descriptive strings

PROBLEM

Some require statements are missing error strings, which makes it more difficult to debug when the function reverts.

SEVERITY

Non-critical

PROOF OF CONCEPT

NibblVaultFactory.sol

require(_success)

TOOL USED

Manual Analysis

MITIGATION

Add error strings to all require statements.

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5). Underscores are used throughout the contracts and do improve readability too, so this is more of a suggestion.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

uint256 private constant SCALE = 1_000_000

TOOLS USED

Manual Analysis

Timelock for critical parameter change

PROBLEM

It is good practice to add timelock to critical parameters changes, such as admin changes, to give users time to react.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NibblVault.sol

There is no timelock in updateCurator. If the curator were to input an incorrect address here, they would lose all the curators fees accrued.

TOOLS USED

Manual Analysis

MITIGATION

Add a timelock to updateCurator

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint. This is all the more valid as uint256 is used throughout the contracts, uint is only used in few instances.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Nibblvault.sol

uint _primaryReserveBalance = (primaryReserveRatio * _initialTokenSupply * _initialTokenPrice) / (SCALE * 1e18)
uint _secondaryReserveBalance = secondaryReserveBalance
uint _primaryReserveBalance = primaryReserveBalance
uint _secondaryReserveBalance = secondaryReserveBalance
uint _amount = unsettledBids[msg.sender]

TOOLS USED

Manual Analysis

MITIGATION

replace uint with uint256

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constant expressions
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Immutable variables save storage
• Mathematical optimizations
• Modifier instead of duplicate require
• Prefix increments
• Require instead of AND
• Revert strings length
• unchecked arithmetic
• unused import

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

Twav.sol

scope: _updateTWAV()

• twavObservationsIndex is read 3 times

line 27
line 28
line 29

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

NibblVault.sol

address[] memory _assetAddresses, uint256[] memory _assetIDs
address[] memory _assets
address[] memory _assets, uint256[] memory _assetIDs

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas, approximately 20 gas in require and if statements

PROOF OF CONCEPT

Instances include:

NibblVault.sol

buyoutEndTime <= block.timestamp
_secondaryReserveRatio <= primaryReserveRatio
_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO
_totalSupply >= _initialTokenSupply
_lowerCurveDiff >= msg.value
_minAmtOut <= _purchaseReturn
_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell"
(_initialTokenSupply + _amtIn) <= _totalSupply
_saleReturn >= _minAmtOut, "NibblVault: Return too low"
block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now"
_buyoutBid >= _currentValuation, "NibblVault: Bid too low"
_twav >= buyoutRejectionValuation
block.timestamp <= deadline

NibblVaultFactory.sol

msg.value >= MIN_INITIAL_RESERVE_BALANCE
block.timestamp >= basketUpdateTime
block.timestamp >= feeToUpdateTime
_newFee <= MAX_ADMIN_FEE
block.timestamp >= feeAdminUpdateTime
block.timestamp >= vaultUpdateTime

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-block.timestamp >= basketUpdateTime
+block.timestamp > basketUpdateTime - 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

-block.timestamp >= basketUpdateTime
+block.timestamp > basketUpdateTime;

Constant expressions

IMPACT

Constant expressions are re-calculated each time it is in use, costing an extra 97 gas than a constant every time they are called.

PROOF OF CONCEPT

Instances include:

NibblVault.sol

bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")

AccessControlMechanism.sol

bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE")
bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE")
bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE")

EIP712Base.sol

bytes32 internal constant EIP712_DOMAIN_TYPEHASH = keccak256(bytes("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"))

TOOLS USED

Manual Analysis

MITIGATION

Mark these as immutable instead of constant

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

NibblVaultFactory.sol

vaultImplementation = _vaultImplementation
feeTo = _feeTo
basketImplementation = _basketImplementation

ProxyBasket.sol

implementation = payable(_implementation)

ProxyVault.sol

factory = payable(_factory)

TOOLS USED

Manual Analysis

MITIGATION

Hardcode storage variables with their initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here.

It not only saves gas upon deployment - ~5500 gas saved per custom error instead of a require statement, but it is also cheaper in a function call, 22 gas saved per require statement replaced with a custom error.

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

NibblVault.sol

require(unlocked == 1, 'NibblVault: LOCKED')
require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out'))
require(status == Status.buyout, "NibblVault: status != buyout")
require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now")
require(!NibblVaultFactory(factory).paused(), 'NibblVault: Paused')
require(_secondaryReserveRatio <= primaryReserveRatio, "NibblVault: Excess initial funds")
require(_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO, "NibblVault: secResRatio too low")
require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low")
require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell")
require(_saleReturn >= _minAmtOut, "NibblVault: Return too low")
require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now")
require(status == Status.initialized, "NibblVault: Status!=initialized")
require(_buyoutBid >= _currentValuation, "NibblVault: Bid too low")
require(status == Status.buyout, "NibblVault: Status!=Buyout")
require(msg.sender == curator,"NibblVault: Only Curator")
require(msg.sender == curator,"NibblVault: Only Curator")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(block.timestamp <= deadline, "NibblVault: expired deadline")
require(signer == owner, "NibblVault: invalid signature")
require(success, "NibblVault: ETH transfer failed")

NibblVaultFactory.sol

require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low")
require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender")
require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(_success)
require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE")
require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")

AccessControlMechanism.sol

require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in NibbleVault.sol:

Replace

require(unlocked == 1, 'NibblVault: LOCKED')

with

if (unlocked != 1) {
		revert NibblVaultLocked();
}

and define the custom error in the contract

error NibblVaultLocked();

Here are the deployment costs comparison between:

• the original NibbleVault contract ······································| NibblVault · - · - · 7045960 │

• the same NibbleVault contract with one require statement replaced with a custom error:

·············· | NibblVault · - · - · 7036463 │

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes 3 gas per variable initialized.

PROOF OF CONCEPT

Instances include:

NibbleVault.sol

uint i = 0
uint i = 0
uint i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwrds, marking it as immutable can save a storage operation - 20,000 gas.

PROOF OF CONCEPT

Instances include:

NibblVaultFactoryData.sol

uint256 public UPDATE_TIME = 2 days

Note: the contract is technically not in scope, but as NibblVaultFactory, which is in scope, inherits from NibblVaultFactoryData, I consider it a valid optimization.

TOOLS USED

Manual Analysis

MITIGATION

Mark these variables as immutable.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y.

PROOF OF CONCEPT

Instances include:

NibblVault.sol

feeAccruedCurator += _feeCurator
secondaryReserveBalance += _feeCurve
feeAccruedCurator += _feeCurator
secondaryReserveBalance += _lowerCurveDiff
_purchaseReturn += _buyPrimaryCurve(msg.value - _lowerCurveDiff, _totalSupply + _purchaseReturn)
primaryReserveBalance -= _saleReturn
_saleReturn += _sellSecondaryCurve(_amtIn - _tokensPrimaryCurve, _initialTokenSupply)
unsettledBids[bidder] += _buyoutValuationDeposit
totalUnsettledBids += _buyoutValuationDeposit
totalUnsettledBids -= _amount

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Modifier instead of duplicate require

PROBLEM

When a require statement is use multiple times, it is cheaper to use a modifier instead.

PROOF OF CONCEPT

Instances include:

NibblVault.sol

require(msg.sender == curator,"NibblVault: Only Curator")
require(msg.sender == curator,"NibblVault: Only Curator")

require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")
require(msg.sender == bidder,"NibblVault: Only winner")

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments: it returns the incremented variable instead of returning a temporary variable storing the initial value of the variable. It saves 5 gas per iteration

PROOF OF CONCEPT

Instances include:

NibbleVault.sol

i++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

NibblVaultFactory.sol

require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

-require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
+require(basketUpdateTime != 0)
+require(block.timestamp >= basketUpdateTime)

You can also improve gas savings by using custom errors

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes. Each string exceeding that 32-byte size adds an extra 9,500 gas upon deployment.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include:

NibblVaultFactory.sol

require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low")
require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender")
require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE")
require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")
require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed")

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

NibbleVault.sol

i++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unused import

IMPACT

Unused imports should be removed as they waste gas upon deployment

PROOF OF CONCEPT

Instances include:

NibbleVaultFactory.sol

import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";

TOOLS USED

Manual Analysis

MITIGATION

Remove the SafeMath import.