Nibbl contest - IllIllI's results

Summary

Low Risk Issues

Total: 22 instances over 9 issues

Non-critical Issues

Total: 109 instances over 20 issues

Low Risk Issues

1. Buyouts that occur during the timestamp wrap will have valuation errors

The _blockTimestamp has a modulo applied, so at some point, there will be a timestamp with a value close to 2^32, followed by a timestamp close to zero. The _updateTWAV function does an unchecked subtraction of the two timestamps, so this will lead to an underflow, making the valuation based on a long time period rather than the actual one. Until more TWAV entries are added, valuations will be wrong

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

303              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
304              if (_blockTimestamp != lastBlockTimeStamp) {
305:                 _updateTWAV(getCurrentValuation(), _blockTimestamp);   

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

2. ecrecover() not checked for signer address of zero

The ecrecover() function returns an address of zero when the signature does not match. This can cause problems if address zero is ever the owner of assets, and someone uses the permit function on address zero. If that happens, any invalid signature will pass the checks, and the assets will be stealable. In this case, the asset of concern is the vault's ERC20 token, and fortunately OpenZeppelin's implementation does a good job of making sure that address zero is never able to have a positive balance. If this contract ever changes to another ERC20 implementation that is laxer in its checks in favor of saving gas, this code may become a problem.

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

563:         address signer = ecrecover(toTypedMessageHash(structHash), v, r, s);

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

3. Return values of transfer()/transferFrom() not checked

Not all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

There are 4 instances of this issue:

File: contracts/Basket.sol   #1

87:           IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L87

File: contracts/Basket.sol   #2

94:               IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L94

File: contracts/NibblVault.sol   #3

517:          IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));

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

File: contracts/NibblVault.sol   #4

526:              IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));

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

4. Input array lengths may differ

If the caller makes a copy-paste error, the lengths may be mismatchd and an operation believed to have been completed may not in fact have been completed

There are 4 instances of this issue:

File: contracts/Basket.sol   #1

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L41

File: contracts/Basket.sol   #2

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L68

File: contracts/NibblVault.sol   #3

545:     function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {

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

File: contracts/NibblVault.sol   #4

504:     function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {

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

5. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

There is 1 instance of this issue:

File: contracts/Basket.sol   #1

24:           _mint(_curator, 0);

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L24

6. Missing checks for address(0x0) when assigning values to address state variables

There are 6 instances of this issue:

File: contracts/NibblVault.sol

191:          assetAddress = _assetAddress;

193:          curator = _curator;

487:          curator = _newCurator;

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

File: contracts/NibblVaultFactory.sol

100:         pendingBasketImplementation = _newBasketImplementation;

124:         pendingFeeTo = _newFeeAddress;

159:         pendingVaultImplementation = _newVaultImplementation;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L100

7. Vulnerable to cross-chain replay attacks due to static DOMAIN_SEPARATOR/domainSeparator

See this issue from a prior contest for details

There is 1 instance of this issue:

File: contracts/Utilities/EIP712Base.sol   #1

15       function INIT_EIP712(string memory name, string memory version) internal {
16           domainSeperator = keccak256(
17               abi.encode(
18                   EIP712_DOMAIN_TYPEHASH,
19                   keccak256(bytes(name)),
20                   keccak256(bytes(version)),
21                   getChainID(),
22                   address(this)
23               )
24           );
25:      }

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L15-L25

8. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

20:   contract NibblVault is INibblVault, BancorFormula, ERC20Upgradeable, Twav, EIP712Base {

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

9. Incorrect comments

There are 3 instances of this issue:

File: contracts/Basket.sol   #1

/// @audit ERC1155, not ERC721
58:      /// @notice withdraw an ERC721 token from this contract into your wallet

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L58

File: contracts/Twav/Twav.sol   #2

/// @audit or zero if there have been fewer than four blocks
34:      /// @return _twav TWAV of the last 4 blocks

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

File: contracts/Twav/Twav.sol   #3

/// @audit of the last four updates, not necessarily of the last four blocks (i.e. may be blocked that were skipped)
34:      /// @return _twav TWAV of the last 4 blocks

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

Non-critical Issues

1. Consider addings checks for signature malleability

Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

563:         address signer = ecrecover(toTypedMessageHash(structHash), v, r, s);

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

2. Misleading variable name

_twavObservationPrev is not the previous observation - it's more like the trailing, or next-to-expire TWAV observation`

There is 1 instance of this issue:

File: contracts/Twav/Twav.sol   #1

39:              TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];

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

3. Inconsistent version of English being used

Some functions use American English, whereas others use British English. A single project should use only one of the two

There are 2 instances of this issue:

File: contracts/NibblVault.sol   #1

173:     function initialize(

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

File: contracts/Interfaces/IBasket.sol   #2

10:      function initialise(address _curator) external;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Interfaces/IBasket.sol#L10

4. Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.

There is 1 instance of this issue:

File: contracts/Basket.sol   #1

13:   contract Basket is IBasket, ERC721("NFT Basket", "NFTB"), Initializable {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L13

5. Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

/// @audit onERC721Received(), onERC1155Received()
20:   contract NibblVault is INibblVault, BancorFormula, ERC20Upgradeable, Twav, EIP712Base {

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

6. require()/revert() statements should have descriptive reason strings

There is 1 instance of this issue:

File: contracts/NibblVaultFactory.sol   #1

114:          require(_success);

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L114

7. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 3 instances of this issue:

File: contracts/NibblVaultFactory.sol   #1

64        function getVaultAddress(
65            address _curator,
66            address _assetAddress,
67            uint256 _assetTokenID,
68            uint256 _initialSupply,
69:           uint256 _initialTokenPrice) public view returns(address _vault) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L64-L69

File: contracts/NibblVaultFactory.sol   #2

76:       function getVaults() public view returns(ProxyVault[] memory ) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L76

File: contracts/Twav/Twav.sol   #3

44:       function getTwavObservations() public view returns(TwavObservation[TWAV_BLOCK_NUMBERS] memory) {

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

8. Non-assembly method available

assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length

There is 1 instance of this issue:

File: contracts/Utilities/EIP712Base.sol   #1

29:               id := chainid()

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L29

9. 2**<n> - 1 should be re-written as type(uint<n>).max

Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)

There are 4 instances of this issue:

File: contracts/NibblVault.sol   #1

303:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

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

File: contracts/NibblVault.sol   #2

365:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

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

File: contracts/NibblVault.sol   #3

413:          _updateTWAV(_currentValuation, uint32(block.timestamp % 2**32));

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

File: contracts/NibblVault.sol   #4

445:          uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

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

10. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 10 instances of this issue:

File: contracts/NibblVaultFactory.sol

/// @audit 0xff
72:           bytes32 _hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));

/// @audit 0xff
91:           bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L72

File: contracts/NibblVault.sol

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

/// @audit 1e18
195:          uint _primaryReserveBalance = (primaryReserveRatio * _initialTokenSupply * _initialTokenPrice) / (SCALE * 1e18);

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

/// @audit 1e18
253:              return ((secondaryReserveRatio * initialTokenSupply * initialTokenPrice) / (1e18 * SCALE));

/// @audit 32
303:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

/// @audit 32
365:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

/// @audit 32
413:          _updateTWAV(_currentValuation, uint32(block.timestamp % 2**32));

/// @audit 32
445:          uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

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

11. Cast is more restrictive than the type of the variable being assigned

If address foo is being used in an expression such as IERC20 token = FooToken(foo), then the more specific cast to FooToken is a waste because the only thing the compiler will check for is that FooToken extends IERC20 - it won't check any of the function signatures. Therefore, it makes more sense to do IERC20 token = IERC20(token) or better yet FooToken token = FooToken(foo). The former may allow the file in which it's used to remove the import for FooToken

There is 1 instance of this issue:

File: contracts/Proxy/ProxyBasket.sol   #1

/// @audit payable vs address
20:           implementation = payable(_implementation);

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

12. Missing event and or timelock for critical parameter change

Events help non-contract tools to track changes, and events prevent users from being surprised by changes

There are 4 instances of this issue:

File: contracts/NibblVault.sol   #1

485       function updateCurator(address _newCurator) external override {
486           require(msg.sender == curator,"NibblVault: Only Curator");
487           curator = _newCurator;
488:      }

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

File: contracts/NibblVaultFactory.sol   #2

100:         pendingBasketImplementation = _newBasketImplementation;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L100

File: contracts/NibblVaultFactory.sol   #3

124:         pendingFeeTo = _newFeeAddress;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L124

File: contracts/NibblVaultFactory.sol   #4

159:         pendingVaultImplementation = _newVaultImplementation;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L159

13. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

There are 5 instances of this issue:

File: contracts/NibblVault.sol

51:       bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

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

File: contracts/Utilities/AccessControlMechanism.sol

12:       bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");

13:       bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

14:       bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

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

File: contracts/Utilities/EIP712Base.sol

7         bytes32 internal constant EIP712_DOMAIN_TYPEHASH = keccak256(
8             bytes(
9                 "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
10            )
11:       );

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol#L7-L11

14. Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file

There are 27 instances of this issue:

File: contracts/NibblVault.sol

28:       uint32 private constant primaryReserveRatio = 200_000; //20%

34:       uint256 private constant REJECTION_PREMIUM = 150_000; //15%

46:       uint256 private constant MIN_CURATOR_FEE = 5_000; //0.5%

122:      ///@notice current status of vault

125:      ///@notice reenterancy guard

200:          //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

201:          curatorFee = (((_secondaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO) * MIN_CURATOR_FEE) / (primaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO)) + MIN_CURATOR_FEE; //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

220:          //_maxSecondaryBalanceIncrease: is the max amount of secondary reserve balance that can be added to the vault

221:          //_maxSecondaryBalanceIncrease cannot be more than fictitiousPrimaryReserveBalance

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

228:              safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract

244:              safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract

301:          //Make update on the first tx of the block

318:                  //Gas Optimization

363:          //Make update on the first tx of the block

368:                  _rejectBuyout(); //For the case when TWAV goes up when updated on sell

377:                  //Gas Optimization

389:          safeTransferETH(_to, _saleReturn); //send _saleReturn to _to

402:          //_buyoutBid: Bid User has made

448:              _rejectBuyout(); //For the case when TWAV goes up when updated externally

500:      ///@notice withdraw multiple ERC721s

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

File: contracts/Proxy/ProxyBasket.sol

28:       //solhint-disable-next-line no-complex-fallback

31:           //solhint-disable-next-line no-inline-assembly

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

File: contracts/Proxy/ProxyVault.sol

28:       //solhint-disable-next-line no-complex-fallback

31:           //solhint-disable-next-line no-inline-assembly

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

File: contracts/Twav/Twav.sol

12:       uint8 private constant TWAV_BLOCK_NUMBERS = 4; //TWAV of last 4 Blocks 

28:           twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative

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

15. Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

There are 14 instances of this issue:

File: contracts/Basket.sol

109:      function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L109

File: contracts/NibblVaultFactory.sol

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L50

File: contracts/NibblVault.sol

19:   /// @dev The secondary curve is dynamic and has a variable reserveRatio, which depends on initial conditions given by the curator and the fee accumulated by the curve.

79:       /// @dev This variable also defines the amount of reserve token that should be in the primary curve if the primary curve started from 0 and went till initialTokenSupply 

201:          curatorFee = (((_secondaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO) * MIN_CURATOR_FEE) / (primaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO)) + MIN_CURATOR_FEE; //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

224:          _feeCurve = _maxSecondaryBalanceIncrease > _feeCurve ? _feeCurve : _maxSecondaryBalanceIncrease; // the curve fee is capped so that secondaryReserveBalance <= fictitiousPrimaryReserveBalance

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

263:      /// @dev Valuation = If current supply is on seconday curve we use secondaryReserveBalance and secondaryReserveRatio to calculate valuation else we use primary reserve ratio and balance

266:              return totalSupply() < initialTokenSupply ? (secondaryReserveBalance * SCALE /secondaryReserveRatio) : ((primaryReserveBalance) * SCALE  / primaryReserveRatio);

297:      /// @dev if current totalSupply < initialTokenSupply AND _amount to buy tokens for is greater than (maxSecondaryCurveBalance - currentSecondaryCurveBalance) then buy happens on secondary curve and primary curve both

358:      /// @dev if totalSupply > initialTokenSupply AND _amount to sell is greater than (_amtIn > totalSupply - initialTokenSupply) then sell happens on primary curve and secondary curve both

395:      /// @dev bidder needs to send funds equal to current valuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance) to initiate buyout

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

File: contracts/Twav/Twav.sol

28:           twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative

40:               _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);

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

16. Non-library/interface files should use fixed compiler versions, not floating ones

There is 1 instance of this issue:

File: contracts/Utilities/AccessControlMechanism.sol   #1

4:    pragma solidity ^0.8.0;

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

17. Typos

There are 14 instances of this issue:

File: contracts/NibblVault.sol

/// @audit reenterancy
125:      ///@notice reenterancy guard

/// @audit pausablity
152:      /// @dev pausablity implemented in factory

/// @audit primaryReseveRatio
200:          //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

/// @audit primaryReseveRatio
201:          curatorFee = (((_secondaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO) * MIN_CURATOR_FEE) / (primaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO)) + MIN_CURATOR_FEE; //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

/// @audit continous
250:      /// @dev The max continous tokens on SecondaryCurve is equal to initialTokenSupply

/// @audit seconday
263:      /// @dev Valuation = If current supply is on seconday curve we use secondaryReserveBalance and secondaryReserveRatio to calculate valuation else we use primary reserve ratio and balance

/// @audit continous
270:      /// @param _amount amount of reserve tokens to buy continous tokens

/// @audit continous
282:      /// @param _amount amount of reserve tokens to buy continous tokens

/// @audit Continous
359:      /// @param _amtIn Continous Tokens to be sold

/// @audit recieve
361:      /// @param _to Address to recieve the reserve token to

/// @audit airdops
512:      /// @notice ERC20s can be accumulated by the underlying ERC721 in the vault as royalty or airdops 

/// @audit airdops
531:      /// @notice ERC1155s can be accumulated by the underlying ERC721 in the vault as royalty or airdops 

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

File: contracts/Proxy/ProxyBasket.sol

/// @audit internall
26:        * This function does not return to its internall call site, it will return directly to the external caller.

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

File: contracts/Proxy/ProxyVault.sol

/// @audit internall
26:        * This function does not return to its internall call site, it will return directly to the external caller.

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

18. File is missing NatSpec

There is 1 instance of this issue:

File: contracts/Utilities/EIP712Base.sol (various lines)   #1

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/EIP712Base.sol

19. NatSpec is incomplete

There are 12 instances of this issue:

File: contracts/Basket.sol

/// @audit Missing: '@param _to'
32        /// @notice withdraw an ERC721 token from this contract into your wallet
33        /// @param _token the address of the NFT you are withdrawing
34        /// @param _tokenId the ID of the NFT you are withdrawing
35:       function withdrawERC721(address _token, uint256 _tokenId, address _to) external override {

/// @audit Missing: '@param _to'
49        /// @notice withdraw an ERC721 token from this contract into your wallet
50        /// @param _token the address of the NFT you are withdrawing
51        /// @param _tokenId the ID of the NFT you are withdrawing
52:       function withdrawERC721Unsafe(address _token, uint256 _tokenId, address _to) external override {

/// @audit Missing: '@param _to'
58        /// @notice withdraw an ERC721 token from this contract into your wallet
59        /// @param _token the address of the NFT you are withdrawing
60        /// @param _tokenId the ID of the NFT you are withdrawing
61:       function withdrawERC1155(address _token, uint256 _tokenId, address _to) external override {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L32-L35

File: contracts/NibblVaultFactory.sol

/// @audit Missing: '@return'
36        /// @param _initialTokenPrice desired initial token price
37        /// @param _minBuyoutTime minimum time after which buyout can be triggered
38        function createVault(
39            address _assetAddress,
40            address _curator,
41            string memory _name,
42            string memory _symbol,
43            uint256 _assetTokenID,
44            uint256 _initialSupply,
45            uint256 _initialTokenPrice,
46            uint256 _minBuyoutTime
47:           ) external payable override whenNotPaused returns(address payable _proxyVault) {

/// @audit Missing: '@return'
62        /// @param _initialSupply desired initial token supply
63        /// @param _initialTokenPrice desired initial token price    
64        function getVaultAddress(
65            address _curator,
66            address _assetAddress,
67            uint256 _assetTokenID,
68            uint256 _initialSupply,
69:           uint256 _initialTokenPrice) public view returns(address _vault) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L36-L47

File: contracts/NibblVault.sol

/// @audit Missing: '@param _totalSupply'
269       /// @notice function to buy tokens on the primary curve
270       /// @param _amount amount of reserve tokens to buy continous tokens
271       /// @dev This is executed when current supply >= initial supply
272       /// @dev _amount is charged with fee
273       /// @dev _purchaseReturn is minted to _to
274       /// @return _purchaseReturn Purchase return
275:      function _buyPrimaryCurve(uint256 _amount, uint256 _totalSupply) private returns (uint256 _purchaseReturn) {

/// @audit Missing: '@param _totalSupply'
281       /// @notice function to buy tokens on secondary curve
282       /// @param _amount amount of reserve tokens to buy continous tokens
283       /// @dev This is executed when current supply < initial supply
284       /// @dev only admin and curator fee is charged in secondary curve
285       /// @dev _purchaseReturn is minted to _to
286       /// @return _purchaseReturn Purchase return
287:      function _buySecondaryCurve(uint256 _amount, uint256 _totalSupply) private returns (uint256 _purchaseReturn) {

/// @audit Missing: '@return'
298       /// @param _minAmtOut Minimum amount of continuous token user receives, else the tx fails.
299       /// @param _to Address to mint the purchase return to
300:      function buy(uint256 _minAmtOut, address _to) external override payable notBoughtOut lock whenNotPaused returns(uint256 _purchaseReturn) {

/// @audit Missing: '@param _totalSupply'
330       /// @notice The function to sell fractional tokens on primary curve
331       /// @dev Executed when currentSupply > initialSupply
332       /// @dev _amount is charged with fee
333       /// @param _amount Amount of tokens to be sold on primary curve
334       /// @return _saleReturn Sale Return
335:      function _sellPrimaryCurve(uint256 _amount, uint256 _totalSupply) private returns(uint256 _saleReturn) {

/// @audit Missing: '@param _totalSupply'
342       /// @notice The function to sell fractional tokens on secondary curve
343       /// @dev Executed when current supply <= initial supply
344       /// @dev only admin and curator fee is charged in secondary curve
345       /// @param _amount Amount of tokens to be sold on SecondaryCurve
346       ///  @return _saleReturn Sale Return
347:      function _sellSecondaryCurve(uint256 _amount, uint256 _totalSupply) private returns(uint256 _saleReturn){

/// @audit Missing: '@return'
360       /// @param _minAmtOut Minimum amount of reserve token user receives, else the tx fails.
361       /// @param _to Address to recieve the reserve token to
362:      function sell(uint256 _amtIn, uint256 _minAmtOut, address payable _to) external override notBoughtOut whenNotPaused returns(uint256 _saleReturn) {

/// @audit Missing: '@return'
472       /// @param _to the address where curator fee will be sent
473       /// @dev can only be called by curator
474:      function redeemCuratorFee(address payable _to) external override returns(uint256 _feeAccruedCurator) {

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

20. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There are 5 instances of this issue:

File: contracts/Basket.sol

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

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

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

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

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L15

Summary

Gas Optimizations

Total: 153 instances over 23 issues

Gas Optimizations

1. Setting DEFAULT_ADMIN_ROLE as the role admin is redundant

DEFAULT_ADMIN_ROLE is automatically designated as the role admin of any new role, so setting it again is a waste of gas since it involves fetching role-related state variables, updating state variables, and emitting an event

There is 1 instance of this issue:

File: contracts/Utilities/AccessControlMechanism.sol   #1

22           _setRoleAdmin(_defaultAdminRole, _defaultAdminRole);
23           _setRoleAdmin(FEE_ROLE, _defaultAdminRole);
24           _setRoleAdmin(PAUSER_ROLE, _defaultAdminRole);
25:          _setRoleAdmin(IMPLEMENTER_ROLE, _defaultAdminRole);

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/AccessControlMechanism.sol#L22-L25

2. Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

There are 23 instances of this issue:

File: contracts/Basket.sol

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

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

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

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

91:       function withdrawMultipleERC20(address[] memory _tokens) external override {

99:       function onERC721Received(address, address from, uint256 id, bytes memory) external override returns(bytes4) {

104:      function onERC1155Received(address, address from, uint256 id, uint256 amount, bytes memory) external virtual override returns (bytes4) {

109:      function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {

109:      function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {

109:      function onERC1155BatchReceived(address, address from, uint256[] memory ids, uint256[] memory amounts, bytes memory) external virtual override returns (bytes4) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L41

File: contracts/NibblVaultFactory.sol

41:           string memory _name,

42:           string memory _symbol,

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L41

File: contracts/NibblVault.sol

174:          string memory _tokenName, 

175:          string memory _tokenSymbol, 

504:      function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {

504:      function withdrawMultipleERC721(address[] memory _assetAddresses, uint256[] memory _assetIDs, address _to) external override boughtOut {

523:      function withdrawMultipleERC20(address[] memory _assets, address _to) external override boughtOut {

545:      function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {

545:      function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {

577:      function onERC1155Received(address, address, uint256, uint256, bytes memory) external pure returns (bytes4) {

581:      function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) external pure returns (bytes4) {

581:      function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) external pure returns (bytes4) {

581:      function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) external pure returns (bytes4) {

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

3. Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

There are 2 instances of this issue:

File: contracts/Twav/Twav.sol   #1

38:               TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];

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

File: contracts/Twav/Twav.sol   #2

39:               TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];

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

4. State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

/// @audit secondaryReserveBalance on line 225
226:          secondaryReserveRatio = uint32((secondaryReserveBalance * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice)); //secondaryReserveRatio is updated on every trade 

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

5. <x> += <y> costs more gas than <x> = <x> + <y> for state variables

There are 7 instances of this issue:

File: contracts/NibblVault.sol

219:          feeAccruedCurator += _feeCurator;

225:          secondaryReserveBalance += _feeCurve;

242:          feeAccruedCurator += _feeCurator;

320:                  secondaryReserveBalance += _lowerCurveDiff;

380:                  primaryReserveBalance -= _saleReturn;

429:              totalUnsettledBids += _buyoutValuationDeposit;

457:          totalUnsettledBids -= _amount;

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

6. internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There is 1 instance of this issue:

File: contracts/Utilities/EIP712Base.sol   #1

27:       function getChainID() internal view returns (uint256 id) {

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

7. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There are 5 instances of this issue:

File: contracts/NibblVault.sol

/// @audit require() on line 185
201:          curatorFee = (((_secondaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO) * MIN_CURATOR_FEE) / (primaryReserveRatio - MIN_SECONDARY_RESERVE_RATIO)) + MIN_CURATOR_FEE; //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator

/// @audit require() on line 404
406:          buyoutValuationDeposit = msg.value - (_buyoutBid - _currentValuation);

/// @audit require() on line 404
415:              safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation));

/// @audit if-condition on line 373
378:                  uint256 _tokensPrimaryCurve = _totalSupply - _initialTokenSupply;

/// @audit if-condition on line 414
415:              safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation));

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

8. <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 6 instances of this issue:

File: contracts/Basket.sol

43:           for (uint256 i = 0; i < _tokens.length; i++) {

70:           for (uint256 i = 0; i < _tokens.length; i++) {

93:           for (uint256 i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

File: contracts/NibblVault.sol

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

525:          for (uint256 i = 0; i < _assets.length; i++) {

547:          for (uint256 i = 0; i < _assets.length; i++) {

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

9. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 6 instances of this issue:

File: contracts/Basket.sol

43:           for (uint256 i = 0; i < _tokens.length; i++) {

70:           for (uint256 i = 0; i < _tokens.length; i++) {

93:           for (uint256 i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

File: contracts/NibblVault.sol

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

525:          for (uint256 i = 0; i < _assets.length; i++) {

547:          for (uint256 i = 0; i < _assets.length; i++) {

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

10. require()/revert() strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

There are 7 instances of this issue:

File: contracts/NibblVaultFactory.sol

48:           require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");

49:           require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");

107:          require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

131:          require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

141:          require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");

149:          require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

166:          require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L48

11. Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

There is 1 instance of this issue:

File: contracts/Utilities/AccessControlMechanism.sol   #1

16:       mapping(bytes32 => mapping(address => bool)) public pendingRoles;

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

12. Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

There is 1 instance of this issue:

File: contracts/Utilities/AccessControlMechanism.sol   #1

4:    pragma solidity ^0.8.0;

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

13. >= costs less gas than >

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

There is 1 instance of this issue:

File: contracts/NibblVault.sol   #1

224:          _feeCurve = _maxSecondaryBalanceIncrease > _feeCurve ? _feeCurve : _maxSecondaryBalanceIncrease; // the curve fee is capped so that secondaryReserveBalance <= fictitiousPrimaryReserveBalance

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

14. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 6 instances of this issue:

File: contracts/Basket.sol

43:           for (uint256 i = 0; i < _tokens.length; i++) {

70:           for (uint256 i = 0; i < _tokens.length; i++) {

93:           for (uint256 i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

File: contracts/NibblVault.sol

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

525:          for (uint256 i = 0; i < _assets.length; i++) {

547:          for (uint256 i = 0; i < _assets.length; i++) {

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

15. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 6 gas per loop

There are 6 instances of this issue:

File: contracts/Basket.sol

43:           for (uint256 i = 0; i < _tokens.length; i++) {

70:           for (uint256 i = 0; i < _tokens.length; i++) {

93:           for (uint256 i = 0; i < _tokens.length; i++) {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43

File: contracts/NibblVault.sol

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

525:          for (uint256 i = 0; i < _assets.length; i++) {

547:          for (uint256 i = 0; i < _assets.length; i++) {

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

16. Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

There are 4 instances of this issue:

File: contracts/NibblVaultFactory.sol   #1

107:          require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L107

File: contracts/NibblVaultFactory.sol   #2

131:          require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L131

File: contracts/NibblVaultFactory.sol   #3

149:          require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L149

File: contracts/NibblVaultFactory.sol   #4

166:          require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L166

17. Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

There are 14 instances of this issue:

File: contracts/NibblVault.sol

28:       uint32 private constant primaryReserveRatio = 200_000; //20%

57:       uint32 public secondaryReserveRatio;

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

303:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

365:              uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

445:          uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

557:          uint8 v,

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

File: contracts/Twav/Twav.sol

6:            uint32 timestamp;

11:       uint8 public twavObservationsIndex;

12:       uint8 private constant TWAV_BLOCK_NUMBERS = 4; //TWAV of last 4 Blocks 

13:       uint32 public lastBlockTimeStamp;

21:       function _updateTWAV(uint256 _valuation, uint32 _blockTimestamp) internal {

22:           uint32 _timeElapsed; 

37:               uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;

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

18. Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/Utilities/AccessControlMechanism.sol   #1

12:       bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");

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

File: contracts/Utilities/AccessControlMechanism.sol   #2

13:       bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

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

File: contracts/Utilities/AccessControlMechanism.sol   #3

14:       bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");

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

19. Don't use SafeMath once the solidity version is 0.8.0 or greater

Version 0.8.0 introduces internal overflow checks, so using SafeMath is redundant and adds overhead

There is 1 instance of this issue:

File: contracts/NibblVaultFactory.sol   #1

9:    import { SafeMath } from  "@openzeppelin/contracts/utils/math/SafeMath.sol";

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L9

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

Saves deployment costs

There are 3 instances of this issue:

File: contracts/Basket.sol   #1

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L42

File: contracts/NibblVault.sol   #2

486:          require(msg.sender == curator,"NibblVault: Only Curator");

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

File: contracts/NibblVault.sol   #3

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

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

21. Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

There are 5 instances of this issue:

File: contracts/Basket.sol

114:      receive() external payable {}

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L114

File: contracts/NibblVaultFactory.sol

183:      receive() payable external {    }

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L183

File: contracts/NibblVault.sol

585:      receive() external payable {}

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

File: contracts/Proxy/ProxyBasket.sol

56:       receive() external payable {    }

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

File: contracts/Proxy/ProxyVault.sol

56:       receive() external payable {    }

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

22. Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 41 instances of this issue:

File: contracts/Basket.sol

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

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

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

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

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

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

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

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

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L36

File: contracts/NibblVaultFactory.sol

48:           require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");

49:           require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");

107:          require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

131:          require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

141:          require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");

149:          require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

166:          require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L48

File: contracts/NibblVault.sol

129:          require(unlocked == 1, 'NibblVault: LOCKED');

139:          require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');

146:          require(status == Status.buyout, "NibblVault: status != buyout");

147:          require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now");

154:          require(!NibblVaultFactory(factory).paused(), 'NibblVault: Paused');

184:          require(_secondaryReserveRatio <= primaryReserveRatio, "NibblVault: Excess initial funds");

185:          require(_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO, "NibblVault: secResRatio too low");

325:          require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low");

351:          require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell");

387:          require(_saleReturn >= _minAmtOut, "NibblVault: Return too low");

399:          require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now");

400:          require(status == Status.initialized, "NibblVault: Status!=initialized");

404:          require(_buyoutBid >= _currentValuation, "NibblVault: Bid too low");

444:          require(status == Status.buyout, "NibblVault: Status!=Buyout");

475:          require(msg.sender == curator,"NibblVault: Only Curator");

486:          require(msg.sender == curator,"NibblVault: Only Curator");

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

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

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

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

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

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

561:          require(block.timestamp <= deadline, "NibblVault: expired deadline");

564:          require(signer == owner, "NibblVault: invalid signature");

570:          require(success, "NibblVault: ETH transfer failed");

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

File: contracts/Utilities/AccessControlMechanism.sol

48:           require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");

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

23. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 8 instances of this issue:

File: contracts/NibblVaultFactory.sol

99:       function proposeNewBasketImplementation(address _newBasketImplementation) external override onlyRole(IMPLEMENTER_ROLE) {

123:      function proposeNewAdminFeeAddress(address _newFeeAddress) external override onlyRole(FEE_ROLE) {

140:      function proposeNewAdminFee(uint256 _newFee) external override onlyRole(FEE_ROLE) {

158:      function proposeNewVaultImplementation(address _newVaultImplementation) external override onlyRole(IMPLEMENTER_ROLE) {

173:      function pause() external onlyRole(PAUSER_ROLE) override {

179:      function unPause() external onlyRole(PAUSER_ROLE) override {

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L99

File: contracts/Utilities/AccessControlMechanism.sol

32:       function setRoleAdmin(bytes32 _role, bytes32 _adminRole) external override onlyRole(getRoleAdmin(_role)) {

40:       function proposeGrantRole(bytes32 _role, address _to) external override onlyRole(getRoleAdmin(_role)) {

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