Nibbl contest - delfin454000's results

Typos

contracts/NibblVault.sol: L36

    /// @notice The days until which a buyout bid is valid, if the bid isn't rejected in buyout duration time, its automatically considered boughtOut

Change until to during

The same typo (when) occurs in all four lines referenced below:

contracts/NibblVault.sol: L137 contracts/NibblVault.sol: L144 contracts/NibblVault.sol: L368 contracts/NibblVault.sol: L448

Example:

    /// @dev Check for the case when buyoutTime has not passed or buyout has been rejected

Change when to where

The same typo (fee is) occurs in the three lines referenced below: contracts/NibblVault.sol: L234 contracts/NibblVault.sol: L284 contracts/NibblVault.sol: L344

Example:

    /// @dev only admin and curator fee is charged in secondary curve

Change fee is to fees are in each case

The same typo (continous) occurs in all four lines referenced below:

contracts/NibblVault.sol: L250 contracts/NibblVault.sol: L270 contracts/NibblVault.sol: L282 contracts/NibblVault.sol: L359

Example:

    /// @dev The max continous tokens on SecondaryCurve is equal to initialTokenSupply

Change continous to continuous in each case

contracts/NibblVault.sol: L258

    /// @dev fictitiousPrimaryReserveBalance doesn't denote any actual reserve balance its just for calculation purpose

Change purpose to purposes

contracts/NibblVault.sol: L263

    /// @dev Valuation = If current supply is on seconday curve we use secondaryReserveBalance and secondaryReserveRatio to calculate valuation else we use primary reserve ratio and balance

Change seconday to seconday

contracts/NibblVault.sol: L361

    /// @param _to Address to recieve the reserve token to

Change recieve to receive and remove the final to

contracts/NibblVault.sol: L462

    /// @dev The redeemed reserve token are in proportion to the token supply someone owns

Change token to tokens

The same typo (internall) occurs in both lines referenced below:

contracts/Proxy/ProxyVault.sol: L26 contracts/Proxy/ProxyBasket.sol: L26

     * This function does not return to its internall call site, it will return directly to the external caller.

Change internall to internal in both cases

Long single line comments

For the sake of readability, comments over 79 characters should wrap using multi-line comment syntax. Note that in cases where the information can be communicated with fewer words, it might be possible (and preferable) to shorten the comments to fit on one line

contracts/NibblVault.sol: L36

    /// @notice The days until which a buyout bid is valid, if the bid isn't rejected in buyout duration time, its automatically considered boughtOut

Suggestion:

    /// @notice The days until which a buyout bid is valid, if the bid isn't rejected
    ///    in buyout duration time, it's automatically considered boughtOut

Similarly for the comment lines referenced below:

contracts/NibblVault.sol: L16 contracts/NibblVault.sol: L17 contracts/NibblVault.sol: L19 contracts/NibblVault.sol: L27 contracts/NibblVault.sol: L33 contracts/NibblVault.sol: L36 contracts/NibblVault.sol: L39 contracts/NibblVault.sol: L45 contracts/NibblVault.sol: L77 contracts/NibblVault.sol: L78 contracts/NibblVault.sol: L79 contracts/NibblVault.sol: L86 contracts/NibblVault.sol: L98 contracts/NibblVault.sol: L169 contracts/NibblVault.sol: L208 contracts/NibblVault.sol: L209 contracts/NibblVault.sol: L210 contracts/NibblVault.sol: L211 contracts/NibblVault.sol: L249 contracts/NibblVault.sol: L251 contracts/NibblVault.sol: L258 contracts/NibblVault.sol: L259 contracts/NibblVault.sol: L260 contracts/NibblVault.sol: L261 contracts/NibblVault.sol: L262 contracts/NibblVault.sol: L263 contracts/NibblVault.sol: L295 contracts/NibblVault.sol: L296 contracts/NibblVault.sol: L297 contracts/NibblVault.sol: L298 contracts/NibblVault.sol: L356 contracts/NibblVault.sol: L357 contracts/NibblVault.sol: L358 contracts/NibblVault.sol: L395 contracts/NibblVault.sol: L396 contracts/NibblVault.sol: L397 contracts/NibblVault.sol: L405 contracts/NibblVault.sol: L461 contracts/NibblVault.sol: L463 contracts/NibblVault.sol: L512 contracts/NibblVault.sol: L530 contracts/NibblVault.sol: L531

Missing require revert string

Require message should be included to enable users to understand reason for failure

contracts/NibblVaultFactory.sol: L114

        require(_success);

Add a brief message to explain require failure

Require message is too long

The revert strings below can be shortened to 32 characters or fewer (as shown) to save gas

contracts/NibblVaultFactory.sol: L48

        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");

Change message to NibblVaultFactory: Low res bal

contracts/NibblVaultFactory.sol: L49

        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");

Change message to NibblVaultFactory: Inval sender

Identical long require revert strings occur in all four lines referenced below. It's not clear how to shorten the message without abbreviating 'NibblVaultFactory'

contracts/NibblVaultFactory.sol: L107 contracts/NibblVaultFactory.sol: L131 contracts/NibblVaultFactory.sol: L149 contracts/NibblVaultFactory.sol: L166

Example:

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

Use of '&&' within a require function

Splitting such require() statements into separate requires instead of using '&&' saves gas

contracts/NibblVaultFactory.sol: L107

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

Recommendation:

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

Similarly for the three additional instances of '&&' within require functions referenced below:

contracts/NibblVaultFactory.sol: L131 contracts/NibblVaultFactory.sol: L149 contracts/NibblVaultFactory.sol: L166

Variables should not be initialized to their default values

For example, initializing uint variables to their default value of 0 is unnecessary and costs gas

The for loop counter i is initialized unnecessarily to zero in the six loops referenced below:

contracts/NibblVault.sol: L506-508 contracts/NibblVault.sol: L525-527 contracts/NibblVault.sol: L547-550 contracts/Basket.sol: L43-46 contracts/Basket.sol: L70-74 contracts/Basket.sol: L93-96

Example:

        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }

Change uint256 i = 0; to uint256 i; in all cases

Array length should not be looked up in every iteration of for loop

Since calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop

contracts/NibblVault.sol: L506-508

        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }

Recommendation:

        uint256 totalAssetAddresses = _assetAddresses.length; 
        for (uint256 i = 0; i < totalAssetAddresses; ++i) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }

Similarly for the five for loops referenced below:

contracts/NibblVault.sol: L525-527 contracts/NibblVault.sol: L547-550 contracts/Basket.sol: L43-46 contracts/Basket.sol: L70-74 contracts/Basket.sol: L93-96

Use of i++ to increase count in a for loop

Since use of i++ costs more gas, it would be better to use ++i in the six for loops referenced below:

contracts/NibblVault.sol: L506-508 contracts/NibblVault.sol: L525-527 contracts/NibblVault.sol: L547-550 contracts/Basket.sol: L43-46 contracts/Basket.sol: L70-74 contracts/Basket.sol: L93-96

Use of default "checked" behavior in for loop

Under/overflow checks are made every time i++ or (++i) is called. Such checks are unnecessary since i is already limited. Therefore, use unchecked{++i}/unchecked{i++} instead

contracts/NibblVault.sol: L506-508

        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }

Recommendation:

        for (uint256 i = 0; i < _assetAddresses.length;) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
            unchecked{ i++; }
        }

Similarly for the five additional for loops that could save gas by using unchecked:

contracts/NibblVault.sol: L525-527 contracts/NibblVault.sol: L547-550 contracts/Basket.sol: L43-46 contracts/Basket.sol: L70-74 contracts/Basket.sol: L93-96

For loop gas optimization example

While, for presentation purposes, I have separated the for loop-related gas savings methods, they should be combined. Below I show how all four of the gas saving methods outlined in this submission can be implemented together.

1. Don't initialize the loop counter (i) to zero
2. Read the array length before executing the loop
3. Use ++i rather than i++
4. Use unchecked ++i

contracts/NibblVault.sol: L506-508

        for (uint256 i = 0; i < _assetAddresses.length; i++) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
        }

Recommendation:

        uint256 totalAssetAddresses = _assetAddresses.length; 
        for (uint256 i; i < totalAssetAddresses;) {
            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
            unchecked{ ++i; }
        }

Storage of uints or ints smaller than 32 bytes

Storage of uints or ints smaller than 32 bytes incurs overhead. Instead, use size of at least 32, then downcast where needed

Recommendation: Use a larger size than uint8 in the four lines referenced below:

contracts/NibblVault.sol: L557 contracts/Twav/Twav.sol: L11 contracts/Twav/Twav.sol: L12 contracts/Twav/Twav.sol: L37