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 #107

[HardlyDifficult]

Merging with #108

[HardlyDifficult]

Good suggestions - targeted & clear recommendations.