Nibbl contest - cryptphi's results

1. Unbounded loops The following contains a for loop that can be unbounded. This would mean it could run out of gas and the function would revert

**Occurrences in: 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/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

2. Missing input validation on array lengths The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior.

**Occurences in: NibblVault.withdrawMultipleERC721() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L504 NibblVault.withdrawMultipleERC1155() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L545 NibblVault.onERC1155BatchReceived() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L545 Basket.withdrawMultipleERC721() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L41 Basket.withdrawMultipleERC1155() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L68 Basket.onERC1155BatchReceived() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L109

3. Missing validation check for non-zero totalSupply() NibblVault.redeem() doesn't check for a non-zero totalSupply to prevent a divide-by-zero issue in https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L466

4. Use of unchecked transfer() The following are functions make use of unsafe transfer() which does not check the return value.

**Occurrences in: Basket.withdrawERC20() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L87 Basket.withdrawMultipleERC20() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L94 NibblVault.withdrawERC20() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L517 NibblVault.withdrawMultipleERC20() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L526 Basket.withdrawETH() - https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L80

5. Missing zero address check during transfers The following functions are missing a zero address check to prevent transfer of ETH or ERC to zero address

**Occurrences in: https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L454-L459 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L464-L469 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L474-L479 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L504-L509 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L495-L498 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L362-L389 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L300-L326 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L515-L518 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L523-L528 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L535-L539 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L545-L550 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L35-L37 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L41-L44 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L52-L54 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L61-L64 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Basket.sol#L68-L72

6. Missing zero address check can make curator lose fee The updateCurator() function in https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L485-L488 is missing a require check to ensure the _to parameter is not a zero address. Current function could have the curator address changed to 0 address by mistake and with that the accrued curator fees will not be redeemable.

7. Missing zero address check on new Basket Implementation proposeNewBasketImplementation() sets the address for a proposed new Basket Implementation, however fails to check _newBasketImplementation is not a zero address. This can allow IMPLEMENTER_ROLE to mistakenly set the proposed new Basket Implementation to address zero and call updateBasketImplementation() to effect the change.

**Occurrence in: https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L99-L100

8.  Missing zero address check on new Admin Fee address proposeNewAdminFeeAddress() sets the address for a proposed  new Admin Fee address, however fails to check _newFeeAddress is not a zero address. TThis can allow IMPLEMENTER_ROLE to mistakenly set the proposed  new Admin Fee address to address zero and the call updateNewAdminFeeAddress() to effect the change.

**Occurrence in: https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L123

9. Missing zero address check on new Vault plementationproposeNewVaultImplementation() sets the address for a proposed new Vault Implementation, however fails to check _newVaultImplementation is not a zero address. This can allow IMPLEMENTER_ROLE to mistakenly set the proposed new Vault Implementation to address zero and call updateVaultImplementation() to effect the change.

**Occurrence in: https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L158

10. Missing zero address check and zero value check in constructor https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L23

11. Contract can lock ETH. Some contracts' receive() function allows anyone to send ETH to the contract since it's missing a require check https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L183 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Proxy/ProxyVault.sol#L56 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Proxy/ProxyBasket.sol#L56

1. Caching array length can save gas Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory

**Occurrences in: 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/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

2. The following variables should be cached to save gas _amtIn in NibblVault.sell() _amount in NibblVault._chargeFeeSecondaryCurve() _amount in NibblVaul._chargeFee() _tokens in Basket.withdrawMultipleERC721() _token in Basket.withdrawERC1155() _tokens in Basket.withdrawMultipleERC1155() _tokens in Basket.withdrawMultipleERC20()