QA Report

[code423n4]

1. NibblVault sell is missing reentrancy protection

Impact

Function NibbleVault.sell is missing lock modifier that protects against reentrancy attacks.

Proof of Concept

NibblVault.sol:

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L362

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add lock modifier to sell function.

2. Confusing intialize vs initialise

Risk

Low

Impact

Protocol uses proxy pattern for vaults and baskets which requires properly initializing the contracts. The issue is that contract NibblVault implements function initialize and contract Basket.sol implements initialise. Using different naming convention for critical functionality such as initialization makes it prone to errors and leads to vulnerabilities.

Since openzeppelin goes with initializer modifier we recommend using initialize for Basket.sol.

Proof of Concept

• NibblVault uses initialize - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L173
• Basket uses initialise - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L23

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommend to change function Basket.initialise to Basket.initialize.

3. NibblVault incorrect check

Risk

Low

Impact

Contract NibblVault charges admin fee in _chargeFee and _chargeFeeSecondaryCurve:

        if(_adminFeeAmt > 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

The check is invalid since there might be edge case (when amount is very small) that _adminFeeAmt is positive and _feeAdmin is equal to 0. ETH should be sent when _feeAdmin is positive.

Proof of Concept

NibblVault.sol:

• Incorrect check in _chargeFee - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L227-L229
• Incorrect check in _chargeFeeSecondaryCurve - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L243-L245

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to change the check to:

        if(_feeAdmin > 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

or even better after gas optimization to:

        if(_feeAdmin != 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

4. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

NibblVaultFactory.sol:

• Missing checks for _vaultImplementation, _feeTo, _admini and _basketImplementation - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L23
• Missing check for _curator - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L40
• Missing check for _curator - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L80
• Missing check for _newBasketImplementation - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L99
• Missing check for _newFeeAddress - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L123
• Missnig check for _newVaultImplementation - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L158

NibblVault.sol:

• Missing check in initialize for _assetAddress - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L176
• Missing check in initialize for _curator - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L178
• Missing check in buy for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L300
• Missing check in sell for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L362
• Missing check in withdrawUnsettledBids for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L454
• Missing check in redeem for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L464
• Missing check in redeemCuratorFee for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L474
• Missing check in updateCurator for _newCurator - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L485
• Missing check in withdrawERC721 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L495
• Missing check in withdrawMultipleERC721 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L504
• Missing check in withdrawERC20 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L515
• Missing check in withdrawMultipleERC20 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L523
• Missing check in withdrawERC1155 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L535
• Missing check in withdrawMultipleERC1155 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L545
• Missing check in safeTransferETH for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L568

Basket.sol:

• Missing check in initialise for _curator - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L23
• Missing check in withdrawERC721 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L35
• Missing check in withdrawERC721Unsafe for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L52
• Missing check in withdrawERC1155 for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L61
• Missing check in withdrawETH for _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L78

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

5. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

NibblVaultFactory.sol:

• Missing proposeNewBasketImplementation function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L99
• Missing updateBasketImplementation function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L106
• Missing withdrawAdminFee function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L112
• Missing proposeNewAdminFeeAddress function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L123
• Missing updateNewAdminFeeAddress function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L130
• Missing proposeNewAdminFee function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L140
• Missing updateNewAdminFee function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L148 8 Missing proposeNewVaultImplementation function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L158
• Missing updateVaultImplementation function event - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L165

NibblVault.sol:

• Missing withdrawUnsettledBids function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L454
• Missing redeem function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L464
• Missing redeemCuratorFee function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L474
• Missing updateCurator function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L485
• Missing withdrawERC721 function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L495
• Missing withdrawERC20 function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L515
• Missing withdrawERC1155 function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L535
• Missing permit function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L552

Twav.sol:

• Missing _updateTWAV function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Twav/Twav.sol#L21

AccessControlMechanism.sol:

• Missing proposeGrantRole function event - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/AccessControlMechanism.sol#L40-L42

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

6. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

NibblVault.sol:

• Change of curator - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L485-L488

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing curator address.

7. Missing indexing for events

Risk

Non-Critical

Impact

Events should index addresses which helps off-chain applications in monitoring the protocol.

Proof of Concept

Interfaces/INibblVaultFactory.sol:5:    event Fractionalise(address assetAddress, uint256 assetTokenID, address proxyVault);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add indexing to address type parameters.

8. The contracts use unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

• (^0.8.0) Utilities/AccessControlMechanism.sol
• (^0.8.0) Interfaces/IAccessControlMechanism.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Consider locking compiler version, for example pragma solidity 0.8.10.

9. Misleading constant variable name

Risk

Non-Critical

Impact

Contract NibblVault implement constant variable primaryReserveRatio. The variable is constant so it should follow the naming convention for constant variables PRIMARY_RESERVE_RATIO.

Proof of Concept

NibblVault.sol:

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L28

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to change name of primaryReserveRatio to PRIMARY_RESERVE_RATIO.

10. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

NibblVaultFactory.sol:

• Missing @return - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L29-L38
• Missing @return - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L58-L64
• Missing natspec - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L76
• Missing natspec - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L80
• Missing natspec - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L88
• Missing natspec - https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVaultFactory.sol#L112

NibblVault.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L128
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L249-L252
• Missing @param _totalSupply - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L269-L275
• Missing @param _totalSupply - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L281-L287
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L294-L300
• Missing @param _totalSupply - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L330-L335
• Missing @param _totalSupply - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L342-L347
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L355-L362
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L393-L398
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L461-L464
• Missing @return - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L471-L474
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L552
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L568
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L573
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L577
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L581

Basket.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L23
• Missing @param _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L32-L35
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L41
• Missing @param _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L49-L52
• Missing @param _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L58-L61
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L68
• Missing @param _to - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L77-L78
• Missing @param _token - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L84-L85
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L91
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L99
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L104
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L109

Twav.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Twav/Twav.sol#L44

ProxyVault.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Proxy/ProxyVault.sol#L19

ProxyBasket.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Proxy/ProxyBasket.sol#L19

AccessControlMechanism.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/AccessControlMechanism.sol#L19

EIP721Base.sol:

• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L15
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L27
• Missing natspec - https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L40

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

[HardlyDifficult]

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

[HardlyDifficult]

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

[HardlyDifficult]

Great stuff, very clear report