QA Report

[code423n4]

QA & NON CRITICAL ERRORS

AccessControlMechanism.sol

[QA] Lint error

Remove blank line on AccessControlMechanism.sol#L6 Add blank line between L7 and L8

[L] Missing address(0) validation on _admin parameter

Add validation to _admin parameter on constructor to avoid admin to be address(0)

[L] Missing function to removeGrantRole, (opposite to proposeGrantRole)

There is a function to proposeGrantRole, but there is no way to remove the propose of role. https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Utilities/AccessControlMechanism.sol#L40

[QA] Unnecesary override of functions

Consider remove the override from AccessControlMechanism.sol#L32 AccessControlMechanism.sol#L40 AccessControlMechanism.sol#L47

[QA] Variable UPDATE_TIME should be a constant

On NibblVaultFactoryData.sol#L6 variable UPDATE_TIME should be a constant.

NibblVaultFactory.sol

[QA] Add address to salt

Add address to salt on NibblVaultFactory.sol:L50

Change

_proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));

to

_proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(address(this)_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));

[QA] Double payable casting

On NibblVaultFactory.sol#L50-L51 there is no need to recast to payable. Change;

        _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));
        NibblVault _vault = NibblVault(payable(_proxyVault));

To

        _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));
        NibblVault _vault = NibblVault(_proxyVault);

Non-critical

[N-01] THE CONTRACTS USE UNLOCKED PRAGMA

AccessControlMechanism.sol and IAccessControlMechanism.sol use pragma solidity ^0.8.0; change to pragma solidity 0.8.10;

[N-02] TYPO

In Basket.sol, L23: initialise to initialize In IBasket.sol, L10: initialise to initialize In NibblVaultFactory.sol, L83: initialise to initialize

In Basket.sol#L41, Basket.sol#L44, Basket.sol#L45: _tokenId to _tokenIds In IBasket.sol, L12: _tokenId to _tokenIds

In ProxyVault.sol, L26: internall to internal In ProxyBasket.sol, L26: internall to internal

In Basket.sol#L58; change natspec comment from /// @notice withdraw an ERC721 token from this contract into your wallet to /// @notice withdraw an ERC1155 token from this contract into your wallet

[N-03] LINT

In ProxyVault.sol:

L56:
- receive() external payable {    }
+ receive() external payable {}
L58:
-    }
+}

In NibblVaultFactory.sol:

L47:
-        ) external payable override whenNotPaused returns(address payable _proxyVault) {
+    ) external payable override whenNotPaused returns(address payable _proxyVault) {
L69:
-        uint256 _initialTokenPrice) public view returns(address _vault) {
+        uint256 _initialTokenPrice
+    ) public view returns(address _vault) {

[N-04] EVENTS EXTRA INDEXED

There are events with extra indexed parameters, the indexed used off-chain to filter, why filter with this parameters?:

• Basket.sol:

  
  L20:

• event WithdrawETH(address indexed who);
• event WithdrawETH(address who); L21:
• event WithdrawERC20(address indexed token, address indexed who);
• event WithdrawERC20(address indexed token, address who);

• INibblVault.sol:

  
  L9:

• event BuyoutInitiated(address indexed bidder, uint256 indexed bid);
• event BuyoutInitiated(address indexed bidder, uint256 bid); L10:
• event BuyoutRejected(uint256 indexed rejectionValuation);
• event BuyoutRejected(uint256 rejectionValuation); L11:
• event CuratorFeeUpdated(uint256 indexed fee);
• event CuratorFeeUpdated(uint256 fee); L12:
• event Buy(address indexed buyer, uint256 indexed continousTokenAmount, uint256 indexed reserveTokenAmt);
• event Buy(address indexed buyer, uint256 continousTokenAmount, uint256 reserveTokenAmt); L13:
• event Sell(address indexed seller, uint256 indexed continousTokenAmount, uint256 indexed reserveTokenAmt);
• event Sell(address indexed seller, uint256 continousTokenAmount, uint256 reserveTokenAmt);

[N-05] USE A COMMON PATTERN

I recommend use the pattern of the EIP721 from-to-tokenId and EIP1155 operator-from-to-id-amount

Instances include:

• IBasket.sol:

  
  L11:

• function withdrawERC721(address _token, uint256 _tokenId, address _to) external;

• function withdrawERC721(address _token, address _to, uint256 _tokenId) external; L12:

• function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external;

• function withdrawMultipleERC721(address[] memory _tokens, address _to, uint256[] memory _tokenId) external; L13:

• function withdrawERC721Unsafe(address _token, uint256 _tokenId, address _to) external;

• function withdrawERC721Unsafe(address _token, address _to, uint256 _tokenId) external; L14:

• function withdrawERC1155(address _token, uint256 _tokenId, address _to) external;

• function withdrawERC1155(address _token, address _to, uint256 _tokenId) external; L15:

• function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external;

• function withdrawMultipleERC1155(address[] memory _tokens, address _to, uint256[] memory _tokenIds) external;

• Basket.sol:

  
  L15:

• event DepositERC721(address indexed token, uint256 tokenId, address indexed from);

• event DepositERC721(address indexed token, address indexed from, uint256 tokenId); L16:

• event WithdrawERC721(address indexed token, uint256 tokenId, address indexed to);

• event WithdrawERC721(address indexed token, address indexed to, uint256 tokenId); L17:

• event DepositERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);

• event DepositERC1155(address indexed token, address indexed from, uint256 tokenId, uint256 amount); L18:

• event DepositERC1155Bulk(address indexed token, uint256[] tokenId, uint256[] amount, address indexed from);

• event DepositERC1155Bulk(address indexed token, address indexed from, uint256[] tokenId, uint256[] amount); L19:

• event WithdrawERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);

• event WithdrawERC1155(address indexed token, address indexed from, uint256 tokenId, uint256 amount); L35:

• function withdrawERC721(address _token, uint256 _tokenId, address _to) external override {

• function withdrawERC721(address _token, address _to, uint256 _tokenId) external override { L41:

• function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {

• function withdrawMultipleERC721(address[] memory _tokens, address _to, uint256[] memory _tokenId) external override { L52:

• function withdrawERC721Unsafe(address _token, uint256 _tokenId, address _to) external override {

• function withdrawERC721Unsafe(address _token, address _to, uint256 _tokenId) external override { L61:

• function withdrawERC1155(address _token, uint256 _tokenId) external override {

• function withdrawERC1155(address _token, address _to, uint256 _tokenId, address _to) external override { L68

• function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external override {

• function withdrawMultipleERC1155(address[] memory _tokens, address _to, uint256[] memory _tokenIds) external override {

• INibblVaultFactory.sol:

  
  L15:

• event DepositERC721(address indexed token, uint256 tokenId, address indexed from);

• event DepositERC721(address indexed token, address indexed from, uint256 tokenId); L16:

• event WithdrawERC721(address indexed token, uint256 tokenId, address indexed to);

• event WithdrawERC721(address indexed token, address indexed to, uint256 tokenId); L17:

• event DepositERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);

• event DepositERC1155(address indexed token, address indexed from, uint256 tokenId, uint256 amount); L18:

• event DepositERC1155Bulk(address indexed token, uint256[] tokenId, uint256[] amount, address indexed from);

• event DepositERC1155Bulk(address indexed token, address indexed from, uint256[] tokenId, uint256[] amount); L19:

• event WithdrawERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);

• event WithdrawERC1155(address indexed token, address indexed from, uint256 tokenId, uint256 amount);

Order the parameters from more important to less

• INibblVaultFactory.sol:

• event Fractionalise(address assetAddress, uint256 assetTokenID, address proxyVault);
• event Fractionalise(address proxyVault, address assetAddress, uint256 assetTokenID);

[N-06] UNUSED IMPORT

In NibblVaultFactory.sol, this imports are unused:

• L5: import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
• L6: import { IERC1155 } from "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";
• L9: import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";

In ProxyBasket.sol, this imports are unused:

• L4: import { NibblVaultFactory } from "../NibblVaultFactory.sol";

In Basket.sol, ERC165 is imported but not used, only import IERC165

• L7: import { IERC165, ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol";

[N-07] MISSING ERROR MESSAGES IN REQUIRE STATEMENTS

In NibblVaultFactory.sol, L114: require(_success);

Low Risk

[L-01] EVENTS ARE NOT INDEXED

The emitted events are not indexed, making off-chain scripts such as front-ends of dApps difficult to filter the events efficiently.

Recommended Mitigation Steps: Add the indexed keyword in filter parameters of the events

Instances include:

• Basket.sol:

  
  L15:

• event DepositERC721(address indexed token, uint256 tokenId, address indexed from);
• event DepositERC721(address indexed token, uint256 indexed tokenId, address indexed from); L16:
• event WithdrawERC721(address indexed token, uint256 tokenId, address indexed to);
• event WithdrawERC721(address indexed token, uint256 indexed tokenId, address indexed to); L17:
• event DepositERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);
• event DepositERC1155(address indexed token, uint256 indexed tokenId, uint256 amount, address indexed from); L19:
• event WithdrawERC1155(address indexed token, uint256 tokenId, uint256 amount, address indexed from);
• event WithdrawERC1155(address indexed token, uint256 indexed tokenId, uint256 amount, address indexed from);

• INibblVaultFactory.sol:

  
  L5:

• event Fractionalise(address assetAddress, uint256 assetTokenID, address proxyVault);
• event Fractionalise(address indexed assetAddress, uint256 indexed assetTokenID, address indexed proxyVault);

[L-02] DUPLICATE CONTRACTS

ProxyBasket.sol and ProxyVault.sol are the same contracts with different name and only the L30 it's different, unify both

[L-03] Missing input validations on arrays

On Basket.sol#L41 and Basket.sol#L68 if the array length of _tokens, _tokenId is not equal, it can lead to an error.

On NibblVault.sol#L504 if the array length of _assetAddresses, _assetIDs is not equal, it can lead to an error.

On NibblVault.sol#L545 if the array length of _assets, _assetIDs is not equal, it can lead to an error.

Recommend checking the input array length.

[HardlyDifficult]

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

[HardlyDifficult]

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

[HardlyDifficult]

Good suggestions - targeted & clear recommendations.