QA Report

[code423n4]

1.Use a single modifier to check the same condition in mutliple functions

• Multiple instances of _isApprovedOrOwner() can be checked using a single modifier in basket.sol

modifier isApprovedOnwer() {
    require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed")
}

function withdrawERC721(....) isApprovedOnwer{ ..}
function withdrawMultipleERC721(....) isApprovedOnwer{ ..}
function withdrawERC721Unsafe(....) isApprovedOnwer{ ..}
function withdrawERC1155(....) isApprovedOnwer{ ..}
........
......

• same in NibblVault.sol for require(msg.sender == bidder,"NibblVault: Only winner")s

2. Add descriptive revert string in "require()"

Some require statements dont have revert strings, using short descriptive message is good practice

./contracts/NibblVaultFactory.sol:114:        require(_success);

./contracts/Bancor/BancorFormula.sol:259:        require(_baseN < MAX_NUM);
./contracts/Bancor/BancorFormula.sol:356:        require(false);

3. Unbounded loops with external calls

Some external function take arrays from users and iterate thorugh them. If a user sends very large loop the transaction may be too big to fit in a single block. Checking for a resonable array size in such case is a best pracitce.

• In withdrawMultipleERC1155() , withdrawMultipleERC20(), etc..

4. Address(0) checks while setting adderss in initialize() in NibblVault.sol

Check for zero address in the initialize function, so it doesn't gets set to 0 accidently.

5. Address(0) checks for withdraw methods for ERC20, ERC721 and ERC1155

The token withdraw methods for ERC20, ERC721 and ERC1155 take _to address to which these tokens are transferred, but this address may be accidently sent to zero address and the tokens may be lost forever.

• In withdrawUnsettledBids, redeem, redeemCuratorFee, updateCurator, withdrawERC721, withdrawMultipleERC721, withdrawERC20, withdrawMultipleERC20, withdrawERC1155, withdrawMultipleERC1155

This is very important for critical role changes such as updateCurator()

6. buy() function is not making any external calls, so reentrancy guard lock maynot be needed

• In NibblVault.sol#L300

7. Unused receive() should revert

• If the intention is for the Ether to be used, the user should call another function, otherwise it should revert.

8. floating pragma in AccessControlMechanism.sol

• 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.
• pragma solidity ^0.8.0;

[mundhrakeshav]

https://github.com/code-423n4/2022-06-nibbl-findings/issues/2, https://github.com/code-423n4/2022-06-nibbl-findings/issues/3, https://github.com/code-423n4/2022-06-nibbl-findings/issues/6, https://github.com/code-423n4/2022-06-nibbl-findings/issues/7, https://github.com/code-423n4/2022-06-nibbl-findings/issues/8, https://github.com/code-423n4/2022-06-nibbl-findings/issues/15

[HardlyDifficult]

Merging with https://github.com/code-423n4/2022-06-nibbl-findings/issues/248

[HardlyDifficult]

Merging with https://github.com/code-423n4/2022-06-nibbl-findings/issues/253

[HardlyDifficult]

Good & relevant feedback. Low & NC suggestions.