QA Report

[code423n4]

Impact

Detailed description of the impact of this finding.

A user can initialize a vault as the curator to 0th address, thus being unable to retrieve their funds. They can also list an erronenous asset address. There should be some basic input validation on the parameters of these functions to account to prevent frontend errors calling this function with nonsensical inputs.

This is also present here, which should do some input validation against the zero address.

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L83

Proof of Concept

OpenZepellin requires zero address checks in their codebase

https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222

Tools Used

Manual Review

Recommended Mitigation Steps

[eugenioclrc]

I dont think so, lets say _curator is address(0), then _basket.initialise(_curator); will be minting to address(0); Basket.sol#L24 and minting to address(0) will trigger a revert from OpenZeppellin ERC721 lib; https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L280

[HardlyDifficult]

I dont think so, lets say _curator is address(0), then _basket.initialise(_curator); will be minting to address(0); Basket.sol#L24 and minting to address(0) will trigger a revert from OpenZeppellin ERC721 lib; https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L280

Agree. Also erronenous asset address should revert on the transfer. Closing this report as invalid.