Nibbl contest - BowTiedWardens's results

Overview

Table of Contents

• Low Risk Issues
  • 1. whenNotPaused is not used on createBasket
  • 2. createBasket uses basketImplementation for salt, but createVault doesn't use vaultImplementation for salt
  • 3. Add constructor initializers
  • 4. DDOS on nibbledTokens array
  • 5. Check Effects Interactions pattern not respected
  • 6. AccessControlMechanism.sol: propose/accept pattern is redundant since grantRole can be used to push a role
  • 7. TWAV is only over four blocks
  • 8. Unsafe casting may overflow
  • 9. Basic ecrecover is used
  • 10. Missing address(0) checks
  • 11. Unsafe use of transfer()/transferFrom() with IERC20
  • 12. Return values of transfer()/transferFrom() not checked
  • 13. Unused/empty receive() function
  • 14. Lack of event emission after critical initialize() functions
  • 15. Prevent accidentally burning tokens
  • 16. _safeMint() should be used rather than _mint() wherever possible
  • 17. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 18. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
  • 19. All initialize() functions are front-runnable in the solution
  • 20. Use a constant instead of duplicating the same string
  • 21. Calculation doesn't match with comment
• Non-Critical Issues
  • 1. Typos
  • 2. Deprecated library used for Solidity >= 0.8 : SafeMath
  • 3. Commented code
  • 4. Missing friendly revert strings
  • 5. Use a more recent version of solidity

Low Risk Issues

1. whenNotPaused is not used on createBasket

While whenNotPaused is used on createVault:

• Good: NibblVaultFactory.createVault()

File: NibblVaultFactory.sol
38:     function createVault(
...
47:         ) external payable override whenNotPaused returns(address payable _proxyVault) {

This seems to have been forgotten on createBasket:

• Bad: NibblVaultFactory.createBasket()

File: NibblVaultFactory.sol
80:     function createBasket(address _curator, string memory _mix) public override returns(address)  {
...
86:     }

2. createBasket uses basketImplementation for salt, but createVault doesn't use vaultImplementation for salt

• Good: NibblVaultFactory.createBasket()

File: NibblVaultFactory.sol
80:     function createBasket(address _curator, string memory _mix) public override returns(address)  {
81:         address payable _basketAddress = payable(new ProxyBasket{salt: keccak256(abi.encodePacked(_curator, _mix))}(basketImplementation));
...
86:     }

• Bad: NibblVaultFactory.createVault()

File: NibblVaultFactory.sol
38:     function createVault(
...
47:         ) external payable override whenNotPaused returns(address payable _proxyVault) {
...
50:         _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));

3. Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

contracts/Basket.sol:
  23:     function initialise(address _curator) external override initializer {

contracts/NibblVault.sol:
  182:     ) external override initializer payable {

4. DDOS on nibbledTokens array

The minimum price to create a vault is pretty trivial, 1 gwei:

File: NibblVaultFactory.sol
19:     uint256 private constant MIN_INITIAL_RESERVE_BALANCE = 1e9;

An attacker could spam and bloat the size of the nibbledTokens array to the point getVaults() becomes unusable

contracts/NibblVaultFactory.sol:
  22:     ProxyVault[] public nibbledTokens;
  54:         nibbledTokens.push(ProxyVault(_proxyVault));
  77:         return nibbledTokens;

5. Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

Affected code:

NibblVaultFactory.sol:53:        IERC721(_assetAddress).safeTransferFrom(msg.sender, address(_vault), _assetTokenID);

6. AccessControlMechanism.sol: propose/accept pattern is redundant since grantRole can be used to push a role

• File: @openzeppelin/contracts/access/AccessControl.sol

217:     function _grantRole(bytes32 role, address account) internal virtual {
218:         if (!hasRole(role, account)) {
219:             _roles[role].members[account] = true;
220:             emit RoleGranted(role, account, _msgSender());
221:         }
222:     }

• File: AccessControlMechanism.sol

40:     function proposeGrantRole(bytes32 _role, address _to) external override onlyRole(getRoleAdmin(_role)) {
41:         pendingRoles[_role][_to] = true;
42:     }
43: 
44:     /// @notice proposed user needs to claim the role
45:     /// @dev can only be called by the proposed user
46:     /// @param _role role to be claimed
47:     function claimRole(bytes32 _role) external override {
48:         require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");
49:         _grantRole(_role, msg.sender);
50:         delete pendingRoles[_role][msg.sender];
51:     }

7. TWAV is only over four blocks

Someone does not have to maintain price variation for long to reject a buyout. This can result in blocking of buyouts.

File: Twav.sol
12:     uint8 private constant TWAV_BLOCK_NUMBERS = 4; //TWAV of last 4 Blocks 

8. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

NibblVault.sol:183:        uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));
NibblVault.sol:226:        secondaryReserveRatio = uint32((secondaryReserveBalance * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice)); //secondaryReserveRatio is updated on every trade 
NibblVault.sol:303:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
NibblVault.sol:365:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
NibblVault.sol:413:        _updateTWAV(_currentValuation, uint32(block.timestamp % 2**32));
NibblVault.sol:445:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

9. Basic ecrecover is used

Basic ecrecover is used, leading to being able to approve transfers from the zero address. Any tokens sent to 0 address can be recovered by another user.

• NibblVault.permit()

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

Consider using OpenZeppelin’s ECDSA library

10. Missing address(0) checks

(Output from Slither) Consider adding an address(0) check here:

  - _to.transfer(address(this).balance) (contracts/Basket.sol#80)
  - assetAddress = _assetAddress (contracts/NibblVault.sol#191)
  - curator = _curator (contracts/NibblVault.sol#193)
  - curator = _newCurator (contracts/NibblVault.sol#487)
  - vaultImplementation = _vaultImplementation (contracts/NibblVaultFactory.sol#24)
  - feeTo = _feeTo (contracts/NibblVaultFactory.sol#25)
  - basketImplementation = _basketImplementation (contracts/NibblVaultFactory.sol#26)
  - pendingBasketImplementation = _newBasketImplementation (contracts/NibblVaultFactory.sol#100)
  - pendingFeeTo = _newFeeAddress (contracts/NibblVaultFactory.sol#124)
  - pendingVaultImplementation = _newVaultImplementation (contracts/NibblVaultFactory.sol#159)
  - implementation = address(_implementation) (contracts/Proxy/ProxyBasket.sol#20)
  - factory = address(_factory) (contracts/Proxy/ProxyVault.sol#20)

11. Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

Bad:

IERC20(token).transferFrom(msg.sender, address(this), amount);

Good (using OpenZeppelin's SafeERC20):

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";

// ...

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

Consider using safeTransfer/safeTransferFrom here:

Basket.sol:87:        IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));
Basket.sol:94:            IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
NibblVault.sol:517:        IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
NibblVault.sol:526:            IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));

12. 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.

Bad:

IERC20(token).transferFrom(msg.sender, address(this), amount);

Good (using require):

bool success = IERC20(token).transferFrom(msg.sender, address(this), amount);
require(success, "ERC20 transfer failed");

Consider wrapping transfers in require() statements consistently here:

Basket.sol:87:        IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));
Basket.sol:94:            IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
NibblVault.sol:517:        IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
NibblVault.sol:526:            IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));

Alternatively, SafeERC20 could be used here, as stated before

13. Unused/empty receive() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

Proxy/ProxyBasket.sol:56:    receive() external payable {    }
Proxy/ProxyVault.sol:56:    receive() external payable {    }
Basket.sol:114:    receive() external payable {}
NibblVault.sol:585:    receive() external payable {}
NibblVaultFactory.sol:183:    receive() payable external {    }

14. Lack of event emission after critical initialize() functions

Similar issue in the past: here

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions:

Basket.sol:23:    function initialise(address _curator) external override initializer {

15. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

Basket.sol:37:        IERC721(_token).safeTransferFrom(address(this), _to, _tokenId);
Basket.sol:44:            IERC721(_tokens[i]).safeTransferFrom(address(this), _to, _tokenId[i]);
Basket.sol:54:        IERC721(_token).transferFrom(address(this), _to, _tokenId);
Basket.sol:64:        IERC1155(_token).safeTransferFrom(address(this), _to, _tokenId, _balance, "0");
Basket.sol:72:            IERC1155(_tokens[i]).safeTransferFrom(address(this), _to, _tokenIds[i], _balance, "0");
NibblVault.sol:389:        safeTransferETH(_to, _saleReturn); //send _saleReturn to _to
NibblVault.sol:458:        safeTransferETH(_to, _amount);
NibblVault.sol:468:        safeTransferETH(_to, _amtOut);
NibblVault.sol:478:        safeTransferETH(_to, _feeAccruedCurator);
NibblVault.sol:497:        IERC721(_assetAddress).safeTransferFrom(address(this), _to, _assetID);
NibblVault.sol:507:            IERC721(_assetAddresses[i]).safeTransferFrom(address(this), _to, _assetIDs[i]);
NibblVault.sol:517:        IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
NibblVault.sol:526:            IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));
NibblVault.sol:538:        IERC1155(_asset).safeTransferFrom(address(this), _to, _assetID, balance, "0");
NibblVault.sol:549:            IERC1155(_assets[i]).safeTransferFrom(address(this), _to, _assetIDs[i], balance, "0");

16. _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 open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

Basket.sol:6:import { ERC721, IERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
Basket.sol:24:        _mint(_curator, 0);

17. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

Utilities/EIP712Base.sol:47:                abi.encodePacked("\x19\x01", domainSeperator, messageHash)
NibblVaultFactory.sol:50:        _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));
NibblVaultFactory.sol:70:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID,  _initialSupply, _initialTokenPrice));
NibblVaultFactory.sol:72:        bytes32 _hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));
NibblVaultFactory.sol:81:        address payable _basketAddress = payable(new ProxyBasket{salt: keccak256(abi.encodePacked(_curator, _mix))}(basketImplementation));
NibblVaultFactory.sol:89:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _mix));
NibblVaultFactory.sol:90:        bytes memory code = abi.encodePacked(type(ProxyBasket).creationCode, uint256(uint160(basketImplementation)));
NibblVaultFactory.sol:91:        bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));

18. 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.

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

19. All initialize() functions are front-runnable in the solution

Consider adding some access control to them or deploying atomically or using constructor initializer:

Basket.sol:23:    function initialise(address _curator) external override initializer {
NibblVault.sol:173:    function initialize(

20. Use a constant instead of duplicating the same string

Basket.sol:36:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:42:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:53:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:62:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:69:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:79:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:86:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:92:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

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

NibblVault.sol:496:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:505:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:516:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:524:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:536:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:546:        require(msg.sender == bidder, "NibblVault: Only winner");

21. Calculation doesn't match with comment

• NibblVault.initiateBuyout()

405:         // buyoutValuationDeposit = _currentValuation - ((primaryReserveBalance - fictitiousPrimaryReserveBalance) + secondaryReserveBalance); 
406:         buyoutValuationDeposit = msg.value - (_buyoutBid - _currentValuation);

Non-Critical Issues

1. Typos

• function initialise (vs initialize)

Basket.sol:23:    function initialise(address _curator) external override initializer {

• internall

Proxy/ProxyBasket.sol:26:     * This function does not return to its internall call site, it will return directly to the external caller.
Proxy/ProxyVault.sol:26:     * This function does not return to its internall call site, it will return directly to the external caller.

• reenterancy

NibblVault.sol:125:    ///@notice reenterancy guard

• pausablity

NibblVault.sol:152:    /// @dev pausablity implemented in factory

• primaryReseveRatio

NibblVault.sol:200:        //curator fee is proportional to the secondary reserve ratio/primaryReseveRatio i.e. initial liquidity added by curator
NibblVault.sol: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

• seconday

NibblVault.sol: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

• Continous

NibblVault.sol:250:    /// @dev The max continous tokens on SecondaryCurve is equal to initialTokenSupply
NibblVault.sol:270:    /// @param _amount amount of reserve tokens to buy continous tokens
NibblVault.sol:282:    /// @param _amount amount of reserve tokens to buy continous tokens
NibblVault.sol:359:    /// @param _amtIn Continous Tokens to be sold

• recieve

NibblVault.sol:361:    /// @param _to Address to recieve the reserve token to

• airdops

NibblVault.sol:512:    /// @notice ERC20s can be accumulated by the underlying ERC721 in the vault as royalty or airdops 
NibblVault.sol:531:    /// @notice ERC1155s can be accumulated by the underlying ERC721 in the vault as royalty or airdops 

2. Deprecated library used for Solidity >= 0.8 : SafeMath

NibblVaultFactory.sol:3:pragma solidity 0.8.10;
NibblVaultFactory.sol:9:import { SafeMath } from  "@openzeppelin/contracts/utils/math/SafeMath.sol";"

3. Commented code

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

File: NibblVault.sol
321:                 // _purchaseReturn = _buySecondaryCurve(_to, _lowerCurveDiff);
322:                 _purchaseReturn += _buyPrimaryCurve(msg.value - _lowerCurveDiff, _totalSupply + _purchaseReturn);

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

File: NibblVault.sol
382:                 // _saleReturn = _sellPrimaryCurve(_tokensPrimaryCurve);
383:                 _saleReturn += _sellSecondaryCurve(_amtIn - _tokensPrimaryCurve, _initialTokenSupply);

4. Missing friendly revert strings

NibblVaultFactory.sol:114:        require(_success);

5. Use a more recent version of solidity

From solidity version of at least 0.8.4 , you can use bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

Utilities/EIP712Base.sol:3:pragma solidity 0.8.10;
Utilities/EIP712Base.sol:47:                abi.encodePacked("\x19\x01", domainSeperator, messageHash)

NibblVaultFactory.sol:3:pragma solidity 0.8.10;
NibblVaultFactory.sol:50:        _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));
NibblVaultFactory.sol:70:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID,  _initialSupply, _initialTokenPrice));
NibblVaultFactory.sol:71:        bytes memory code = abi.encodePacked(type(ProxyVault).creationCode, uint256(uint160(address(this))));
NibblVaultFactory.sol:72:        bytes32 _hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));
NibblVaultFactory.sol:81:        address payable _basketAddress = payable(new ProxyBasket{salt: keccak256(abi.encodePacked(_curator, _mix))}(basketImplementation));
NibblVaultFactory.sol:89:        bytes32 newsalt = keccak256(abi.encodePacked(_curator, _mix));
NibblVaultFactory.sol:90:        bytes memory code = abi.encodePacked(type(ProxyBasket).creationCode, uint256(uint160(basketImplementation)));
NibblVaultFactory.sol:91:        bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));

Overview

Table of Contents:

• 1. Use of the memory keyword when storage should be used
• 2. Unchecking arithmetics operations that can't underflow/overflow
• 3. Caching storage values in memory
• 4. Cheap Contract Deployment Through Clones
• 5. Reduce the size of error messages (Long revert Strings)
• 6. SafeMath is not needed when using Solidity version 0.8+
• 7. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 8. Internal/Private functions only called once can be inlined to save gas
• 9. >= is cheaper than > (and <= cheaper than <)
• 10. Splitting require() statements that use && saves gas
• 11. Using private rather than public for constants saves gas
• 12. Amounts should be checked for 0 before calling a transfer
• 13. <array>.length should not be looked up in every loop of a for-loop
• 14. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 15. Increments/decrements can be unchecked in for-loops
• 16. Public functions to external
• 17. It costs more gas to initialize variables with their default value than letting the default value be applied
• 18. Use Custom Errors instead of Revert Strings to save Gas
• 19. Functions guaranteed to revert when called by normal users can be marked payable

1. Use of the memory keyword when storage should be used

Here, the storage keyword should be used instead of memory:

File: Twav.sol
35:     function _getTwav() internal view returns(uint256 _twav){
36:         if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
37:             uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
- 38:             TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];
+ 38:             TwavObservation storage _twavObservationCurrent = twavObservations[(_index)];
- 39:             TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
+ 39:             TwavObservation storage _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
40:             _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);
41:         }
42:     }

2. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here:

• File: NibblVault.sol

311:         if (_totalSupply >= _initialTokenSupply) {
...
313:         } else {
...
- 319:                 _purchaseReturn = _initialTokenSupply - _totalSupply; 
+ 319:                 unchecked { _purchaseReturn = _initialTokenSupply - _totalSupply; }

373:         if(_totalSupply > _initialTokenSupply) {
...
- 378:                 uint256 _tokensPrimaryCurve = _totalSupply - _initialTokenSupply;
+ 378:                 unchecked { uint256 _tokensPrimaryCurve = _totalSupply - _initialTokenSupply; }

414:         if (_buyoutBid > _currentValuation) {
- 415:             safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation));
+ 415:             unchecked { safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation) });

3. Caching storage values in memory

The code can be optimized by minimising the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

See the @audit tags for details about the multiple SLOADs where a cached value should be used instead of SLOAD 2 and above:

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

File: Twav.sol
27:         uint256 _prevCumulativeValuation = twavObservations[((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS].cumulativeValuation;
28:         twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative
29:         twavObservationsIndex = (twavObservationsIndex + 1) % TWAV_BLOCK_NUMBERS;

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

File: NibblVault.sol
222:         uint256 _maxSecondaryBalanceIncrease = fictitiousPrimaryReserveBalance - secondaryReserveBalance;
...
226:         secondaryReserveRatio = uint32((secondaryReserveBalance * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice)); //secondaryReserveRatio is updated on every trade 

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

File: NibblVault.sol
314:             uint256 _lowerCurveDiff = getMaxSecondaryCurveBalance() - secondaryReserveBalance;
...
320:                 secondaryReserveBalance += _lowerCurveDiff;

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

File: NibblVault.sol
379:                 _saleReturn = primaryReserveBalance - fictitiousPrimaryReserveBalance;
380:                 primaryReserveBalance -= _saleReturn;

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

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

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

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

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

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

4. Cheap Contract Deployment Through Clones

See @audit tag:

67:     function _executeTransfer(address _owner, uint256 _idx) internal {
68:         (bytes32 salt, ) = precompute(_owner, _idx);
69:         new FlashEscrow{salt: salt}( //@audit gas: deployment can cost less through clones
70:             nftAddress,
71:             _encodeFlashEscrowPayload(_idx)
72:         );
73:     }

NibblVaultFactory.sol:50:        _proxyVault = payable(new ProxyVault{salt: keccak256(abi.encodePacked(_curator, _assetAddress, _assetTokenID, _initialSupply, _initialTokenPrice))}(payable(address(this))));
NibblVaultFactory.sol:81:        address payable _basketAddress = payable(new ProxyBasket{salt: keccak256(abi.encodePacked(_curator, _mix))}(basketImplementation));

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

Consider applying a similar pattern, here with a cloneDeterministic method to mimic the current create2

5. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

NibblVaultFactory.sol:48:        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
NibblVaultFactory.sol:49:        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

Consider shortening the revert strings to fit in 32 bytes.

6. SafeMath is not needed when using Solidity version 0.8+

Solidity version 0.8+ already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8+ overflow checks) is therefore redundant.

Consider using the built-in checks instead of SafeMath and remove SafeMath here:

NibblVaultFactory.sol:3:pragma solidity 0.8.10;
NibblVaultFactory.sol:9:import { SafeMath } from  "@openzeppelin/contracts/utils/math/SafeMath.sol";

7. Duplicated conditions should be refactored to a modifier or function to save deployment costs

Basket.sol:36:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:42:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:53:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:62:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:69:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:79:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:86:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:92:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

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

NibblVault.sol:496:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:505:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:516:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:524:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:536:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:546:        require(msg.sender == bidder, "NibblVault: Only winner");

8. Internal/Private 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.

Affected code:

• NibblVault.sol#getMaxSecondaryCurveBalance()

contracts/NibblVault.sol:
  252:     function getMaxSecondaryCurveBalance() private view returns(uint256){
  314:             uint256 _lowerCurveDiff = getMaxSecondaryCurveBalance() - secondaryReserveBalance;

9. >= is cheaper than > (and <= cheaper than <)

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Consider replacing strict inequalities with non-strict ones to save some gas here:

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

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

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, Consider using multiple require statements with 1 condition per require statement:

NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

Please, note that this might not hold true at a higher number of runs for the Optimizer (10k). However, it indeed is true at 200.

11. Using private rather than public for constants saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

Utilities/AccessControlMechanism.sol:12:    bytes32 public constant FEE_ROLE = keccak256("FEE_ROLE");
Utilities/AccessControlMechanism.sol:13:    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
Utilities/AccessControlMechanism.sol:14:    bytes32 public constant IMPLEMENTER_ROLE = keccak256("IMPLEMENTER_ROLE");
Utilities/NibblVaultFactoryData.sol:7:    uint256 public constant MAX_ADMIN_FEE = 10_000; //1% 

12. Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas (especially in loops, like in NibblVault.sol#withdrawMultipleERC20()).

Consider adding a non-zero-value check here:

Basket.sol:80:        _to.transfer(address(this).balance);
Basket.sol:87:        IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));
Basket.sol:94:            IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));
NibblVault.sol:517:        IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));
NibblVault.sol:526:            IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));

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

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, Consider storing the array's length in a variable before the for-loop, and use this new variable instead:

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

14. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

15. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Affected code:

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

16. Public functions to external

An external call cost is less expensive than one of a public function. The following functions could be set external to save gas and improve code quality (extracted from Slither).

Twav/Twav.sol:44:    function getTwavObservations() public view returns(TwavObservation[TWAV_BLOCK_NUMBERS] memory) {
NibblVaultFactory.sol:76:    function getVaults() public view returns(ProxyVault[] memory ) {

17. It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Consider removing explicit initializations for default values.

18. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Consider replacing all revert strings with custom errors in the solution.

Utilities/AccessControlMechanism.sol:48:        require(pendingRoles[_role][msg.sender], "AccessControl: Role not pending");
Basket.sol:36:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:42:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:53:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:62:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:69:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:79:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:86:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
Basket.sol:92:        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
NibblVault.sol:129:        require(unlocked == 1, 'NibblVault: LOCKED');
NibblVault.sol:139:        require(buyoutEndTime > block.timestamp || buyoutEndTime == 0,'NibblVault: Bought Out');
NibblVault.sol:146:        require(status == Status.buyout, "NibblVault: status != buyout");
NibblVault.sol:147:        require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now");
NibblVault.sol:154:        require(!NibblVaultFactory(factory).paused(), 'NibblVault: Paused');
NibblVault.sol:184:        require(_secondaryReserveRatio <= primaryReserveRatio, "NibblVault: Excess initial funds");
NibblVault.sol:185:        require(_secondaryReserveRatio >= MIN_SECONDARY_RESERVE_RATIO, "NibblVault: secResRatio too low");
NibblVault.sol:325:        require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low");
NibblVault.sol:351:        require(_secondaryReserveBalance - _saleReturn >= MIN_SECONDARY_RESERVE_BALANCE, "NibblVault: Excess sell");
NibblVault.sol:387:        require(_saleReturn >= _minAmtOut, "NibblVault: Return too low");
NibblVault.sol:399:        require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now");
NibblVault.sol:400:        require(status == Status.initialized, "NibblVault: Status!=initialized");
NibblVault.sol:404:        require(_buyoutBid >= _currentValuation, "NibblVault: Bid too low");
NibblVault.sol:444:        require(status == Status.buyout, "NibblVault: Status!=Buyout");
NibblVault.sol:475:        require(msg.sender == curator,"NibblVault: Only Curator");
NibblVault.sol:486:        require(msg.sender == curator,"NibblVault: Only Curator");
NibblVault.sol:496:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:505:        require(msg.sender == bidder,"NibblVault: Only winner");
NibblVault.sol:516:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:524:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:536:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:546:        require(msg.sender == bidder, "NibblVault: Only winner");
NibblVault.sol:561:        require(block.timestamp <= deadline, "NibblVault: expired deadline");
NibblVault.sol:564:        require(signer == owner, "NibblVault: invalid signature");
NibblVault.sol:570:        require(success, "NibblVault: ETH transfer failed");
NibblVaultFactory.sol:48:        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
NibblVaultFactory.sol:49:        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:114:        require(_success);
NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

19. 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.

Utilities/AccessControlMechanism.sol:32:    function setRoleAdmin(bytes32 _role, bytes32 _adminRole) external override onlyRole(getRoleAdmin(_role)) {
Utilities/AccessControlMechanism.sol:40:    function proposeGrantRole(bytes32 _role, address _to) external override onlyRole(getRoleAdmin(_role)) {
NibblVaultFactory.sol:99:    function proposeNewBasketImplementation(address _newBasketImplementation) external override onlyRole(IMPLEMENTER_ROLE) {
NibblVaultFactory.sol:123:    function proposeNewAdminFeeAddress(address _newFeeAddress) external override onlyRole(FEE_ROLE) {
NibblVaultFactory.sol:140:    function proposeNewAdminFee(uint256 _newFee) external override onlyRole(FEE_ROLE) {
NibblVaultFactory.sol:158:    function proposeNewVaultImplementation(address _newVaultImplementation) external override onlyRole(IMPLEMENTER_ROLE) {
NibblVaultFactory.sol:173:    function pause() external onlyRole(PAUSER_ROLE) override {
NibblVaultFactory.sol:179:    function unPause() external onlyRole(PAUSER_ROLE) override {