Nibbl contest - naps62's results

QA Report

ERC20Upgradeable _totalSupply shadowed

There are multiple usages of _totalSupplythat shadow the inherited ERC20Upgradeable variable _totalSupply

Gas optimization Report

For loops gas optimization

Change the length variable to a local variable and increment the index on the end of the for loop statement inside an unchecked block.

For example:

function withdrawMultipleERC20(address[] memory _tokens) external override {
    require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
    
    for (uint256 i = 0; i < _tokens.length; i++) {
        IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
        emit WithdrawERC20(_tokens[i], msg.sender);
    }
}

function withdrawMultipleERC20(address[] memory _tokens) external override {
    require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
    
    uint256 tokensLength = _tokens.length
    
    for (uint256 i = 0; i < tokensLength ; ) {
        IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
        emit WithdrawERC20(_tokens[i], msg.sender);

        unchecked {
            ++i;
        }
    }
}

Timestamp overflow protection not needed

The multiple usages of uint32(block.timestamp % 2**32) doesn't seem needed , because solidity already deals with the overflow case, this has been described on this uniswapV2-core issue by moodysalem, https://github.com/Uniswap/v2-core/issues/96.

A more in depth explanation :

• the current unix timestamp only need 4 bytes to represent so a uint32 wont actually have any practical colision.
• The next possible colision would be on 4294967296 when hex representation is 0xffffffff + 1 which is 84 years from now.
• Calculating the modulo of the timestamp with 2**32 is the same logic as casting to a uint32, doing both is essentially truncating to the same length twice.

Constant value is not constant

On the NibblVaultFactoryData on line 6 uint256 public UPDATE_TIME = 2 days; is missing the constant keyword.

<!-- The variable name appears to represent a constant and is never updated on the codebase.-->

NibblVault and NibblVaultStorage variables can be smaller, and better packed

Some variables in NibblVault and NibblVaultFactoryData can be converted to smaller integer values (e.g.: uint32 instead of uint256) with no risk of overflow.

This in turn opens the possibility of more tighly packing them together for more gas-efficient storage (e.g.: by packing and address with 3 uint32, we can use a single 256-bit slot instead of 4).

Relevant examples:

• NibbVaultData.feeAdmin can likely be a uint32 since it represents a percentage
• NibbVaultData.pendingFeeAdmin can likely be a uint32 since it represents a percentage
• NibblVaultData.feeAdminUpdateTime can be a uint32 as it represents a timestamp
• NibblVaultData.basketUpdateTime can be a uint32 as it represents a timestamp
• NibblVaultData.vaultUpdateTime can be a uint32 as it represents a timestamp
• NibblVault.buyoutEndTime can be a uint32 as it represents a timestamp
• NibblVault.minBuyoutTime can be a uint32 as it represents a timestamp
• NibblVault.minBuyoutTime can be a uint32 as it represents a timestamp timestamp
• NibblVault.unlocked can be a uint8 as it represents a non-zero boolean