Nibbl contest - 0x29A's results

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.

Gas Report

[G-01] CALLDATA INSTEAD OF MEMORY FOR FUNCTION PARAMETERS

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

Instances include:

• Basket.sol:

L41: 
- function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {
+ function withdrawMultipleERC721(address[] calldata _tokens, uint256[] calldata _tokenId, address _to) external override {
L68: 
- function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external override {
+ function withdrawMultipleERC1155(address[] calldata _tokens, uint256[] calldata _tokenIds, address _to) external override {
L91:
- function withdrawMultipleERC20(address[] memory _tokens) external override {
+ function withdrawMultipleERC20(address[] calldata _tokens) external override {
L99:
- function onERC721Received(address, address from, uint256 id, bytes memory) external override returns(bytes4) {
+ function onERC721Received(address, address from, uint256 id, bytes calldata) external override returns(bytes4) {
L104:
- function onERC1155Received(address, address from, uint256 id, uint256 amount, bytes memory) external virtual override returns (bytes4) {
+ function onERC1155Received(address, address from, uint256 id, uint256 amount, bytes calldata) external virtual override returns (bytes4) {
L109:
- function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {
+ function onERC1155BatchReceived(address, address from, uint256[] calldata ids, uint256[] calldata amounts, bytes calldata) external virtual override returns (bytes4) {

• IBasket.sol:

L12:
- function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external;
+ function withdrawMultipleERC721(address[] calldata _tokens, uint256[] calldata _tokenId, address _to) external;
L15:
- function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external;
+ function withdrawMultipleERC721(address[] calldata _tokens, uint256[] calldata _tokenId, address _to) external;
L18:
- function withdrawMultipleERC20(address[] memory _tokens) external;
+ function withdrawMultipleERC20(address[] calldata _tokens) external;

• INibblVault.sol:

L15:
- function initialize(string memory _tokenName, string memory _tokenSymbol, address _assetAddress, uint256 _assetID, address _curator, uint256 _initialTokenSupply, uint256 _initialTokenPrice, uint256 _minBuyoutTime) external payable;
+ function initialize(string calldata _tokenName, string calldata _tokenSymbol, address _assetAddress, uint256 _assetID, address _curator, uint256 _initialTokenSupply, uint256 _initialTokenPrice, uint256 _minBuyoutTime) external payable;
L25:
- function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external;
+ function withdrawMultipleERC721(address[] calldata _assetAddresses, uint256[] calldata _assetIDs, address _to) external;
L27:
- function withdrawMultipleERC20(address[] memory _assets, address _to) external;
+ function withdrawMultipleERC20(address[] calldata _assets, address _to) external;
L29:
- function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external;
+ function withdrawMultipleERC1155(address[] calldata _assets, uint256[] calldata _assetIDs, address _to) external;

• INibblVaultFactory.sol:

L18:
- function createBasket(address _curator, string memory _mix) external returns(address);
+ function createBasket(address _curator, string calldata _mix) external returns(address);
L19:
- function getBasketAddress(address _curator, string memory _mix) external view returns(address);
+ function getBasketAddress(address _curator, string calldata _mix) external view returns(address);

• NibblVault.sol:

L174:
- string memory _tokenName,
+ string calldata _tokenName,
L175:
- string memory _tokenSymbol,
+ string calldata _tokenSymbol,
L504:
- function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {
+ function withdrawMultipleERC721(address[] calldata _assetAddresses, uint256[] calldata _assetIDs, address _to) external override boughtOut {
L523:
- function withdrawMultipleERC20(address[] memory _assets, address _to) external override boughtOut {
+ function withdrawMultipleERC20(address[] calldata _assets, address _to) external override boughtOut {
L545:
- function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {
+ function withdrawMultipleERC1155(address[] calldata _assets, uint256[] calldata _assetIDs, address _to) external override boughtOut {
L577:
- function onERC1155Received(address, address, uint256, uint256, bytes memory) external pure returns (bytes4) {
+ function onERC1155Received(address, address, uint256, uint256, bytes calldata) external pure returns (bytes4) {
L581:
- function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) external pure returns (bytes4) {
+ function onERC1155BatchReceived(address, address, uint256[] calldata, uint256[] calldata, bytes calldata) external pure returns (bytes4) {

• NibblVaultFactory.sol:

L80:
- function createBasket(address _curator, string memory _mix) public override returns(address)  {
+ function createBasket(address _curator, string calldata _mix) public override returns(address)  {
L88:
- function getBasketAddress(address _curator, string memory _mix) public override view returns(address _basket) {
+ function getBasketAddress(address _curator, string calldata _mix) public override view returns(address _basket) {

[G-02] CACHING LENGTH AND VARS USED MORE THAN ONE TIME ON FOR

Caching the array length is more gas efficient(only in memory and storage) Caching variables used more than one time is more gas efficient

Instances include:

• Basket.sol: L43-L46, From:

for (uint256 i = 0; i < _tokens.length; i++) {
    IERC721(_tokens[i]).safeTransferFrom(address(this), _to, _tokenId[i]);
    emit WithdrawERC721(_tokens[i], _tokenId[i], _to);
}

To:

uint256 tokensLength = _tokens.length;

for (uint256 i = 0; i < tokensLength; i++) {
    uint256 tokenId = _tokenId[i];
    address token = _tokens[i];

    IERC721(token).safeTransferFrom(address(this), _to, tokenId);
    emit WithdrawERC721(token, tokenId, _to);
}

L70-L74, From:

for (uint256 i = 0; i < _tokens.length; i++) {
    uint256 _balance = IERC1155(_tokens[i]).balanceOf(address(this),  _tokenIds[i]);
    IERC1155(_tokens[i]).safeTransferFrom(address(this), _to, _tokenIds[i], _balance, "0");
    emit WithdrawERC1155(_tokens[i], _tokenIds[i], _balance, _to);
}

To:

uint256 tokensLength = _tokens.length;

for (uint256 i = 0; i < tokensLength; i++) {
    uint256 tokenId = _tokenIds[i];
    address token = _tokens[i];

    uint256 _balance = IERC1155(token).balanceOf(address(this),  tokenId);
    IERC1155(token).safeTransferFrom(address(this), _to, tokenId, _balance, "0");
    emit WithdrawERC1155(token, tokenId, _balance, _to);
}

L93-L96, From:

for (uint256 i = 0; i < _tokens.length; i++) {
    IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
    emit WithdrawERC20(_tokens[i], msg.sender);
}

To:

uint256 tokensLength = _tokens.length;

for (uint256 i = 0; i < tokensLength; i++) {
    address token = _tokens[i];

    IERC20(token).transfer(msg.sender, IERC20(token).balanceOf(address(this)));
    emit WithdrawERC20(token, msg.sender);
}

• NibblVault.sol: L506-L508, From:

for (uint256 i = 0; i < _assetAddresses.length; i++) {

To:

uint256 assetAddressesLength = _assetAddresses.length;

for (uint256 i = 0; i < assetAddressesLength; i++) {

L525-L527, From:

for (uint256 i = 0; i < _assets.length; i++) {
    IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));
}

To:

uint256 assetsLength = _assets.length;

for (uint256 i = 0; i < assetsLength; i++) {
    address asset = _assets[i];

    IERC20(asset).transfer(_to, IERC20(asset).balanceOf(address(this)));
}

L547-L550, From:

for (uint256 i = 0; i < _assets.length; i++) {
    uint256 balance = IERC1155(_assets[i]).balanceOf(address(this),  _assetIDs[i]);
    IERC1155(_assets[i]).safeTransferFrom(address(this), _to, _assetIDs[i], balance, "0");
}

To:

uint256 assetsLength = _assets.length;

for (uint256 i = 0; i < assetsLength; i++) {
    address asset = _assets[i];
    uint256 _assetID = _assetIDs[i];

    uint256 balance = IERC1155(asset).balanceOf(address(this),  _assetID);
    IERC1155(asset).safeTransferFrom(address(this), _to, _assetID, balance, "0");
}

[G-03] USE unchecked FOR OPERATIONS NOT EXPECTED TO OVERFLOW

for (uint256 i; i < array.length;) {
    ...
    unchecked {
        ++i;
    }
}

Instances include:

• Basket.sol: L43, L70, L93
• NibblVault.sol: L506, L525, L547