Nibbl contest - catchup's results

Floating pragma and different compiler versions among contracts

All the contracts are using fixed 0.8.10 except for AccessControlMechanism.sol which is using floating pragma ^0.8.0. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. It is recommended to use the same solidity version for all the contracts.

References: https://swcregistry.io/docs/SWC-103 https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used

Each event can have up to three indexed fields.

Although indexed fields are used widely, still some events have less than 3 indexed fields.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L15-L19

Missing non-zero address checks when setting state variable addresses.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L100 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L124 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L159 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L176 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L178 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L485

Missing natspec comments

This is one of the best projects I've seen regarding the rich and explanatory natspec comments. There are just a few functions which are missing natspec comments.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L76 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L80 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L88

Array length equality checks missing

Some functions have 2 array parameters, length of which must be equal in order for the execution to complete properly. Therefore, these functions should check if the array lengths are equal. For example: withdrawMultipleERC721() function in Basket.sol

    function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
	require(_tokens.length == _tokenId.length, "array length mismatch");	//@audit array lengths equality check
        for (uint256 i = 0; i < _tokens.length; i++) {
            IERC721(_tokens[i]).safeTransferFrom(address(this), _to, _tokenId[i]);
            emit WithdrawERC721(_tokens[i], _tokenId[i], _to);
        }
    }

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L41 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L68 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L504 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L545

Unused import

NibblVaultFactory.sol imports SafeMath.sol, but it is not used.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L9

Missing non-zero address checks at token transfers

Tokens sent to zero address would be burned by mistake. It is a good practice to add non-zero check for the receiver address during token transfers.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L78 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L300 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L362 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L454 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L464 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L474 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L515 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523

Two-step process for updateCurator()

Consider changing the updateCurator() to a 2-step process. First, current curator sets the pending curator, then pending curator claims the curator role. This way, the risk of an accidental wrong assignment can be eliminated.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L485-L488

for loops can be optimized

1- For loop index increments can be made unchecked for solidity versions > 0.8.0. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint variable to 0 since default value is already 0.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L43 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L70 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L93 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L506 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L525 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L547

Recommended Mitigation Steps

I suggest to change the original code from this

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

to this

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

Error string longer than 32 characters

Error reason strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L48 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L49 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L107 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L131 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L141 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L149 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L166

It is better to use calldata for read-only reference type parameters

Read-only reference type function parameters can be made calldata rather than memory. Calldata is more gas efficient since it avoids copying the parameters.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L41 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L68 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L91 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L109 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L174-L175 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L504 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L545 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L80

basketUpdateTime can be cached

basketUpdateTime is used twice in the require statement. It can be cached before the require and used from stack to save an SLOAD.

    function updateBasketImplementation() external override {
	uint256 _basketUpdateTime = basketUpdateTime;
        require(_basketUpdateTime != 0 && block.timestamp >= _basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
        basketImplementation = pendingBasketImplementation;
        delete basketUpdateTime;
    }

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L107

feeToUpdateTime can be cached

feeToUpdateTime is used twice in the require statement. It can be cached before the require and used from stack to save an SLOAD.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L131

feeAdminUpdateTime can be cached

feeAdminUpdateTime is used twice in the require statement. It can be cached before the require and used from stack to save an SLOAD.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L149

vaultUpdateTime can be cached

vaultUpdateTime is used twice in the require statement. It can be cached before the require and used from stack to save an SLOAD.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L166

Constant keccak variables can be changed to immutable

When variables are defined as constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed only during deployment. If the variable is going to be used within the constructor, the value assignment of the immutable variable should be executed within the constructor as well. See below for previous findings. https://github.com/code-423n4/2021-10-slingshot-findings/issues/3 https://github.com/code-423n4/2022-01-livepeer-findings/issues/172

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Utilities/AccessControlMechanism.sol#L12-L14

Prefix increment/decrements are cheaper than postfix

nonces[owner]++ can be changed to postfix increment ++nonces[owner]. Same goes for all of the for loop index increments.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L562

Missing non-zero amount checks

It is a good practice to apply non-zero amount checks for token transactions to avoid unnecessary executions.

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L213 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L237 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L275 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L287 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L362

Execution of strict inequalities (>) are cheaper than non-strict ones (>=)

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =. Therefore it is more efficient to change this:

	require(msg.value >= MIN_INITIAL_RESERVE_BALANCE)

to this:

	require(msg.value > MIN_INITIAL_RESERVE_BALANCE)

and change the definition of MIN_INITIAL_RESERVE_BALANCE to: uint256 private constant MIN_INITIAL_RESERVE_BALANCE = 1e9 - 1;

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L48 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L107 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L131 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L141 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L149 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L166