QA Report

[code423n4]

FINDINGS

Use of solidity's transfer() function might render ETH impossible to withdraw

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when :

1. The claimer smart contracts does not implement a payable function
2. The claimer smart contract does implement a payable function whcih uses more than 2300 gas unit
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy ,raising the call's gas usage above 2300

File: Basket.sol line 80

     function withdrawETH(address payable _to) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        _to.transfer(address(this).balance);
        emit WithdrawETH(_to);
    }

Note the line _to.transfer(address(this).balance);

Reccommendation Call() should be used instead of transfer() on an address payable

see consensys blog on transfer

Constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended. Solidity’s style guide. File: NibblVault.sol line 183

        uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));

File: NibblVault.sol line 195

        uint _primaryReserveBalance = (primaryReserveRatio * _initialTokenSupply * _initialTokenPrice) / (SCALE * 1e18);

File: NibblVault.sol line 226

        secondaryReserveRatio = uint32((secondaryReserveBalance * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice)); //secondaryReserveRatio is updated on every trade 

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File:AccessControlMechanism.sol line 4

pragma solidity ^0.8.0;

Most of the other contracts are using the following pragma directive

pragma solidity ^0.8.10;

Duplicated require()/revert() checks should be refactored to a modifier or function

File: NibblVault.sol line 496

        require(msg.sender == bidder,"NibblVault: Only winner");

The above check has been repeated several times as shown in the following lines

File: NibblVault.sol line 505 File: NibblVault.sol line 516 File: NibblVault.sol line 524 File: NibblVault.sol line 536 File: NibblVault.sol line 546

Other instances File: Basket.sol line 36

        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

The above check has been repeated in the following

File: Basket.sol line 42 File: Basket.sol line 53 File: Basket.sol line 62 File: Basket.sol line 69 File: Basket.sol line 79 File: Basket.sol line 86 File: Basket.sol line 92

Missing checks for address(0x0) when assigning values to address state variables( Immutable addresses should be 0-checked)

File: NibblVaultFactory.sol line 24

        vaultImplementation = _vaultImplementation;

File: NibblVaultFactory.sol line 25

        feeTo = _feeTo;

File: NibblVaultFactory.sol line 26

        basketImplementation = _basketImplementation;

File: NibblVaultFactory.sol line 100

        pendingBasketImplementation = _newBasketImplementation;

File: NibblVault.sol line 191

        assetAddress = _assetAddress;

File: /ProxyBasket.sol line 20

        implementation = payable(_implementation);

File: ProxyVault.sol line 20

        factory = payable(_factory);

File: NibblVault.sol line 487

        curator = _newCurator;

Consider adding zero-address checks in the above.

require(newAddr != address(0));

[HardlyDifficult]

A couple low risk & some NC suggestions. Well explained rationals.