Gas Optimizations

[code423n4]

G01 - More gas efficient for loop with ++i increment and unchecked block

Loop index increments can be written as unchecked { ++i } instead of simply i++ to save gas.

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

G02 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

L415 could be unchecked due to check on L414

contracts/NibblVault.sol:414    if (_buyoutBid > _currentValuation) {
contracts/NibblVault.sol:415        safeTransferETH(payable(msg.sender), (_buyoutBid - _currentValuation)); // @audit gas unchecked L404
contracts/NibblVault.sol:416    }

L319 could be unchecked due to check on L311

L322 could be unchecked due to check on L315 And _totalSupply + _purchaseReturn on L315 could be substituted with _initialTokenSupply since _purchaseReturn = _initialTokenSupply - _totalSupply; (L319)

L351 could be unchecked since it's would not underflow, on L350 would check it.

L378 could be unchecked due to check on L373

L428 could be unchecked since would be check for overflow on L429 and any user unsettledBids could not be bigger than totalUnsettledBids

Lines with adding block.timestamp value to constant value could be unchecked since their overflow would appear only in the extremely far future:

contracts/NibblVault.sol:411    buyoutEndTime = block.timestamp + BUYOUT_DURATION; 
contracts/NibblVaultFactory.sol:101 basketUpdateTime = block.timestamp + UPDATE_TIME; 
contracts/NibblVaultFactory.sol:125 feeToUpdateTime = block.timestamp + UPDATE_TIME; 
contracts/NibblVaultFactory.sol:143 feeAdminUpdateTime = block.timestamp + UPDATE_TIME;  
contracts/NibblVaultFactory.sol:160 vaultUpdateTime = block.timestamp + UPDATE_TIME;

L457 could be unchecked since totalUnsettledBids always >= than any user unsettledBids .

    function withdrawUnsettledBids(address payable _to) external override {
        uint _amount = unsettledBids[msg.sender];
        delete unsettledBids[msg.sender];
        totalUnsettledBids -= _amount;
        safeTransferETH(_to, _amount);
    }

G03 - Avoid 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 has been met. Next revert strings are longer than 32 bytes:

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

G04 - Admin role setup

No need to setup role admin with DEFAULT_ADMIN_ROLE since due to openzeppelin docs: By default, the admin role for all roles is DEFAULT_ADMIN_ROLE

    constructor (address _admin) {
        bytes32 _defaultAdminRole = DEFAULT_ADMIN_ROLE;
        _grantRole(_defaultAdminRole, _admin);
        _setRoleAdmin(_defaultAdminRole, _defaultAdminRole); // @audit gas needed? here and below
        _setRoleAdmin(FEE_ROLE, _defaultAdminRole); 
        _setRoleAdmin(PAUSER_ROLE, _defaultAdminRole);
        _setRoleAdmin(IMPLEMENTER_ROLE, _defaultAdminRole);
    }

G05 - Use previous index in _getTwav()

Since in _getTwav() on L39 index for previous observation will always be the same as current twavObservationsIndex there is no need to count it. twavObservationsIndex value could be cached and user on L37 and L39, like next:

    function _getTwav() internal view returns(uint256 _twav){
        if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
-           uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
+           uint8 _prevIndex = twavObservationsIndex; 
+           uint8 _index = ((_prevIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
            TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];
-           TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
+       TwavObservation memory _twavObservationPrev = twavObservations[_prevIndex];
            _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);
        }
    }

G06 - No needed operations

Next lines:

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

Equivalent to:

primaryReserveBalance = fictitiousPrimaryReserveBalance;

[mundhrakeshav]

2, #3, #6, #7, #8, #9, #15